On 06/03/2014 10:48 PM, Alan Stern wrote:
On Tue, 3 Jun 2014 Wei.Yang@xxxxxxxxxxxxx wrote:
From: Yang Wei <Wei.Yang@xxxxxxxxxxxxx>Michal and Andrzej:
While loading g_mass_storage module, the following warning is triggered.
In fact, it is more easy to reproduce it with RT kernel.
WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
The root cause just likes the following scenario.
|->fsg_disable() fsg->common->new_fsg = NULL
and then wake fsg_main_thread
with seting common->state to
|->fsg_set_alt() fsg->common->new_fsg = new_fsg
and then also wake up fsg_main_thread
with setting common->state to
In this case, fsg_main_thread would invoke usb_composite_setup_continue
twice, so the second call would trigger the above call trace, as we also
save common->new_fsg while changing the common->state.
I haven't paid much attention to these matters, because you handled the
conversion from g_file_storage to f_mass_storage using the composite
framework. But this patch seemed odd, so I took a closer look.
Let me make sense it, Robert once introduced the following patch to fix disconnect handling of s3c-hsotg.
Author: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
Date: Thu Nov 21 13:49:18 2013 +0100
usb: gadget: s3c-hsotg: fix disconnect handling
This patch moves s3c_hsotg_disconnect function call from USBSusp interrupt
handler to SET_ADDRESS request handler.
It's because disconnected state can't be detected directly, because this
hardware doesn't support Disconnected interrupt for device mode. For both
Suspend and Disconnect events there is one interrupt USBSusp, but calling
s3c_hsotg_disconnect from this interrupt handler causes config reset in
composite layer, which is not undesirable for Suspended state.
For this reason s3c_hsotg_disconnect is called from SET_ADDRESS request
handler, which occurs always after disconnection, so we do disconnect
immediately before we are connected again. It's probably only way we
can do handle disconnection correctly.
Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Signed-off-by: Felipe Balbi <balbi@xxxxxx>
Just like what the commit log described, s3c_hsotg_disconnect is called from SET_ADDRESS request handler, therefore,
reset_config would finally be called, it raises a FSG_STATE_CONFIG_CHANGE exception and wakes up fsg_main_thread to
handle this exception. After handling SET_ADDRESS, subsequently the irq hanler of s3c-hsotg would also invokes composite_setup()
function to handle USB_REQ_SET_CONFIGURATION request, set_config would be invoked, it also raises a FSG_STATE_CONFIG_CHANGE
exception and wakes up fsg_main_thread, in mean time, cdev->delayed_status would be plus one. Right? If I am missing something, please
let me know it.:-) If so, the following scenario would trigger the above call trace.
|-> common->new_fsg = NULL
|-> common->state to FSG_STATE_CONFIG.
|-> wakes up fsg_main_thread.
|-> set USB device address
fsg_main_thread finds the common->state == FSG_STATE_CONFIG
|-> set common->state to FSG_STATE_IDLE
irq hanppens ------------>|
irq handler needs to hanle USB_REQ_SET_CONFIGURATION request. |->do_set_interface()
|-> common->new_fsg = new_fsg;
|-> common->state = FSG_STATE_CONFIG
|-> wakes up fsg_main_thread
|-> Now the common->state == FSG_STATE_CONFIG
| fsg_main_thread finds the common->state still is FSG_STATE_CONFIG,
| so it would invoke handle_execption again.
|-> set common->state to FSG_STATE_IDLE
|-> if (common->new_fsg)
|-> cdev->delayed_status == 0, so
this warning is triggered.
In f_mass_storage.c, struct fsg_common is shared among all the function
instances. This structure includes things like cmnd and nluns, which
will in general be different for different functions.
That's okay if each function is in a separate config, but what happens
when there are multiple functions in the same config, using different
interfaces? What if the host sends concurrent commands to two of these
Am I missing something?