When binding the function to usb_configuration, check whether the thread
is running before starting another one. Without that, when function
instance is added to multiple configurations, fsg_bing starts multiple
threads with all but the latest one being forgotten by the driver. This
leads to obvious thread leaks, possible lockups when trying to halt the
machine and possible more issues.
This fixes issues with legacy/multi¹ gadget as well as configfs gadgets
when mass_storage function is added to multiple configurations.
This change also simplifies API since the legacy gadgets no longer need
to worry about starting the thread by themselves (which was where bug
in legacy/multi was in the first place).
¹ I have no example failure though. Conclusion that legacy/multi has
a bug is based purely on me reading the code.
Signed-off-by: Michal Nazarewicz <[email protected]>
---
drivers/usb/gadget/function/f_mass_storage.c | 36 ++++++++++++----------------
drivers/usb/gadget/function/f_mass_storage.h | 2 --
drivers/usb/gadget/legacy/acm_ms.c | 4 ----
drivers/usb/gadget/legacy/mass_storage.c | 4 ----
drivers/usb/gadget/legacy/multi.c | 12 ----------
drivers/usb/gadget/legacy/nokia.c | 7 ------
6 files changed, 15 insertions(+), 50 deletions(-)
This has been only compile tested so it would be great if someone
could see if this fixes the configfs issue.
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index acf210f..5c6d4d7 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2977,25 +2977,6 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
}
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;
-}
-EXPORT_SYMBOL_GPL(fsg_common_run_thread);
-
static void fsg_common_release(struct kref *ref)
{
struct fsg_common *common = container_of(ref, struct fsg_common, ref);
@@ -3005,6 +2986,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 +3032,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;
diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
index 445df67..b6a9918 100644
--- a/drivers/usb/gadget/function/f_mass_storage.h
+++ b/drivers/usb/gadget/function/f_mass_storage.h
@@ -153,8 +153,6 @@ int fsg_common_create_luns(struct fsg_common *common, struct fsg_config *cfg);
void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn,
const char *pn);
-int fsg_common_run_thread(struct fsg_common *common);
-
void fsg_config_from_params(struct fsg_config *cfg,
const struct fsg_module_parameters *params,
unsigned int fsg_num_buffers);
diff --git a/drivers/usb/gadget/legacy/acm_ms.c b/drivers/usb/gadget/legacy/acm_ms.c
index c16089e..c39de65 100644
--- a/drivers/usb/gadget/legacy/acm_ms.c
+++ b/drivers/usb/gadget/legacy/acm_ms.c
@@ -133,10 +133,6 @@ static int acm_ms_do_config(struct usb_configuration *c)
if (status < 0)
goto put_msg;
- status = fsg_common_run_thread(opts->common);
- if (status)
- goto remove_acm;
-
status = usb_add_function(c, f_msg);
if (status)
goto remove_acm;
diff --git a/drivers/usb/gadget/legacy/mass_storage.c b/drivers/usb/gadget/legacy/mass_storage.c
index e61af53..125974f 100644
--- a/drivers/usb/gadget/legacy/mass_storage.c
+++ b/drivers/usb/gadget/legacy/mass_storage.c
@@ -132,10 +132,6 @@ static int msg_do_config(struct usb_configuration *c)
if (IS_ERR(f_msg))
return PTR_ERR(f_msg);
- ret = fsg_common_run_thread(opts->common);
- if (ret)
- goto put_func;
-
ret = usb_add_function(c, f_msg);
if (ret)
goto put_func;
diff --git a/drivers/usb/gadget/legacy/multi.c b/drivers/usb/gadget/legacy/multi.c
index 229d704..a70a406 100644
--- a/drivers/usb/gadget/legacy/multi.c
+++ b/drivers/usb/gadget/legacy/multi.c
@@ -137,7 +137,6 @@ static struct usb_function *f_msg_rndis;
static int rndis_do_config(struct usb_configuration *c)
{
- struct fsg_opts *fsg_opts;
int ret;
if (gadget_is_otg(c->cdev->gadget)) {
@@ -169,11 +168,6 @@ static int rndis_do_config(struct usb_configuration *c)
goto err_fsg;
}
- fsg_opts = fsg_opts_from_func_inst(fi_msg);
- ret = fsg_common_run_thread(fsg_opts->common);
- if (ret)
- goto err_run;
-
ret = usb_add_function(c, f_msg_rndis);
if (ret)
goto err_run;
@@ -225,7 +219,6 @@ static struct usb_function *f_msg_multi;
static int cdc_do_config(struct usb_configuration *c)
{
- struct fsg_opts *fsg_opts;
int ret;
if (gadget_is_otg(c->cdev->gadget)) {
@@ -258,11 +251,6 @@ static int cdc_do_config(struct usb_configuration *c)
goto err_fsg;
}
- fsg_opts = fsg_opts_from_func_inst(fi_msg);
- ret = fsg_common_run_thread(fsg_opts->common);
- if (ret)
- goto err_run;
-
ret = usb_add_function(c, f_msg_multi);
if (ret)
goto err_run;
diff --git a/drivers/usb/gadget/legacy/nokia.c b/drivers/usb/gadget/legacy/nokia.c
index 0997504..b1e535f 100644
--- a/drivers/usb/gadget/legacy/nokia.c
+++ b/drivers/usb/gadget/legacy/nokia.c
@@ -152,7 +152,6 @@ static int nokia_bind_config(struct usb_configuration *c)
struct usb_function *f_ecm;
struct usb_function *f_obex2 = NULL;
struct usb_function *f_msg;
- struct fsg_opts *fsg_opts;
int status = 0;
int obex1_stat = -1;
int obex2_stat = -1;
@@ -222,12 +221,6 @@ static int nokia_bind_config(struct usb_configuration *c)
goto err_ecm;
}
- fsg_opts = fsg_opts_from_func_inst(fi_msg);
-
- status = fsg_common_run_thread(fsg_opts->common);
- if (status)
- goto err_msg;
-
status = usb_add_function(c, f_msg);
if (status)
goto err_msg;
--
2.8.0.rc3.226.g39d4020
On Tue, 5 Apr 2016, Michal Nazarewicz wrote:
> When binding the function to usb_configuration, check whether the thread
> is running before starting another one. Without that, when function
> instance is added to multiple configurations, fsg_bing starts multiple
> threads with all but the latest one being forgotten by the driver. This
> leads to obvious thread leaks, possible lockups when trying to halt the
> machine and possible more issues.
>
> This fixes issues with legacy/multi¹ gadget as well as configfs gadgets
> when mass_storage function is added to multiple configurations.
>
> This change also simplifies API since the legacy gadgets no longer need
> to worry about starting the thread by themselves (which was where bug
> in legacy/multi was in the first place).
>
> ¹ I have no example failure though. Conclusion that legacy/multi has
> a bug is based purely on me reading the code.
>
> Signed-off-by: Michal Nazarewicz <[email protected]>
This doesn't address the problem I raised in a previous email.
Sharing one thread among several function instances in the same config
will not work if one of them encounters an error.
Alan Stern
> On Tue, 5 Apr 2016, Michal Nazarewicz wrote:
>> When binding the function to usb_configuration, check whether the thread
>> is running before starting another one. Without that, when function
>> instance is added to multiple configurations, fsg_bing starts multiple
>> threads with all but the latest one being forgotten by the driver. This
>> leads to obvious thread leaks, possible lockups when trying to halt the
>> machine and possible more issues.
>>
>> This fixes issues with legacy/multi¹ gadget as well as configfs gadgets
>> when mass_storage function is added to multiple configurations.
>>
>> This change also simplifies API since the legacy gadgets no longer need
>> to worry about starting the thread by themselves (which was where bug
>> in legacy/multi was in the first place).
>>
>> ¹ I have no example failure though. Conclusion that legacy/multi has
>> a bug is based purely on me reading the code.
>>
>> Signed-off-by: Michal Nazarewicz <[email protected]>
On Tue, Apr 05 2016, Alan Stern wrote:
> This doesn't address the problem I raised in a previous email.
> Sharing one thread among several function instances in the same config
> will not work if one of them encounters an error.
Each usb_function_instance has its own fsg_common and its own thread.
This was true in the past and is true with this patch as well.
And unless I’m missing something, sharing a thread among multiple
usb_function’s does not prevent the driver from working correctly.
Having the thread run even when it’s not used may be considered wasteful
but that’s an orthogonal issue to the configfs failure.
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
On Tue, 5 Apr 2016, Michal Nazarewicz wrote:
> > On Tue, 5 Apr 2016, Michal Nazarewicz wrote:
> >> When binding the function to usb_configuration, check whether the thread
> >> is running before starting another one. Without that, when function
> >> instance is added to multiple configurations, fsg_bing starts multiple
> >> threads with all but the latest one being forgotten by the driver. This
> >> leads to obvious thread leaks, possible lockups when trying to halt the
> >> machine and possible more issues.
> >>
> >> This fixes issues with legacy/multi¹ gadget as well as configfs gadgets
> >> when mass_storage function is added to multiple configurations.
> >>
> >> This change also simplifies API since the legacy gadgets no longer need
> >> to worry about starting the thread by themselves (which was where bug
> >> in legacy/multi was in the first place).
> >>
> >> ¹ I have no example failure though. Conclusion that legacy/multi has
> >> a bug is based purely on me reading the code.
> >>
> >> Signed-off-by: Michal Nazarewicz <[email protected]>
>
> On Tue, Apr 05 2016, Alan Stern wrote:
> > This doesn't address the problem I raised in a previous email.
> > Sharing one thread among several function instances in the same config
> > will not work if one of them encounters an error.
>
> Each usb_function_instance has its own fsg_common and its own thread.
> This was true in the past and is true with this patch as well.
We're getting confused by the stupid terminology again. Yes, each
usb_function_instance has its own fsg_common and its own thread. But
multiple usb_function structures (each one being a separate function
instance) can belong to the same usb_function_instance and they will
all share the same fsg_common.
That's what happened with the nokia driver. It creates one
usb_function_instance with two usb_function structures. In this case
they are in different configs, so sharing a thread doesn't matter. But
it would matter if they were in the same config.
> And unless I’m missing something, sharing a thread among multiple
> usb_function’s does not prevent the driver from working correctly.
Suppose one usb_function is carrying out an I/O operation while another
one in the same config gets a Set-Interface request from the host.
The request causes the driver to raise an FSG_STATE_CONFIG_CHANGE
exception. When the thread sees this exception, it will abort the I/O
that it is carrying out for the first usb_function.
In other words, exceptions raised by one instance will affect the
shared thread even when it's doing work for a different instance.
> Having the thread run even when it’s not used may be considered wasteful
> but that’s an orthogonal issue to the configfs failure.
Yes. Having extra unused threads isn't terrible.
Alan Stern
On Tue, Apr 05 2016, Alan Stern wrote:
> Suppose one usb_function is carrying out an I/O operation while
> another one in the same config gets a Set-Interface request from the
> host.
That cannot happen. A single instance of mass_storage cannot¹ be added
twice to the same configuration.
¹ To be more precise, not via configfs. A legacy gadget could do that,
but that would be a bug in that legacy driver, not f_mass_storage.
Moreover, no current legacy gadgets do that though so IMO this is an
academic discussion.
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
On Wed, 6 Apr 2016, Michal Nazarewicz wrote:
> On Tue, Apr 05 2016, Alan Stern wrote:
> > Suppose one usb_function is carrying out an I/O operation while
> > another one in the same config gets a Set-Interface request from the
> > host.
>
> That cannot happen. A single instance of mass_storage cannot¹ be added
> twice to the same configuration.
>
> ¹ To be more precise, not via configfs. A legacy gadget could do that,
> but that would be a bug in that legacy driver, not f_mass_storage.
> Moreover, no current legacy gadgets do that though so IMO this is an
> academic discussion.
Okay. Then I suggest adding this explanation to the patch description.
BTW, is configfs capable of adding a single instance twice in different
configs? Or is that again something only legacy gadgets can do?
Alan Stern
>> On Tue, Apr 05 2016, Alan Stern wrote:
>>> Suppose one usb_function is carrying out an I/O operation while
>>> another one in the same config gets a Set-Interface request from the
>>> host.
> On Wed, 6 Apr 2016, Michal Nazarewicz wrote:
>> That cannot happen. A single instance of mass_storage cannot¹ be added
>> twice to the same configuration.
>>
>> ¹ To be more precise, not via configfs. A legacy gadget could do that,
>> but that would be a bug in that legacy driver, not f_mass_storage.
>> Moreover, no current legacy gadgets do that though so IMO this is an
>> academic discussion.
On Wed, Apr 06 2016, Alan Stern wrote:
> Okay. Then I suggest adding this explanation to the patch
> description.
Sounds good to me. I’d love someone to test this patch (I sadly have no
way of doing that at the moment) and with that I can resend it with
updated message.
> BTW, is configfs capable of adding a single instance twice in different
> configs? Or is that again something only legacy gadgets can do?
I don’t think so. I might be wrong though, but here’s configuration
from the original post:
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
This makes me suspect it’s not possible to link a function instance to
the same configuration twice, but now that I think about it, I’m not
quite sure what would happen if one did:
ln -s functions/mass_storage.0 configs/c.1/foo
ln -s functions/mass_storage.0 configs/c.1/bar
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
On Thu, 7 Apr 2016, Michal Nazarewicz wrote:
> Sounds good to me. I’d love someone to test this patch (I sadly have no
> way of doing that at the moment) and with that I can resend it with
> updated message.
Ivaylo should be able to try it.
> > BTW, is configfs capable of adding a single instance twice in different
> > configs? Or is that again something only legacy gadgets can do?
>
> I don’t think so. I might be wrong though, but here’s configuration
> from the original post:
>
> 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
>
> This makes me suspect it’s not possible to link a function instance to
> the same configuration twice, but now that I think about it, I’m not
> quite sure what would happen if one did:
>
> ln -s functions/mass_storage.0 configs/c.1/foo
> ln -s functions/mass_storage.0 configs/c.1/bar
Do you think it would be worthwhile to check for this possibility in
the driver and report an error?
Alan Stern
Hi,
On 7.04.2016 17:25, Alan Stern wrote:
> On Thu, 7 Apr 2016, Michal Nazarewicz wrote:
>
>> Sounds good to me. I’d love someone to test this patch (I sadly have no
>> way of doing that at the moment) and with that I can resend it with
>> updated message.
>
> Ivaylo should be able to try it.
>
I applied the patch agains 4.6-rc1 and while it seems there is no more
oops, trying to "cd /sys/bus/platform/drivers/musb-hdrc && echo
musb-hdrc.0.auto > unbind" still results in console hang. Will try to
find time later today and put printks all over the place to see what's
going on.
Ivo
> On Thu, 7 Apr 2016, Michal Nazarewicz wrote:
>> This makes me suspect it’s not possible to link a function instance to
>> the same configuration twice, but now that I think about it, I’m not
>> quite sure what would happen if one did:
>>
>> ln -s functions/mass_storage.0 configs/c.1/foo
>> ln -s functions/mass_storage.0 configs/c.1/bar
On Thu, Apr 07 2016, Alan Stern wrote:
> Do you think it would be worthwhile to check for this possibility in
> the driver and report an error?
I think this should be (if it isn’t already) blocked on configfs side.
I cannot see a legitimate use of such configuration and I wouldn’t be
surprised if other function drivers broke as well.
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
Hi,
On 7.04.2016 17:46, Ivaylo Dimitrov wrote:
>
> Hi,
>
> On 7.04.2016 17:25, Alan Stern wrote:
>> On Thu, 7 Apr 2016, Michal Nazarewicz wrote:
>>
>>> Sounds good to me. I’d love someone to test this patch (I sadly have no
>>> way of doing that at the moment) and with that I can resend it with
>>> updated message.
>>
>> Ivaylo should be able to try it.
>>
>
> I applied the patch agains 4.6-rc1 and while it seems there is no more
> oops, trying to "cd /sys/bus/platform/drivers/musb-hdrc && echo
> musb-hdrc.0.auto > unbind" still results in console hang. Will try to
> find time later today and put printks all over the place to see what's
> going on.
>
The $subject patch fixes the oops in fsg, so you may add:
Tested-by: Ivaylo Dimitrov <[email protected]>
However, there is another problem, this time with f_acm - if there is an
open /dev/ttyGSn device, it is impossible to reboot/power down the device.
My investigation shows so far that there is a process(pnatd) that opens
/dev/ttyGSn devices, so gserial_free_port() hangs on
wait_event(port->close_wait, gs_closed(port)); if I do "cd
/sys/bus/platform/drivers/musb-hdrc && echo musb-hdrc.0.auto > unbind".
Unfortunately I don't have serial port connector on my N900, so I can't
capture logs after the reboot command, however, I suspect it hangs on
the same place as with unbind.
That looks weird, as one would expect that close() is called when the
kernel kills user processes on reboot/powerdown.
Ivo
Hi Michał,
W dniu 07.04.2016 o 18:40, Michal Nazarewicz pisze:
>> On Thu, 7 Apr 2016, Michal Nazarewicz wrote:
>>> This makes me suspect it’s not possible to link a function instance to
>>> the same configuration twice, but now that I think about it, I’m not
>>> quite sure what would happen if one did:
>>>
>>> ln -s functions/mass_storage.0 configs/c.1/foo
>>> ln -s functions/mass_storage.0 configs/c.1/bar
>
> On Thu, Apr 07 2016, Alan Stern wrote:
>> Do you think it would be worthwhile to check for this possibility in
>> the driver and report an error?
>
> I think this should be (if it isn’t already) blocked on configfs side.
> I cannot see a legitimate use of such configuration and I wouldn’t be
> surprised if other function drivers broke as well.
>
Sorry about late response.
The function responsible for verifying if a symlink can be made is
in drivers/usb/gadget/configfs.c: config_usb_cfg_link()
There is a comment from the author:
/*
* Make sure this function is from within our _this_ gadget and not
* from another gadget or a random directory.
* Also a function instance can only be linked once.
*/
This is the code fragment of interest:
list_for_each_entry(f, &cfg->func_list, list) {
if (f->fi == fi) {
ret = -EEXIST;
goto out;
}
}
AP
On Mon, Apr 18 2016, Andrzej Pietrasiewicz wrote:
> The function responsible for verifying if a symlink can be made is in
> drivers/usb/gadget/configfs.c: config_usb_cfg_link()
>
> There is a comment from the author:
>
> * Also a function instance can only be linked once.
>
> This is the code fragment of interest:
>
> list_for_each_entry(f, &cfg->func_list, list) {
> if (f->fi == fi) {
> ret = -EEXIST;
> goto out;
> }
> }
Thanks for checking. This is exactly the kind of check I expected so
checking for thread not being run multiple times is enough when
configfs is in use.
Legacy gadgets may still construct crazy configurations but I’d call
that a bug in such legacy gadget.
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»