On Mon, 4 Apr 2016, Michal Nazarewicz wrote:
> On Sat, Apr 02 2016, Alan Stern wrote:
> > On Sat, 2 Apr 2016, Michal Nazarewicz wrote:
> >> At the same time, mass storage should work fine if it’s bound to
> >> multiple configurations. Only one configuration can be active at any
> >> given time so interfaces on different configurations cannot interfere
> >> with each other.
>
> > Yes, it _should_. But it doesn't with the nokia legacy driver.
> > I don't know if this has any connection with configfs; it could be
> > a problem with the way f_mass_storage interacts with the composite
> > core.
>
> I believe the failure is related to the thread being started twice where
> it indeed shouldn’t.
Yes, of course. The questions we need to answer are: Why is the thread
being started twice, and how can we fix it?
> >> The problem we are having is that when mass storage is added to
> >> a configuration, fsg_bind is called and it starts the thread.
>
> > This is what I'm not sure about. Which callbacks does the composite
> > core invoke when a config is installed or uninstalled?
>
> usb_add_config is what is called to create a usb_configuration. It
> initialises the structure and passes it to a callback bind function
> (most code removed for brevity):
Note: I did not ask what happens when a configuration is _created_; I
asked what happens when a configuration is _installed_ -- that is, when
the host sends a Set-Config request.
> int usb_add_config(struct usb_composite_dev *cdev,
> struct usb_configuration *config,
> int (*bind)(struct usb_configuration *))
> {
> usb_add_config_only(cdev, config);
> bind(config);
> /* set_alt(), or next bind(), sets up ep->claimed as needed */
> usb_ep_autoconfig_reset(cdev->gadget);
> return 0;
> }
>
> The bind callback calls usb_add_function to add usb_function to a newly
> created usb_configuration (again, most code removed for brevity):
>
> int usb_add_function(struct usb_configuration *config,
> struct usb_function *function)
> {
> function->config = config;
> value = function->bind(config, function);
> return 0;
> }
So there is no way to add a single function to several configurations?
> For mass_storage, function->bind is fsg_bind (ditto):
>
> static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
> {
> struct fsg_dev *fsg kzalloc(sizeof(*fsg), GFP_KERNEL);
> fsg->function.name = FSG_DRIVER_DESC;
> fsg->function.bind = fsg_bind; /* !!! */
> fsg->function.unbind = fsg_unbind;
> fsg->function.setup = fsg_setup;
> fsg->function.set_alt = fsg_set_alt;
> fsg->function.disable = fsg_disable;
> fsg->function.free_func = fsg_free;
> fsg->common = common;
> return &fsg->function;
> }
>
> > Those callbacks should be where the thread is started and stopped.
>
> Starting thread in that callback is what is happening right now and
> because single usb_function_instance can have multiple usb_function
> structures each binding to separate usb_configuration, this causes
> problem where the thread is started multiple times.
It sounds like there are two problems then. The first problem is that
struct usb_function has a ->disable() callback but no ->enable()
callback. The set_config() routine in composite.c should invoke the
->enable() callback for each function in the config when the config is
installed.
The second problem is that f_mass_storage should start the thread in
the enable callback and stop the thread in the disable callback, rather
than in fsg_bind() and fsg_free_inst() (or wherever).
> This is also why the thread is not stopped in fsg_unbind but only once
> fsg_common structure is released.
This design is a holdover from the days before the composite core
existed and g_file_storage was a standalone gadget driver. It could
stand to be updated.
> Conceptually, the thread should be started when fsg_common structure is
> created (which is at the same time when usb_function_instance is
> created) and stopped when fsg_common is released.
>
> At this point, I’m not entirely sure if there is a reason why this isn’t
> the case. The only reason I can think of is that starting the thread
> right away may be considered wasteful since the thread won’t have
> anything to do until the function is bound to a configuration. In the
> current code, there may also be issues where perhaps the thread would
> not get stopped if fsg_bind has never been called.
Even after the function is bound to a configuration, the thread won't
have anything to do until the configuration is installed by the host.
> Because of all that, I think the best course of action is to just check
> whether the thread is running and conditionally start it in fsg_bind,
> i.e.:
> This should get rid of all the confusion and just do the right thing.
Except that you'll have a bunch of threads hanging around with nothing
to do. They shouldn't be created until it is known that they will be
needed, and they should be destroyed when it is known that they won't
have any more work to do.
That's my opinion; you may disagree.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
