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.
>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.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--
--
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
