* [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
@ 2024-03-14 6:59 yuan linyu
2024-03-14 7:53 ` Oliver Neukum
2024-03-14 15:02 ` Alan Stern
0 siblings, 2 replies; 5+ messages in thread
From: yuan linyu @ 2024-03-14 6:59 UTC (permalink / raw)
To: Alan Stern, Greg Kroah-Hartman; +Cc: linux-usb, yuan linyu, stable
It is possible trigger below warning message from mass storage function,
------------[ cut here ]------------
WARNING: CPU: 6 PID: 3839 at drivers/usb/gadget/udc/core.c:294 usb_ep_queue+0x7c/0x104
CPU: 6 PID: 3839 Comm: file-storage Tainted: G S WC O 6.1.25-android14-11-g354e2a7e7cd9 #1
pstate: 22400005 (nzCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
pc : usb_ep_queue+0x7c/0x104
lr : fsg_main_thread+0x494/0x1b3c
Root cause is mass storage function try to queue request from main thread,
but other thread may already disable ep when function disable.
As mass storage function have record of ep enable/disable state, let's
add the state check before queue request to UDC, it maybe avoid warning.
Also use common lock to protect ep state which avoid race between main
thread and function disable.
Cc: <stable@vger.kernel.org> # 6.1
Signed-off-by: yuan linyu <yuanlinyu@hihonor.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index c265a1f62fc1..056083cb68cb 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -520,12 +520,25 @@ static int fsg_setup(struct usb_function *f,
static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
struct usb_request *req)
{
+ unsigned long flags;
int rc;
- if (ep == fsg->bulk_in)
+ spin_lock_irqsave(&fsg->common->lock, flags);
+ if (ep == fsg->bulk_in) {
+ if (!fsg->bulk_in_enabled) {
+ spin_unlock_irqrestore(&fsg->common->lock, flags);
+ return -ESHUTDOWN;
+ }
dump_msg(fsg, "bulk-in", req->buf, req->length);
+ } else {
+ if (!fsg->bulk_out_enabled) {
+ spin_unlock_irqrestore(&fsg->common->lock, flags);
+ return -ESHUTDOWN;
+ }
+ }
rc = usb_ep_queue(ep, req, GFP_KERNEL);
+ spin_unlock_irqrestore(&fsg->common->lock, flags);
if (rc) {
/* We can't do much more than wait for a reset */
@@ -2406,8 +2419,10 @@ static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
static void fsg_disable(struct usb_function *f)
{
struct fsg_dev *fsg = fsg_from_func(f);
+ unsigned long flags;
/* Disable the endpoints */
+ spin_lock_irqsave(&fsg->common->lock, flags);
if (fsg->bulk_in_enabled) {
usb_ep_disable(fsg->bulk_in);
fsg->bulk_in_enabled = 0;
@@ -2416,6 +2431,7 @@ static void fsg_disable(struct usb_function *f)
usb_ep_disable(fsg->bulk_out);
fsg->bulk_out_enabled = 0;
}
+ spin_unlock_irqrestore(&fsg->common->lock, flags);
__raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
2024-03-14 6:59 [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep yuan linyu
@ 2024-03-14 7:53 ` Oliver Neukum
2024-03-14 8:14 ` yuanlinyu
2024-03-14 15:02 ` Alan Stern
1 sibling, 1 reply; 5+ messages in thread
From: Oliver Neukum @ 2024-03-14 7:53 UTC (permalink / raw)
To: yuan linyu, Alan Stern, Greg Kroah-Hartman; +Cc: linux-usb, stable
Hi,
I am sorry, but this contains a major issue.
On 14.03.24 07:59, yuan linyu wrote:
> It is possible trigger below warning message from mass storage function,
>
> ------------[ cut here ]------------
> WARNING: CPU: 6 PID: 3839 at drivers/usb/gadget/udc/core.c:294 usb_ep_queue+0x7c/0x104
> CPU: 6 PID: 3839 Comm: file-storage Tainted: G S WC O 6.1.25-android14-11-g354e2a7e7cd9 #1
> pstate: 22400005 (nzCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
> pc : usb_ep_queue+0x7c/0x104
> lr : fsg_main_thread+0x494/0x1b3c
>
> Root cause is mass storage function try to queue request from main thread,
> but other thread may already disable ep when function disable.
>
> As mass storage function have record of ep enable/disable state, let's
> add the state check before queue request to UDC, it maybe avoid warning.
>
> Also use common lock to protect ep state which avoid race between main
> thread and function disable.
>
> Cc: <stable@vger.kernel.org> # 6.1
> Signed-off-by: yuan linyu <yuanlinyu@hihonor.com>
Nacked-by: Oliver Neukum <oneukum@suse.com>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index c265a1f62fc1..056083cb68cb 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -520,12 +520,25 @@ static int fsg_setup(struct usb_function *f,
> static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
> struct usb_request *req)
> {
> + unsigned long flags;
> int rc;
>
> - if (ep == fsg->bulk_in)
> + spin_lock_irqsave(&fsg->common->lock, flags);
Taking a spinlock.
> + if (ep == fsg->bulk_in) {
> + if (!fsg->bulk_in_enabled) {
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> + return -ESHUTDOWN;
> + }
> dump_msg(fsg, "bulk-in", req->buf, req->length);
> + } else {
> + if (!fsg->bulk_out_enabled) {
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> + return -ESHUTDOWN;
> + }
> + }
>
> rc = usb_ep_queue(ep, req, GFP_KERNEL);
This can sleep.
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
Giving up the lock.
Sorry, now for the longer explanation. You'd introduce a deadlock.
You just cannot sleep with a spinlock held. It seems to me that
if you want to do this cleanly, you need to revisit the locking
to use locks you can sleep under.
HTH
Oliver
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
2024-03-14 7:53 ` Oliver Neukum
@ 2024-03-14 8:14 ` yuanlinyu
2024-03-14 9:20 ` yuanlinyu
0 siblings, 1 reply; 5+ messages in thread
From: yuanlinyu @ 2024-03-14 8:14 UTC (permalink / raw)
To: Oliver Neukum, Alan Stern, Greg Kroah-Hartman
Cc: linux-usb@vger.kernel.org, stable@vger.kernel.org
> From: Oliver Neukum <oneukum@suse.com>
> Sent: Thursday, March 14, 2024 3:54 PM
> To: yuanlinyu <yuanlinyu@hihonor.com>; Alan Stern
> <stern@rowland.harvard.edu>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
>
> Hi,
>
> I am sorry, but this contains a major issue.
>
> On 14.03.24 07:59, yuan linyu wrote:
> > It is possible trigger below warning message from mass storage function,
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 6 PID: 3839 at drivers/usb/gadget/udc/core.c:294
> usb_ep_queue+0x7c/0x104
> > CPU: 6 PID: 3839 Comm: file-storage Tainted: G S WC O
> 6.1.25-android14-11-g354e2a7e7cd9 #1
> > pstate: 22400005 (nzCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
> > pc : usb_ep_queue+0x7c/0x104
> > lr : fsg_main_thread+0x494/0x1b3c
> >
> > Root cause is mass storage function try to queue request from main thread,
> > but other thread may already disable ep when function disable.
> >
> > As mass storage function have record of ep enable/disable state, let's
> > add the state check before queue request to UDC, it maybe avoid warning.
> >
> > Also use common lock to protect ep state which avoid race between main
> > thread and function disable.
> >
> > Cc: <stable@vger.kernel.org> # 6.1
> > Signed-off-by: yuan linyu <yuanlinyu@hihonor.com>
> Nacked-by: Oliver Neukum <oneukum@suse.com>
>
> > ---
> > drivers/usb/gadget/function/f_mass_storage.c | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> b/drivers/usb/gadget/function/f_mass_storage.c
> > index c265a1f62fc1..056083cb68cb 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > @@ -520,12 +520,25 @@ static int fsg_setup(struct usb_function *f,
> > static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
> > struct usb_request *req)
> > {
> > + unsigned long flags;
> > int rc;
> >
> > - if (ep == fsg->bulk_in)
> > + spin_lock_irqsave(&fsg->common->lock, flags);
>
> Taking a spinlock.
>
> > + if (ep == fsg->bulk_in) {
> > + if (!fsg->bulk_in_enabled) {
> > + spin_unlock_irqrestore(&fsg->common->lock, flags);
> > + return -ESHUTDOWN;
> > + }
> > dump_msg(fsg, "bulk-in", req->buf, req->length);
> > + } else {
> > + if (!fsg->bulk_out_enabled) {
> > + spin_unlock_irqrestore(&fsg->common->lock, flags);
> > + return -ESHUTDOWN;
> > + }
> > + }
> >
> > rc = usb_ep_queue(ep, req, GFP_KERNEL);
>
> This can sleep.
>
> > + spin_unlock_irqrestore(&fsg->common->lock, flags);
>
> Giving up the lock.
>
>
> Sorry, now for the longer explanation. You'd introduce a deadlock.
> You just cannot sleep with a spinlock held. It seems to me that
I didn't review usb_ep_queue() clearly, in my test, I didn't hit sleep.
But the concern is good, will find better way to avoid it.
> if you want to do this cleanly, you need to revisit the locking
> to use locks you can sleep under.
>
> HTH
> Oliver
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
2024-03-14 8:14 ` yuanlinyu
@ 2024-03-14 9:20 ` yuanlinyu
0 siblings, 0 replies; 5+ messages in thread
From: yuanlinyu @ 2024-03-14 9:20 UTC (permalink / raw)
To: Oliver Neukum, Alan Stern, Greg Kroah-Hartman
Cc: linux-usb@vger.kernel.org, stable@vger.kernel.org
> From: yuanlinyu
> Sent: Thursday, March 14, 2024 4:14 PM
> To: 'Oliver Neukum' <oneukum@suse.com>; Alan Stern
> <stern@rowland.harvard.edu>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org; stable@vger.kernel.org
> Subject: RE: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
>
> > From: Oliver Neukum <oneukum@suse.com>
> > Sent: Thursday, March 14, 2024 3:54 PM
> >
> >
> > Sorry, now for the longer explanation. You'd introduce a deadlock.
> > You just cannot sleep with a spinlock held. It seems to me that
>
> I didn't review usb_ep_queue() clearly, in my test, I didn't hit sleep.
> But the concern is good, will find better way to avoid it.
Oliver, could you share one example which can sleep ?
I check several UDC drivers, like dwc3, cdnsp, cdns3,
Both disable/queue function have spinlock, this means no sleep, right ?
>
> > if you want to do this cleanly, you need to revisit the locking
> > to use locks you can sleep under.
> >
> > HTH
> > Oliver
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
2024-03-14 6:59 [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep yuan linyu
2024-03-14 7:53 ` Oliver Neukum
@ 2024-03-14 15:02 ` Alan Stern
1 sibling, 0 replies; 5+ messages in thread
From: Alan Stern @ 2024-03-14 15:02 UTC (permalink / raw)
To: yuan linyu; +Cc: Greg Kroah-Hartman, linux-usb, stable
On Thu, Mar 14, 2024 at 02:59:49PM +0800, yuan linyu wrote:
> It is possible trigger below warning message from mass storage function,
>
> ------------[ cut here ]------------
> WARNING: CPU: 6 PID: 3839 at drivers/usb/gadget/udc/core.c:294 usb_ep_queue+0x7c/0x104
> CPU: 6 PID: 3839 Comm: file-storage Tainted: G S WC O 6.1.25-android14-11-g354e2a7e7cd9 #1
> pstate: 22400005 (nzCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
> pc : usb_ep_queue+0x7c/0x104
> lr : fsg_main_thread+0x494/0x1b3c
>
> Root cause is mass storage function try to queue request from main thread,
> but other thread may already disable ep when function disable.
>
> As mass storage function have record of ep enable/disable state, let's
> add the state check before queue request to UDC, it maybe avoid warning.
>
> Also use common lock to protect ep state which avoid race between main
> thread and function disable.
This seems like a lot of effort just to avoid a harmless WARNING. How
about instead changing the WARNING to a debug-level message?
Alan Stern
> Cc: <stable@vger.kernel.org> # 6.1
> Signed-off-by: yuan linyu <yuanlinyu@hihonor.com>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index c265a1f62fc1..056083cb68cb 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -520,12 +520,25 @@ static int fsg_setup(struct usb_function *f,
> static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
> struct usb_request *req)
> {
> + unsigned long flags;
> int rc;
>
> - if (ep == fsg->bulk_in)
> + spin_lock_irqsave(&fsg->common->lock, flags);
> + if (ep == fsg->bulk_in) {
> + if (!fsg->bulk_in_enabled) {
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> + return -ESHUTDOWN;
> + }
> dump_msg(fsg, "bulk-in", req->buf, req->length);
> + } else {
> + if (!fsg->bulk_out_enabled) {
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> + return -ESHUTDOWN;
> + }
> + }
>
> rc = usb_ep_queue(ep, req, GFP_KERNEL);
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
> if (rc) {
>
> /* We can't do much more than wait for a reset */
> @@ -2406,8 +2419,10 @@ static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> static void fsg_disable(struct usb_function *f)
> {
> struct fsg_dev *fsg = fsg_from_func(f);
> + unsigned long flags;
>
> /* Disable the endpoints */
> + spin_lock_irqsave(&fsg->common->lock, flags);
> if (fsg->bulk_in_enabled) {
> usb_ep_disable(fsg->bulk_in);
> fsg->bulk_in_enabled = 0;
> @@ -2416,6 +2431,7 @@ static void fsg_disable(struct usb_function *f)
> usb_ep_disable(fsg->bulk_out);
> fsg->bulk_out_enabled = 0;
> }
> + spin_unlock_irqrestore(&fsg->common->lock, flags);
>
> __raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE, NULL);
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-14 15:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-14 6:59 [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep yuan linyu
2024-03-14 7:53 ` Oliver Neukum
2024-03-14 8:14 ` yuanlinyu
2024-03-14 9:20 ` yuanlinyu
2024-03-14 15:02 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).