Re: USB gadgets with configfs hang reboot

[Michal Nazarewicz <mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>]

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.

>> 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):

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;
}

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.

This is also why the thread is not stopped in fsg_unbind but only once
fsg_common structure is released.

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.

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.:

--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2979,20 +2979,7 @@ EXPORT_SYMBOL_GPL(fsg_common_set_inquiry_string);
 
 int fsg_common_run_thread(struct fsg_common *common)
 {
-       common->state = FSG_STATE_IDLE;
-       /* Tell the thread to start working */
-       common->thread_task =
-               kthread_create(fsg_main_thread, common, "file-storage");
-       if (IS_ERR(common->thread_task)) {
-               common->state = FSG_STATE_TERMINATED;
-               return PTR_ERR(common->thread_task);
-       }
-
-       DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task));
-
-       wake_up_process(common->thread_task);
-
-       return 0;
+       /* kill this function and all call sites */
 }
 EXPORT_SYMBOL_GPL(fsg_common_run_thread);
 
@@ -3005,6 +2992,7 @@ static void fsg_common_release(struct kref *ref)
        if (common->state != FSG_STATE_TERMINATED) {
                raise_exception(common, FSG_STATE_EXIT);
                wait_for_completion(&common->thread_notifier);
+               common->thread_task = NULL;
        }
 
        for (i = 0; i < ARRAY_SIZE(common->luns); ++i) {
@@ -3050,9 +3038,21 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
                if (ret)
                        return ret;
                fsg_common_set_inquiry_string(fsg->common, NULL, NULL);
-               ret = fsg_common_run_thread(fsg->common);
-               if (ret)
+       }
+
+       if (!common->thread_task) {
+               common->state = FSG_STATE_IDLE;
+               common->thread_task =
+                       kthread_create(fsg_main_thread, common, "file-storage");
+               if (IS_ERR(common->thread_task)) {
+                       int ret = PTR_ERR(common->thread_task);
+                       common->thread_task = NULL;
+                       common->state = FSG_STATE_TERMINATED;
                        return ret;
+               }
+               DBG(common, "I/O thread pid: %d\n",
+                   task_pid_nr(common->thread_task));
+               wake_up_process(common->thread_task);
        }
 
        fsg->gadget = gadget;

This should get rid of all the confusion and just do the right thing.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html