| Message ID | xa1tmvpdz2h4.fsf@mina86.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 1 Apr 2016, Michal Nazarewicz wrote: > On Thu, Mar 31 2016, Alan Stern wrote: > > Michal, I'm not sure how you intended to handle this. > > For legacy/nokia setting no_configfs should be a valid solution: > > ---- >8 ---------------------------------------------------------------- > diff --git a/drivers/usb/gadget/legacy/nokia.c b/drivers/usb/gadget/legacy/nokia.c > index 0997504..e4bc607 100644 > --- a/drivers/usb/gadget/legacy/nokia.c > +++ b/drivers/usb/gadget/legacy/nokia.c > @@ -223,6 +223,7 @@ static int nokia_bind_config(struct usb_configuration *c) > } > > fsg_opts = fsg_opts_from_func_inst(fi_msg); > + fsg_opts->no_configfs = true; > > status = fsg_common_run_thread(fsg_opts->common); > if (status) > ---- >8 ---------------------------------------------------------------- > > This is what other legacy gadgets do after all. Okay, that makes sense. Legacy gadgets aren't meant to be used with configfs anyway. > For configfs, why not simply check if the thread has already been > started: > > ---- >8 ---------------------------------------------------------------- > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index acf210f..62854b6 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -2984,8 +2984,10 @@ int fsg_common_run_thread(struct fsg_common *common) > 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 PTR_ERR(common->thread_task); > + return ret; > } > > DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task)); > @@ -3005,6 +3007,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 +3053,11 @@ 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) > - return ret; > + if (common->thread_task) { Do you mean: if (!common->thread_task)? > + ret = fsg_common_run_thread(fsg->common); > + if (ret) > + return ret; > + } > } > > fsg->gadget = gadget; > ---- >8 ---------------------------------------------------------------- > > With that, the whole fsg_common_run_thread call should be moved outside > of the if (!no_configfs) and legacy should no longer call it IMO. I'm not so sure about this. Earlier I was undecided about what to do if there are multiple mass-storage interfaces in the same config, but now I realize that one thread won't work in that situation. The difficulty is exception handling. If an error occurs in one of the interfaces, it shouldn't affect the others -- but with only one thread you can't make that work. I don't know the details of how the composite core works (I don't even remember the difference between a usb_function and a usb_function_instance). Suppose there are several interfaces in one config, each bound to the mass-storage driver. Then won't the driver's bind routine get called several times whenever the config is installed? If so, then each of those calls should create a new thread. There's no need to check if the thread has already been started. If not, then there's no way to use f_mass_storage on multiple interfaces in a single config. The driver should refuse to run on more than one interface at a time. 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
> On Fri, 1 Apr 2016, Michal Nazarewicz wrote: >> @@ -3050,9 +3053,11 @@ 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) >> - return ret; >> + if (common->thread_task) { > On Fri, Apr 01 2016, Alan Stern wrote: > Do you mean: if (!common->thread_task)? Correct. >> + ret = fsg_common_run_thread(fsg->common); >> + if (ret) >> + return ret; >> + } >> } >> >> fsg->gadget = gadget; >> ---- >8 ---------------------------------------------------------------- >> >> With that, the whole fsg_common_run_thread call should be moved outside >> of the if (!no_configfs) and legacy should no longer call it IMO. > > I'm not so sure about this. Earlier I was undecided about what to do > if there are multiple mass-storage interfaces in the same config, but > now I realize that one thread won't work in that situation. The > difficulty is exception handling. If an error occurs in one of the > interfaces, it shouldn't affect the others -- but with only one thread > you can't make that work. > > I don't know the details of how the composite core works (I don't even > remember the difference between a usb_function and > a usb_function_instance). configfs made things quite confusing, yes. IIRC, usb_function roughly corresponds to an interface while usb_function_instance corresponds to a module/function driver, except you can easily create multiple instances. > Suppose there are several interfaces in one config, each bound to the > mass-storage driver. Then won't the driver's bind routine get called > several times whenever the config is installed? > > If so, then each of those calls should create a new thread. There's no > need to check if the thread has already been started. > > If not, then there's no way to use f_mass_storage on multiple > interfaces in a single config. The driver should refuse to run on > more than one interface at a time. Right, but that’s not what is happening. We have a situation where single mass storage instance is bound to two interfaces on two different configs: mkdir functions/mass_storage.0 echo $file > functions/mass_storage.0/lun.0/file ln -s functions/mass_storage.0 configs/c.1 ln -s functions/mass_storage.0 configs/c.2 configfs doesn’t even allows to express situation where a single instance of a function is bound to multiple interfaces in a single configuration (IIRC). 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. The problem we are having is that when mass storage is added to a configuration, fsg_bind is called and it starts the thread. If someone wanted to have two mass storage interfaces on a single configuration they would have to create two mass storage instances and each instance has it’s own fsg_common struct (see fsg_alloc_inst function). PS. It seems that legacy/multi is broken because it calls fsg_common_run_thread twice.
On Sat, 2 Apr 2016, Michal Nazarewicz wrote: > > I'm not so sure about this. Earlier I was undecided about what to do > > if there are multiple mass-storage interfaces in the same config, but > > now I realize that one thread won't work in that situation. The > > difficulty is exception handling. If an error occurs in one of the > > interfaces, it shouldn't affect the others -- but with only one thread > > you can't make that work. > > > > I don't know the details of how the composite core works (I don't even > > remember the difference between a usb_function and > > a usb_function_instance). > > configfs made things quite confusing, yes. IIRC, usb_function roughly > corresponds to an interface while usb_function_instance corresponds to > a module/function driver, except you can easily create multiple > instances. That sounds right. I vaguely remember when I did look at this stuff years ago, the names were backward -- usb_function refers to an instance while usb_function_instance refers to a driver (which obviously doesn't have instances). > > Suppose there are several interfaces in one config, each bound to the > > mass-storage driver. Then won't the driver's bind routine get called > > several times whenever the config is installed? > > > > If so, then each of those calls should create a new thread. There's no > > need to check if the thread has already been started. > > > > If not, then there's no way to use f_mass_storage on multiple > > interfaces in a single config. The driver should refuse to run on > > more than one interface at a time. > > Right, but that’s not what is happening. We have a situation where > single mass storage instance is bound to two interfaces on two different > configs: > > mkdir functions/mass_storage.0 > echo $file > functions/mass_storage.0/lun.0/file > ln -s functions/mass_storage.0 configs/c.1 > ln -s functions/mass_storage.0 configs/c.2 > > configfs doesn’t even allows to express situation where a single > instance of a function is bound to multiple interfaces in a single > configuration (IIRC). That doesn't matter for our purposes now. > 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. > 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? Those callbacks should be where the thread is started and stopped. > If someone wanted to have two mass storage interfaces on a single > configuration they would have to create two mass storage instances and > each instance has it’s own fsg_common struct (see fsg_alloc_inst > function). > > PS. It seems that legacy/multi is broken because it calls > fsg_common_run_thread twice. Perhaps because f_mass_storage calls fsg_common_run_thread from the wrong place. 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
On Fri, Apr 01 2016, Michal Nazarewicz wrote: > For legacy/nokia setting no_configfs should be a valid solution: > > ---- >8 ---------------------------------------------------------------- > diff --git a/drivers/usb/gadget/legacy/nokia.c b/drivers/usb/gadget/legacy/nokia.c > index 0997504..e4bc607 100644 > --- a/drivers/usb/gadget/legacy/nokia.c > +++ b/drivers/usb/gadget/legacy/nokia.c > @@ -223,6 +223,7 @@ static int nokia_bind_config(struct usb_configuration *c) > } > > fsg_opts = fsg_opts_from_func_inst(fi_msg); > + fsg_opts->no_configfs = true; > > status = fsg_common_run_thread(fsg_opts->common); > if (status) > ---- >8 ---------------------------------------------------------------- > > This is what other legacy gadgets do after all. Scratch that. legacy/nokia sets no_configfs in nokia_bind so legacy/nokia appears to be a different issue all together.
diff --git a/drivers/usb/gadget/legacy/nokia.c b/drivers/usb/gadget/legacy/nokia.c index 0997504..e4bc607 100644 --- a/drivers/usb/gadget/legacy/nokia.c +++ b/drivers/usb/gadget/legacy/nokia.c @@ -223,6 +223,7 @@ static int nokia_bind_config(struct usb_configuration *c) } fsg_opts = fsg_opts_from_func_inst(fi_msg); + fsg_opts->no_configfs = true; status = fsg_common_run_thread(fsg_opts->common); if (status) ---- >8 ---------------------------------------------------------------- This is what other legacy gadgets do after all. For configfs, why not simply check if the thread has already been started: ---- >8 ---------------------------------------------------------------- diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index acf210f..62854b6 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2984,8 +2984,10 @@ int fsg_common_run_thread(struct fsg_common *common) 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 PTR_ERR(common->thread_task); + return ret; } DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task)); @@ -3005,6 +3007,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 +3053,11 @@ 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) - return ret; + if (common->thread_task) { + ret = fsg_common_run_thread(fsg->common); + if (ret) + return ret; + } } fsg->gadget = gadget;
On Thu, Mar 31 2016, Alan Stern wrote: > Michal, I'm not sure how you intended to handle this. For legacy/nokia setting no_configfs should be a valid solution: ---- >8 ---------------------------------------------------------------- ---- >8 ---------------------------------------------------------------- With that, the whole fsg_common_run_thread call should be moved outside of the if (!no_configfs) and legacy should no longer call it IMO.