Messages in this thread | | | Date | Wed, 4 Jun 2014 10:34:22 +0800 | From | "Yang,Wei" <> | Subject | Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage |
| |
Guys,
It seems the previous description is out of order. I describe it again. Sorry for it.
irq handler | |-> s3c_hsotg_disconnect() | |-> common->new_fsg = NULL |-> common->state = FSG_STATE_CONFIG |-> wakes up fsg_main_thread. |->set USB device address.
fsg_main_thread | |-> handle_exception | |-> common->state = FSG_STATE_IDLE |-> do_set_interface() irq happens -------------->
irq handler needs to handle USB_REQ_SET_CONFIGURATION request | |-> set_config() | |-> common->new_fsg = new_fsg; |-> common->state = FSG_STATE_CONFIG |-> cdev->delayed_status++ |-> wakes up fsg_main_thread
fsg_main_thread | |-> if(common->new_fsg) |-> usb_composite_setup_continue() |-> cdev->delayed_status-- |-> fsg_main_thread still finds the common->state is equal to FSG_STATE_IDLE |-> so it invokes handle_exception again, subsequently the usb_composite_setup_continue |-> is executed again. It would fininally results in the warning.
Thanks Wei On 06/04/2014 09:20 AM, Yang,Wei wrote: > On 06/03/2014 10:48 PM, Alan Stern wrote: >> On Tue, 3 Jun 2014 Wei.Yang@windriver.com wrote: >> >>> From: Yang Wei <Wei.Yang@windriver.com> >>> >>> 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. >>> >>> irq thread >>> >>> composite_disconnect() >>> | >>> |->fsg_disable() fsg->common->new_fsg = NULL >>> and then wake fsg_main_thread >>> with seting common->state to >>> FSG_STATE_CONFIG_CHANGE. >>> fsg_main_thread >>> | >>> |->do_set_interface() >>> irq thread >>> >>> set_config() >>> | >>> |->fsg_set_alt() fsg->common->new_fsg = new_fsg >>> and then also wake up fsg_main_thread >>> with setting common->state to >>> FSG_STATE_CONFIG_CHANGE. >>> |-> >>> if(common->new_fsg) >>> usb_composite_setup_continue() >>> >>> 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. >> Michal and Andrzej: >> >> 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. > > commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791 > Author: Robert Baldyga <r.baldyga@samsung.com> > 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@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > > 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. > > irq handler > | > |-> s3c_hsotg_disconnect() > | > |-> 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 > | > |-> handle_execption > | > |-> set common->state to FSG_STATE_IDLE > irq hanppens ------------>| > irq handler needs to hanle USB_REQ_SET_CONFIGURATION request. > |->do_set_interface() > |-> set_config() > | > |-> common->new_fsg = new_fsg; > |-> common->state = FSG_STATE_CONFIG > |-> cdev->delayed_status++ > |-> wakes up fsg_main_thread > > |-> Now the common->state == FSG_STATE_CONFIG > |-> if(common->new_fsg) > usb_composite_setup_continue() > |->cdev->delayed_status-- > | > fsg_main_thread finds the common->state still is FSG_STATE_CONFIG, > | so it would invoke handle_execption again. > |->hanle_execption > |-> set common->state to FSG_STATE_IDLE > |-> do_set_interface() > |-> if (common->new_fsg) > usb_composite_setup_continue() > |-> cdev->delayed_status > == 0, so > this warning is triggered. > > > Thanks > Wei > >> >> 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 >> functions? >> >> Am I missing something? >> >> Alan Stern >> >> >> >
| |