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

[Andrzej Pietrasiewicz]

Hi Alan,

W dniu 03.06.2014 16:48, Alan Stern pisze:
> On Tue, 3 Jun 2014 Wei.Yang@xxxxxxxxxxxxx wrote:
>
> > From: Yang Wei <Wei.Yang@xxxxxxxxxxxxx>
> >
> > 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/