On Mon, 4 Apr 2016, Michal Nazarewicz wrote:
> On Mon, Apr 04 2016, Alan Stern wrote:
> > So there is no way to add a single function to several configurations?
>
> There is. f_mass_storage (and any other usb_function_instance) simply
> has to have multiple usb_function structures.
>
> > 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.
>
> Well, there is set_alt which is called when configuration is chosen (as
> well as when interface’s alternative is chosen). It’s not ideal but if
> we had to we could work with that.
I suppose so.
> > 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.
>
> I’m not sure how big of a deal that is. The flip side is that equating
> thread’s lifetime to the lifetime of the function instance is
> conceptually easier. Even with thread started in fsg_bind this is still
> less complex than having the thread pop in and out of existence.
True. (Provided one understands that a function instance corresponds
to the usb_function structure, not the usb_function_instance
structure.)
> Furthermore, having it start and stop may lead to unnecessary delays
> when host switches configurations. This may be an esoteric problem
> though.
>
> I’m not married to any either idea, but right now my patch is a better
> course of action because it brings a quick fix to the bug. More
> dramatic changes to thread’s lifetime should be handled by separate
> patches.
Okay. However, I noticed your patch does:
> + if (!common->thread_task) {
> + common->state = FSG_STATE_IDLE;
> + common->thread_task =
> + kthread_create(fsg_main_thread, common,
in fsg_bind(). How could thread_task ever be non-NULL at this point?
Wouldn't that mean the usb_function structure was registered twice?
Which brings us back to the nokia driver. Apparently it _does_ manage
to create the thread twice. How can this happen? The function that
nokia_bind_config() registers is created dynamically:
f_msg = usb_get_function(fi_msg);
which goes through fsg_alloc().
Aha, and now I see the problem. fsg_alloc() shares opts->common among
all the fsg structures it creates. This means all the function
instances will share common->thread_task, because they all share
common. The same goes for common->state and a bunch of other fields.
It looks like a lot of stuff has to move out of fsg->common and into
fsg.
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
