On Wed, 26 Nov 2014, Sebastian Andrzej Siewior wrote: > On 11/26/2014 04:24 PM, Alan Stern wrote: > >> On 11/25/2014 10:52 PM, Sebastian Andrzej Siewior wrote: > >>> The max packet size within the FS descriptor has to be highest possible > >>> value and _not_ the one that is (or will be) used on FS. > >> > >> The current code sets wMaxPacketSize of FS interrupt endpoint descriptor > >> as 64, which is in accordance with the usb 2.0 specification, section 5.7.3 > >> > >> The maximum allowable interrupt data payload size is 64 bytes > >> or less for full-speed. High-speed endpoints are allowed > >> maximum data payload sizes up to 1024 bytes. > > > > The real problem is that we are assuming endpoints can be allocated in > > a single way that will work correctly for all possible connection > > speeds. (I suspect it was done this way out of laziness and a desire > > to minimize the code size.) This assumption is obviously incorrect > > when the hardware has an interrupt endpoint that supports packet sizes > > of 64 but no larger. > > The code will check properly if you pass 1024 and check the size > accordingly. You have to "downsize" your FS descriptor later. This > function will only fail to do this if your gadget isn't dual speed. In > that case it expects 64 as the upper limit for INT (if I recall > correctly). It will also fail in situations where you use up a lot of endpoints. For example, let's say the UDC only supports 4 endpoints, one of which must have a maxpacket value <= 64. If you want to have four interrupt endpoints, you can't run at high speed -- but you can run at full speed. However, the approach you outlined above won't allow it. > But what you can't do (and this is done by ISOC and INT endpoint) is to > upgrade your capabilities by increasing the max packet size after you > received an endpoint. This "works" for ISOC because on FS you can use > up to 1023 bytes and the matching FIFO has usually 1024 bytes and not > 1023. It isn't correct but it works. > > This was done out of laziness and "simplicity" as far as I recall. > Usually the only difference between FS and HS is 0 so the only thing > you do is to update the bEndpointAddress member in HS/SS descriptors. > Nothing more. Most INT don't care for 1024 transfer size or anything > like that and the first gadgets in kernel were not ISOC and even these > days their MPS is < 200. > > So it was easy just to update the endpoint address for the HS > descriptor, you used the same endpoint as for FS. Most in tree gadgets > don't do more. > > > The right way to fix the problem is to avoid allocating endpoints until > > after the connection speed is known. Then we can call > > usb_ep_autoconfig() with the appropriate set of descriptors, and > > nothing will need to be fixed up. > > > > However, I don't know if this approach is compatible with the composite > > framework in its current form. > > I've been looking at this mess by the time I started the configfs. I > tried a few times and decided not now and there was no later. > You need all descriptors prior you connect / at bind time. At this time > you need also your endpoints allocated. A small change to this breaks > things in multiple places therefore the created configfs gadget is > "bound" once it is complete. I tried even to have "one descriptor" and > let the code create HS/SS descriptors out of it (since in 99% they are > the same) but a few gadgets did more and I dropped that idea again. > > So this is what is expected in most parts of the code as of now. Then > you have USB_DT_OTHER_SPEED_CONFIG where you may ask for HS descriptors > on a FS link. So you need those. You're right. It looks like we need to set up separate allocations of endpoints for all possible connection speeds, during initialization. If one of the allocations fails then the gadget must not be allowed to connect at that speed. (Unfortunately the gadget framework doesn't include any method for telling the hardware not to connect at a certain speed...) > All in all I tried to minimize the effects by leaving the descriptors > "untouched" and creating a copy after bind. I didn't manage to redo the > endpoint allocation. > Part of the problem is that you have no clear cut currently between > descriptor preparation and endpoint allocation. The endpoint allocator > does not know about HS/FS/SS. It knows that there is an endpoint, it > can do all speeds and has a special special upper limit (it may not be > able to INT or BULK or ISOC so it considers that as well). > Once it finds a match, it writes the endpoint address into the > descriptor and returns the pointer to the endpoint. So technically you > return two values here. The endpoint is considered taken once > ep->private is set (so it is a little racy). > Based on this you can't get the same endpoint on HS because it is gone. > You could but then you get another one. it will work but is a waste of > resources. > > Ah. And endpoints are never returned to the allocator. Some gadgets set > ->private to NULL, other just ignore it and let the core do it. So > re-doing the endpoint allocator is probably the right thing to do. And The allocator doesn't need to be changed. We already have a usb_ep_autoconfig_reset() function. > then force every gadget to allocate an endpoint for FS/HS/SS and give > it back _and_ please edit the copy of the descriptor and not the global > "original". Yes. > But the work will not be done before we have the next release is out > and as of now it breaks g_zero on musb, net2280 and maybe others. Felipe will have to decide how to handle this. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
