Re: USB gadgets with configfs hang reboot

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

 



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@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