Re: [PATCH v2] USB:gadget: Fix a warning while loading g_mass_storage
Hi Alan, Sorry for my late response, as I was 000, Additionally, I am very grateful for your review. On 06/06/2014 02:08 AM, Alan Stern wrote: On Wed, 4 Jun 2014 wei.y...@windriver.com wrote: From: Yang Wei While loading g_mass_storage module, the following warning is triggered. 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 is that Robert once introduced the following patch to fix disconnect handling of s3c-hsotg. [ commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791 Author: Robert Baldyga 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 Signed-off-by: Kyungmin Park Signed-off-by: Felipe Balbi ] So it also means that s3c_hsotg_disconnect is called from SET_ADDRESS request handler, ultimately reset_config would finally be called, it raises a FSG_STATE_CONFIG_CHANGE exception, and set common->new_fsg to NULL, and then 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 not only raises a FSG_STATE_CONFIG_CHANGE and set common->new_fsg to new_fsg but also makes cdev->delayed_status plus one. If the execution ordering just likes the following scenario, the warning would be triggered. 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 | |-> Now the common->state = FSG_STATE_CONFIG and common->new_fsg is not equal to NULL |-> 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. So we also need to define a variable(struct fsg_dev *new) and then save common->new_fsg to it with the common->lock protection. Signed-off-by: Yang Wei --- drivers/usb/gadget/f_mass_storage.c |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Changes in v2: Only rephrase the commit log to make sense this issue. Wei diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_
Re: [PATCH v2] USB:gadget: Fix a warning while loading g_mass_storage
On Wed, 4 Jun 2014 wei.y...@windriver.com wrote: > From: Yang Wei > > While loading g_mass_storage module, the following warning is triggered. > > 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 is that Robert once introduced the following patch to fix > disconnect handling of s3c-hsotg. > [ > commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791 > Author: Robert Baldyga > 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 > Signed-off-by: Kyungmin Park > Signed-off-by: Felipe Balbi > ] > > So it also means that s3c_hsotg_disconnect is called from SET_ADDRESS request > handler, > ultimately reset_config would finally be called, it raises a > FSG_STATE_CONFIG_CHANGE exception, > and set common->new_fsg to NULL, and then 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 > not only raises a FSG_STATE_CONFIG_CHANGE and set common->new_fsg to new_fsg > but also makes > cdev->delayed_status plus one. If the execution ordering just likes the > following scenario, the warning > would be triggered. > > 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 > | >|-> Now the common->state = FSG_STATE_CONFIG and > common->new_fsg is not equal to NULL > |-> 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. > > So we also need to define a variable(struct fsg_dev *new) and then save > common->new_fsg > to it with the common->lock protection. > > Signed-off-by: Yang Wei > --- > drivers/usb/gadget/f_mass_storage.c |6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > Changes in v2: > > Only rephrase the commit log to make sense this issue. > > Wei > > diff --git a/drive
