Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage

[Andrzej Pietrasiewicz <andrzej.p@samsung.com>]

Hi Alan,

W dniu 03.06.2014 16:48, Alan Stern pisze:
> 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.

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