On Thu, Feb 16, 2012 at 11:29:45AM -0800, Michal Nazarewicz wrote: > On Wed, 15 Feb 2012 00:03:26 -0800, Felipe Balbi <balbi@xxxxxx> wrote: > > >On Tue, Feb 14, 2012 at 10:10:12PM -0800, Michal Nazarewicz wrote: > >>>On Fri, Feb 10, 2012 at 09:54:51AM +0100, Lukasz Majewski wrote: > >>>>It is crucial to assign each req->context value to struct rndis. > >>>> > >>>>The problem happens for multi function gadget (g_multi) when multiple > >>>>functions are calling common usb_composite_dev control request. > >>>> > >>>>It might happen that *_setup method from one usb function will > >>>>alter some fields of this common request issued by other USB > >>>>function. > >>>this is a bit weird, why would a function change a request which isn't > >>>supposed to handled by it ? I mean, functions need to check the ctrl > >>>request to be sure they should be handling it. > >> > >>It's composite layer who directs requests to functions. Functions do > >>not need to check the request. > > > >well, composite does call into functions correctly, but functions need > >to see if the control request is valid for them still. > > Once composite calls given function, the request is valid for them until > its completed or aborted. What I mean is that host could be sending a request with the correct RECIP_INTERFACE but wrong ctrl->bRequest. > >>>Your patch shouldn't be necessary, really, because the context is > >>>initialized when the request is allocated. > >> > >>Yes, but there is only one request for EP0 which is shared between > >>all the functions. > > > >true. > > > >>>what happens exactly ? Who changes the context field and why ? > >> > >>The context is changed by all other functions. > > > >I didn't see that, actually, look at f_acm: > > [...] > > >or f_eem: > > [...] > > >or f_ecm: > > [...] > > >or f_audio: > > [...] > > >or f_hid: > > [...] > > >or f_sourcesink: > > [...] > > >or f_uvc: > > [...] > > That's because those functions do not set req->complete. > > All functions that care when the request is finished and set req->complete > will probably need to set req->context as well (unless they don't use > req->context but that may lead to some global variables, yada yada). > > For all the other functions, which do not send req->complete, the request > will get handled by composite_setup_complete() which does not care about > context (as it only prints a message). > > >For all I know, the buggy gadget drivers are f_ncm, f_mass_storage and > >f_fs because they change a request which *does not* belong to them. > >The real fix would be to sort that out in another way. The ep0 request > >always belongs to composite.c so functions can't go around poking with > >that request. > > Of course they can. If I lend you my (hypothetical) car, I will let you > adjust the seat. Same here, composite may own the usb_ctrlrequest structure, > but by calling f->setup() it lends it to the function which can then adjust > it in ways it needs. Until request is completed or aborted, the function is > the only one that can use the request. > > >Maybe the best solution would be to allow ep0 to have multiple requests > >queued, then functions can allocate and maintain their own ep0 request for > >cases when they need to send anyting to the host, maybe. > > Maybe I'm missing something, but as far as I know, when function wants to send > data to host on EP0, host needs to initialise the transfer. This results in > composite_setup() being called, and until this request is finished (or aborted), > there will be no other control requests sent on EP0. On all other endpoints we allow multiple requests to be queued to the endpoint and each of them will be handled in sequence. Maybe we need to have the same for the control pipe, so different functions can queue their own stuff to ep0. Allowing for functions to change req->context will likely cause more issues if we decide to shuffle things around and change how composite handles requests. Besides, some of req->context changes could be done in a different way. In f_ncm.c for instance, it receives the Setup phase by using composite.c's request, but then it changes the setup phase request and requeues it to receive the data phase, and does nothing with the Status phase. This is one of the places which causes the most confusion and the most problems when writing UDC drivers for linux. There's very little "standardization" on who/how should Setup/Data/Status phases be handled on the framework. It's pretty much a trial and error approach. I say again that the best solution would be to allow functions to queue their own requests to ep0, rather than re-using composite.c's request. That way, we can build into the framework the notion that composite.c's request is for Setup phase ONLY and function is required to queue optinal Data and status phases. So a request such as SetNtbInputSize would require two usb_ep_queue from f_ncm, one for receiving the Data phase and one for sending Status phase, as a bonus we avoid the bug you mentioned on your original email. -- balbi
Attachment:
signature.asc
Description: Digital signature
