Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On Thu, 5 Jun 2014, Andrzej Pietrasiewicz wrote: > I think it is easier to tell the purpose of the two structures taking > gadgets composed with configfs as example. > > In each gadget there is "functions" directory. Function directories > are created there: > > $ cd $CONFIGFS_ROOT/usb_gadget/our_gadget > $ mkdir functions/mass_storage.0 > > mass_storage.0 is internally represented as an instance of > struct usb_function_instance, which has its associated > struct fsg_common (the fsg_common is a member of > container_of(struct usb_function_instance)). Okay. > It can be referenced from multiple configurations. > > $ ln -s functions/mass_storage.0 configs/config.1 > $ ln -s functions/mass_storage.0 configs/config.2 > > Each reference (symbolic link) is internally represented as > an instance of struct usb_function. The struct usb_function here > has its associated struct fsg_dev (the fsg_dev is a > container_of(struct usb_function)). So in essence, a usb_function connects a particular function to a particular config. > By the very nature of any file system one cannot do: > > $ ln -s functions/mass_storage.0 configs/config.1 > $ ln -s functions/mass_storage.0 configs/config.1 => -EEXIST > > By design of how configfs is applied to any usb gadget on cannot even do: > > $ ln -s functions/mass_storage.0 configs/config.1/my_mass_storage.0 > $ ln -s functions/mass_storage.0 configs/config.1/the_same_mass_storage.0 => > -EEXIST So for any given usb_function_instance, only one usb_function will be active at any time: the one connecting the function to the current config. And presumably the reasons why struct fsg_dev contains interface_number, bulk_in, and bulk_out members are because these values are determined when the config is originally set up, and they can vary from one config to another. Right? Whereas the items in struct fsg_common don't depend on the config. > However, there should be no problem with this: > > $ mkdir functions/mass_storage.0 > $ mkdir functions/mass_storage.1 > $ ln -s functions/mass_storage.0 configs/config.1 > $ ln -s functions/mass_storage.1 configs/config.1 That makes sense now. > Legacy gadgets (g_mass_storage, g_acm_ms, g_multi) in fact operate in > a somewhat similar manner, the difference is that instead of creating > directories > and making symbolic links, usb_get_function_instance() and usb_get_function() > are called, respectively, and composing a gadget happens from beginning to end > at module init. > > I hope this clarifies things a bit. Yes, it helps a lot. Thank you. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On Thu, 5 Jun 2014, Peter Chen wrote: > > > > It is a little strange we call gadget's disconnect at SET_ADDRESS? > > > > How the udc calls gadget driver the disconnection has happened when > > > > the usb cable is disconnected from the host? > > > > > > > > Usually, we call gadget's disconnect at two situations > > > > > > > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually > > > > happened when the host sends reset after enumeration. > > > > - udc's disconnect handler, it is usually happened when the usb > > > > cable is disconnected from host. > > > > > > Hmm, usually the two situations, but according to the commit log, s3c > > > hsotg does not support Disconnected interrupt for device mode, so the > > > second situation does not happen for s3c hsotg, therefore, he has to > > > disconnect the connection before it is connected again. > > > > Why does he need to do that? Why not just skip the disconnect > > notification if the hardware can't detect a disconnect? > > > > We must call gadget driver's disconnect, otherwise, there at least > has a warning when unload module, please refer to __composite_unbind > at drivers/usb/gadget/composite.c The disconnect callback can run just before unbind. That's not a valid reason for doing a disconnect notification as part of SET_ADDRESS. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
W dniu 04.06.2014 17:26, Alan Stern pisze: On Wed, 4 Jun 2014, Andrzej Pietrasiewicz wrote: What is the difference in purpose between usb_function and usb_function_instance? I can't tell just by reading the header file. Does one of them get created dynamically when the host sets the appropriate config? Please see below. Among traditional gadgets there is no gadget which uses mass storage function more than once. On the other hand, when gadgets are created with configfs it is forbidden to link a given function more than once into a given config, What is the reason for this restriction? Please see below. that is a struct usb_function_instance can be referenced by more than one config, but can be referenced only once in a given config; each symbolic link corresponds to an instance of struct usb_function (don't confuse with struct usb_function_instance). It's extremely easy to confuse them, since I don't understand the differences between them. It even seems like you confused them in this description: You mentioned "link a given function", "link corresponds to an instance of struct usb_function", and "struct usb_function_instance can be referenced by more than one config". What's the difference between linking a usb_function and referencing a usb_function_instance? Normally "linking" and "referencing" mean more or less the same thing. As I said, I didn't like the naming here. I got used to it, though, but understand (and agree) that it is confusing. As far as explaining the difference is concerned, being a non-native speaker of English has its influence, too, so let me do it again. I think it is easier to tell the purpose of the two structures taking gadgets composed with configfs as example. In each gadget there is "functions" directory. Function directories are created there: $ cd $CONFIGFS_ROOT/usb_gadget/our_gadget $ mkdir functions/mass_storage.0 mass_storage.0 is internally represented as an instance of struct usb_function_instance, which has its associated struct fsg_common (the fsg_common is a member of container_of(struct usb_function_instance)). It can be referenced from multiple configurations. $ ln -s functions/mass_storage.0 configs/config.1 $ ln -s functions/mass_storage.0 configs/config.2 Each reference (symbolic link) is internally represented as an instance of struct usb_function. The struct usb_function here has its associated struct fsg_dev (the fsg_dev is a container_of(struct usb_function)). By the very nature of any file system one cannot do: $ ln -s functions/mass_storage.0 configs/config.1 $ ln -s functions/mass_storage.0 configs/config.1 => -EEXIST By design of how configfs is applied to any usb gadget on cannot even do: $ ln -s functions/mass_storage.0 configs/config.1/my_mass_storage.0 $ ln -s functions/mass_storage.0 configs/config.1/the_same_mass_storage.0 => -EEXIST However, there should be no problem with this: $ mkdir functions/mass_storage.0 $ mkdir functions/mass_storage.1 $ ln -s functions/mass_storage.0 configs/config.1 $ ln -s functions/mass_storage.1 configs/config.1 Legacy gadgets (g_mass_storage, g_acm_ms, g_multi) in fact operate in a somewhat similar manner, the difference is that instead of creating directories and making symbolic links, usb_get_function_instance() and usb_get_function() are called, respectively, and composing a gadget happens from beginning to end at module init. I hope this clarifies things a bit. AP -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
> On Wed, 4 Jun 2014, Yang,Wei wrote: > > > On 06/04/2014 09:45 AM, Peter Chen wrote: > > > > > >> 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 is a little strange we call gadget's disconnect at SET_ADDRESS? > > > How the udc calls gadget driver the disconnection has happened when > > > the usb cable is disconnected from the host? > > > > > > Usually, we call gadget's disconnect at two situations > > > > > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually > > > happened when the host sends reset after enumeration. > > > - udc's disconnect handler, it is usually happened when the usb > > > cable is disconnected from host. > > > > Hmm, usually the two situations, but according to the commit log, s3c > > hsotg does not support Disconnected interrupt for device mode, so the > > second situation does not happen for s3c hsotg, therefore, he has to > > disconnect the connection before it is connected again. > > Why does he need to do that? Why not just skip the disconnect > notification if the hardware can't detect a disconnect? > We must call gadget driver's disconnect, otherwise, there at least has a warning when unload module, please refer to __composite_unbind at drivers/usb/gadget/composite.c Peter > It makes no sense at all to call a disconnect handler from within the > SET_ADDRESS routine. > > Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
> From: linux-usb-ow...@vger.kernel.org > [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Alan Stern > Sent: Wednesday, June 04, 2014 6:57 AM > > On Wed, 4 Jun 2014, Yang,Wei wrote: > > > On 06/04/2014 09:45 AM, Peter Chen wrote: > > > > > >> 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 is a little strange we call gadget's disconnect at SET_ADDRESS? > > > How the udc calls gadget driver the disconnection has happened when > > > the usb cable is disconnected from the host? > > > > > > Usually, we call gadget's disconnect at two situations > > > > > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually > > > happened > > > when the host sends reset after enumeration. > > > - udc's disconnect handler, it is usually happened when the usb cable > > > is disconnected from host. > > > > Hmm, usually the two situations, but according to the commit log, s3c > > hsotg does not support Disconnected interrupt for device mode, > > so the second situation does not happen for s3c hsotg, therefore, he has > > to disconnect the connection before it is connected again. > > Why does he need to do that? Why not just skip the disconnect > notification if the hardware can't detect a disconnect? > > It makes no sense at all to call a disconnect handler from within the > SET_ADDRESS routine. FWIW, here is the section from the DWC2 databook that describes how a disconnect should be handled for the device: When OTG_MODE is set to 0, 1, or 3, the device disconnect flow is as follows: 1. When the USB cable is unplugged or when the VBUS is switched off by the Host, the Device core triggers GINTSTS.OTGInt [bit 2] interrupt bit. 2. When the device application detects GINTSTS.OTGInt interrupt, it checks that the GOTGINT.SesEndDet (Session End Detected) bit is set to 1'b1. When OTG_MODE is set to 2 or 4, the device disconnect flow is as follows: 1. When the USB cable is unplugged or when the VBUS is switched off by the Host, the Device core triggers GINTSTS.USBRst [bit 12] interrupt bit. 2. When the device application detects GINTSTS.USBRst, the application sets a timeout check for SET ADDRESS Control Xfer from Host. 3. If application does not receive SET ADDRESS Control Xfer from Host before the timeout period, it is treated as a device disconnection. OTG_MODE is a configuration parameter that is set when the core is built. From this discussion, it sounds like the s3c-hsotg core is built for either mode 2 or 4. So SET ADDRESS should be involved, but not in the way the driver is currently doing it. Unfortunately I don't have the s3c-hsotg hardware, so I can't work on this myself. -- Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On Wed, 4 Jun 2014, Andrzej Pietrasiewicz wrote: > When Sebastian introduced the function registration interface I didn't > specially like the naming: struct usb_function_instance is something > different than an instance of struct usb_function. What is the difference in purpose between usb_function and usb_function_instance? I can't tell just by reading the header file. Does one of them get created dynamically when the host sets the appropriate config? It's quite noticeable that composite.h does not contain nearly enough documentation. Only four of the structures defined there have any kerneldoc, and none of the functions do. Also, there seems to be some confusion between structures that represent drivers and those that represent devices (or parts of a device). For example, struct usb_function contains instance data as well as driver callbacks. > The purpose of fsg_alloc_inst() is to create a usb_function_instance > whose container_of is struct fsg_opts. In fact it is struct fsg_opts > which is actually allocated; one of its members is struct fsg_common > which is also allocated - individually for each struct usb_function_instance. > > Among traditional gadgets there is no gadget which uses mass storage function > more than once. On the other hand, when gadgets are created with configfs > it is forbidden to link a given function more than once into a given > config, What is the reason for this restriction? > that is a struct usb_function_instance can be referenced by more > than one config, but can be referenced only once in a given config; > each symbolic link corresponds to an instance of struct usb_function > (don't confuse with struct usb_function_instance). It's extremely easy to confuse them, since I don't understand the differences between them. It even seems like you confused them in this description: You mentioned "link a given function", "link corresponds to an instance of struct usb_function", and "struct usb_function_instance can be referenced by more than one config". What's the difference between linking a usb_function and referencing a usb_function_instance? Normally "linking" and "referencing" mean more or less the same thing. > So yes, an fsg_common can be shared among instances of struct usb_function, > but neither with traditional gadgets as they are now nor with configfs > is it possible to have the same fsg_common referenced more than once > in a given config. That's a relief. But it still seems like a bad design. If there can be only one struct fsg_dev associated with struct fsg_common, why have separate structures? And if there can be multiple fsg_dev structures associated with struct fsg_common, why does struct fsg_common contain a pointer to an fsg_dev (in fact, two of them)? The issue that started these thoughts was the way fsg_common.new_fsg gets used, as modified by the patch in the thread's original email. It's not clear why new_fsg is needed at all. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On Wed, 4 Jun 2014, Yang,Wei wrote: > On 06/04/2014 09:45 AM, Peter Chen wrote: > > > >> 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 is a little strange we call gadget's disconnect at SET_ADDRESS? > > How the udc calls gadget driver the disconnection has happened when > > the usb cable is disconnected from the host? > > > > Usually, we call gadget's disconnect at two situations > > > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually happened > > when the host sends reset after enumeration. > > - udc's disconnect handler, it is usually happened when the usb cable > > is disconnected from host. > > Hmm, usually the two situations, but according to the commit log, s3c > hsotg does not support Disconnected interrupt for device mode, > so the second situation does not happen for s3c hsotg, therefore, he has > to disconnect the connection before it is connected again. Why does he need to do that? Why not just skip the disconnect notification if the hardware can't detect a disconnect? It makes no sense at all to call a disconnect handler from within the SET_ADDRESS routine. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
Hi Alan, W dniu 03.06.2014 16:48, Alan Stern pisze: On Tue, 3 Jun 2014 wei.y...@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. Actually when I started dealing with usb gadgets the f_mass_storage had already been there. My involvement started with some cleanup, then moving to the new function registration interface (usb_get/put_function_instance(), usb_get/put_function()) and adding configfs support. That said, it is not impossible for me to have spoilt something :O 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? When Sebastian introduced the function registration interface I didn't specially like the naming: struct usb_function_instance is something different than an instance of struct usb_function. The purpose of fsg_alloc_inst() is to create a usb_function_instance whose container_of is struct fsg_opts. In fact it is struct fsg_opts which is actually allocated; one of its members is struct fsg_common which is also allocated - individually for each struct usb_function_instance. Among traditional gadgets there is no gadget which uses mass storage function more than once. On the other hand, when gadgets are created with configfs it is forbidden to link a given function more than once into a given config, that is a struct usb_function_instance can be referenced by more than one config, but can be referenced only once in a given config; each symbolic link corresponds to an instance of struct usb_function (don't confuse with struct usb_function_instance). So yes, an fsg_common can be shared among instances of struct usb_function, but neither with traditional gadgets as they are now nor with configfs is it possible to have the same fsg_common referenced more than once in a given config. AP -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
> On 06/04/2014 09:45 AM, Peter Chen wrote: > > > >> 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 is a little strange we call gadget's disconnect at SET_ADDRESS? > > How the udc calls gadget driver the disconnection has happened when > > the usb cable is disconnected from the host? > > > > Usually, we call gadget's disconnect at two situations > > > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually > > happened when the host sends reset after enumeration. > > - udc's disconnect handler, it is usually happened when the usb cable > > is disconnected from host. > > Hmm, usually the two situations, but according to the commit log, s3c > hsotg does not support Disconnected interrupt for device mode, so the > second situation does not happen for s3c hsotg, therefore, he has to > disconnect the connection before it is connected again. > Then, it seems Samsung uses other solution to detect disconnection, otherwise how it notifies the app it has disconnected, eg, we disconnect usb cable with pc for android phone, the usb icon should be disappeared. Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On 06/04/2014 09:45 AM, Peter Chen wrote: 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 is a little strange we call gadget's disconnect at SET_ADDRESS? How the udc calls gadget driver the disconnection has happened when the usb cable is disconnected from the host? Usually, we call gadget's disconnect at two situations - udc's reset handler if udc's speed is not UNKNOWN, it is usually happened when the host sends reset after enumeration. - udc's disconnect handler, it is usually happened when the usb cable is disconnected from host. Hmm, usually the two situations, but according to the commit log, s3c hsotg does not support Disconnected interrupt for device mode, so the second situation does not happen for s3c hsotg, therefore, he has to disconnect the connection before it is connected again. Wei Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.y...@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
RE: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
> > 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 is a little strange we call gadget's disconnect at SET_ADDRESS? How the udc calls gadget driver the disconnection has happened when the usb cable is disconnected from the host? Usually, we call gadget's disconnect at two situations - udc's reset handler if udc's speed is not UNKNOWN, it is usually happened when the host sends reset after enumeration. - udc's disconnect handler, it is usually happened when the usb cable is disconnected from host. Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On 06/03/2014 10:48 PM, Alan Stern wrote: On Tue, 3 Jun 2014 wei.y...@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->delay
Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
On Tue, 3 Jun 2014 wei.y...@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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
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. Signed-off-by: Yang Wei --- drivers/usb/gadget/f_mass_storage.c |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Hi All, This patch is based on git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-next branch, and is validated it with altera cyclone V board. Thanks 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; unsigned intexception_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; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
