From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755276AbaFDCe4 (ORCPT ); Tue, 3 Jun 2014 22:34:56 -0400 Received: from mail.windriver.com ([147.11.1.11]:34954 "EHLO mail.windriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755214AbaFDCex (ORCPT ); Tue, 3 Jun 2014 22:34:53 -0400 Message-ID: <538E85AE.5080601@windriver.com> Date: Wed, 4 Jun 2014 10:34:22 +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: "Yang,Wei" , Alan Stern , Michal Nazarewicz , Andrzej Pietrasiewicz CC: Felipe Balbi , , USB list , Kernel development list Subject: Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage References: <538E745B.1020200@windriver.com> In-Reply-To: <538E745B.1020200@windriver.com> 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 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 >>> >>> 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 > 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 > > 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 >> >> >> >