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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
