| Message ID | xa1ty48tilai.fsf@mina86.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 4.04.2016 15:57, Michal Nazarewicz wrote: > On Sat, Apr 02 2016, Alan Stern wrote: >> On Sat, 2 Apr 2016, Michal Nazarewicz wrote: > > 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. > Who and when is going to destroy the thread if one does "/sys/bus/platform/drivers/musb-hdrc# echo musb-hdrc.0.auto > unbind"? Wouldn't some kind of refcounting make sense here? Regards, Ivo -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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). […] > 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. […] > 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. 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.
On Mon, Apr 04 2016, Ivaylo Dimitrov wrote: > Who and when is going to destroy the thread if one does > "/sys/bus/platform/drivers/musb-hdrc# echo musb-hdrc.0.auto > unbind"? > Wouldn't some kind of refcounting make sense here? Currently the thread is killed when fsg_common structure is released and that structure has a reference counter.
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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;