Re: USB gadgets with configfs hang reboot

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux