Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754062AbaFIGCl (ORCPT ); Mon, 9 Jun 2014 02:02:41 -0400 Received: from mail.windriver.com ([147.11.1.11]:34101 "EHLO mail.windriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752722AbaFIGCj (ORCPT ); Mon, 9 Jun 2014 02:02:39 -0400 Message-ID: <53954DDE.5070407@windriver.com> Date: Mon, 9 Jun 2014 14:02:06 +0800 From: "Yang,Wei" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: Alan Stern CC: , , , , , Subject: Re: [PATCH v2] USB:gadget: Fix a warning while loading g_mass_storage References: In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [128.224.162.170] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.Yang@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_mass_storage.c >> index b963939..e3b1798 100644 >> --- a/drivers/usb/gadget/f_mass_storage.c >> +++ b/drivers/usb/gadget/f_mass_storage.c >> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common) >> struct fsg_buffhd *bh; >> enum fsg_state old_state; >> struct fsg_lun *curlun; >> + struct fsg_dev *new; > The spacing is different from the lines above. There should be two tab > characters between the 'v' and the '*', not three spaces. > > Also, the variable should be named "new_fsg", not "new". Okay, I would fix it in v3. > >> unsigned int exception_req_tag; >> >> /* >> @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common) >> } >> common->state = FSG_STATE_IDLE; >> } >> + new = common->new_fsg; >> spin_unlock_irq(&common->lock); >> >> /* Carry out any extra actions required for the exception */ >> @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common) >> break; >> >> case FSG_STATE_CONFIG_CHANGE: >> - do_set_interface(common, common->new_fsg); >> - if (common->new_fsg) >> + do_set_interface(common, new); >> + if (new) >> usb_composite_setup_continue(common->cdev); >> break; > The description is long-winded and confusing, but the patch is correct. > The existing code fails to take into account the possibility that > common->new_fsg can change while do_set_interface() is running, because > the spinlock isn't held at this point. Yes, I used a long description to want to point out that the common->new_fsg can be changed while do_set_interface is running. I would use the above your description instead of mine in v3. Thanks Wei > > Alan Stern > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/