* [PATCH 4.19.y 1/3] usb: dwc3: gadget: add dwc3_request status tracking
2019-07-29 13:43 [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Saranya Gopal
@ 2019-07-29 13:43 ` Saranya Gopal
2019-07-29 13:43 ` [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice Saranya Gopal
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Saranya Gopal @ 2019-07-29 13:43 UTC (permalink / raw)
To: stable; +Cc: linux-usb, fei.yang, john.stultz, Felipe Balbi, Saranya Gopal
From: Felipe Balbi <felipe.balbi@linux.intel.com>
[Upstream commit a3af5e3ad3f11a0001317da9e9fb78b]
This patch starts tracking dwc3_request status. A following patch will
build on top of this to prevent a request from being queued twice.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
---
drivers/usb/dwc3/core.h | 9 +++++++++
drivers/usb/dwc3/gadget.c | 3 +++
drivers/usb/dwc3/gadget.h | 2 ++
3 files changed, 14 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 13102850..301bf94 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -843,6 +843,7 @@ struct dwc3_hwparams {
* @num_pending_sgs: counter to pending sgs
* @num_queued_sgs: counter to the number of sgs which already got queued
* @remaining: amount of data remaining
+ * @status: internal dwc3 request status tracking
* @epnum: endpoint number to which this request refers
* @trb: pointer to struct dwc3_trb
* @trb_dma: DMA address of @trb
@@ -863,6 +864,14 @@ struct dwc3_request {
unsigned num_pending_sgs;
unsigned int num_queued_sgs;
unsigned remaining;
+
+ unsigned int status;
+#define DWC3_REQUEST_STATUS_QUEUED 0
+#define DWC3_REQUEST_STATUS_STARTED 1
+#define DWC3_REQUEST_STATUS_CANCELLED 2
+#define DWC3_REQUEST_STATUS_COMPLETED 3
+#define DWC3_REQUEST_STATUS_UNKNOWN -1
+
u8 epnum;
struct dwc3_trb *trb;
dma_addr_t trb_dma;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e7461c9..3f337a0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -209,6 +209,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
struct dwc3 *dwc = dep->dwc;
dwc3_gadget_del_and_unmap_request(dep, req, status);
+ req->status = DWC3_REQUEST_STATUS_COMPLETED;
spin_unlock(&dwc->lock);
usb_gadget_giveback_request(&dep->endpoint, &req->request);
@@ -838,6 +839,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request(struct usb_ep *ep,
req->direction = dep->direction;
req->epnum = dep->number;
req->dep = dep;
+ req->status = DWC3_REQUEST_STATUS_UNKNOWN;
trace_dwc3_alloc_request(req);
@@ -1297,6 +1299,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
trace_dwc3_ep_queue(req);
list_add_tail(&req->list, &dep->pending_list);
+ req->status = DWC3_REQUEST_STATUS_QUEUED;
/*
* NOTICE: Isochronous endpoints should NEVER be prestarted. We must
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 023a473..6aebe8c 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -76,6 +76,7 @@ static inline void dwc3_gadget_move_started_request(struct dwc3_request *req)
struct dwc3_ep *dep = req->dep;
req->started = true;
+ req->status = DWC3_REQUEST_STATUS_STARTED;
list_move_tail(&req->list, &dep->started_list);
}
@@ -91,6 +92,7 @@ static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request *req)
struct dwc3_ep *dep = req->dep;
req->started = false;
+ req->status = DWC3_REQUEST_STATUS_CANCELLED;
list_move_tail(&req->list, &dep->cancelled_list);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice
2019-07-29 13:43 [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Saranya Gopal
2019-07-29 13:43 ` [PATCH 4.19.y 1/3] usb: dwc3: gadget: add dwc3_request status tracking Saranya Gopal
@ 2019-07-29 13:43 ` Saranya Gopal
2019-07-29 16:56 ` Greg KH
2019-07-29 13:43 ` [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag Saranya Gopal
2019-07-29 17:34 ` [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Greg KH
3 siblings, 1 reply; 11+ messages in thread
From: Saranya Gopal @ 2019-07-29 13:43 UTC (permalink / raw)
To: stable; +Cc: linux-usb, fei.yang, john.stultz, Felipe Balbi, Saranya Gopal
From: Felipe Balbi <felipe.balbi@linux.intel.com>
[Upstream commit b2b6d601365a1acb90b87c85197d79]
Queueing the same request twice can introduce hard-to-debug
problems. At least one function driver - Android's f_mtp.c - is known
to cause this problem.
While that function is out-of-tree, this is a problem that's easy
enough to avoid.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
---
drivers/usb/dwc3/gadget.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3f337a0..a56a92a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
&req->request, req->dep->name))
return -EINVAL;
+ if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
+ "%s: request %pK already in flight\n",
+ dep->name, &req->request))
+ return -EINVAL;
+
pm_runtime_get(dwc->dev);
req->request.actual = 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice
2019-07-29 13:43 ` [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice Saranya Gopal
@ 2019-07-29 16:56 ` Greg KH
2019-07-29 17:06 ` Gopal, Saranya
0 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2019-07-29 16:56 UTC (permalink / raw)
To: Saranya Gopal; +Cc: stable, linux-usb, fei.yang, john.stultz, Felipe Balbi
On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote:
> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> [Upstream commit b2b6d601365a1acb90b87c85197d79]
>
> Queueing the same request twice can introduce hard-to-debug
> problems. At least one function driver - Android's f_mtp.c - is known
> to cause this problem.
>
> While that function is out-of-tree, this is a problem that's easy
> enough to avoid.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> ---
> drivers/usb/dwc3/gadget.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3f337a0..a56a92a 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> &req->request, req->dep->name))
> return -EINVAL;
>
> + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
> + "%s: request %pK already in flight\n",
> + dep->name, &req->request))
> + return -EINVAL;
So we are going to trip syzbot up on this out-of-tree driver? Brave...
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice
2019-07-29 16:56 ` Greg KH
@ 2019-07-29 17:06 ` Gopal, Saranya
2019-07-29 17:28 ` Greg KH
0 siblings, 1 reply; 11+ messages in thread
From: Gopal, Saranya @ 2019-07-29 17:06 UTC (permalink / raw)
To: Greg KH
Cc: stable@vger.kernel.org, linux-usb@vger.kernel.org, Yang, Fei,
john.stultz@linaro.org, Felipe Balbi
> On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote:
> > From: Felipe Balbi <felipe.balbi@linux.intel.com>
> >
> > [Upstream commit b2b6d601365a1acb90b87c85197d79]
> >
> > Queueing the same request twice can introduce hard-to-debug
> > problems. At least one function driver - Android's f_mtp.c - is known
> > to cause this problem.
> >
> > While that function is out-of-tree, this is a problem that's easy
> > enough to avoid.
> >
> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> > ---
> > drivers/usb/dwc3/gadget.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 3f337a0..a56a92a 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct
> dwc3_ep *dep, struct dwc3_request *req)
> > &req->request, req->dep->name))
> > return -EINVAL;
> >
> > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
> > + "%s: request %pK already in flight\n",
> > + dep->name, &req->request))
> > + return -EINVAL;
>
> So we are going to trip syzbot up on this out-of-tree driver? Brave...
I had retained the commit message from the upstream commit.
However, without this patch, I see issues with adb as well.
Adb will hang after adb root/unroot command and needs a reboot to recover.
Thanks,
Saranya
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice
2019-07-29 17:06 ` Gopal, Saranya
@ 2019-07-29 17:28 ` Greg KH
0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2019-07-29 17:28 UTC (permalink / raw)
To: Gopal, Saranya
Cc: stable@vger.kernel.org, linux-usb@vger.kernel.org, Yang, Fei,
john.stultz@linaro.org, Felipe Balbi
On Mon, Jul 29, 2019 at 05:06:13PM +0000, Gopal, Saranya wrote:
> > On Mon, Jul 29, 2019 at 07:13:38PM +0530, Saranya Gopal wrote:
> > > From: Felipe Balbi <felipe.balbi@linux.intel.com>
> > >
> > > [Upstream commit b2b6d601365a1acb90b87c85197d79]
> > >
> > > Queueing the same request twice can introduce hard-to-debug
> > > problems. At least one function driver - Android's f_mtp.c - is known
> > > to cause this problem.
> > >
> > > While that function is out-of-tree, this is a problem that's easy
> > > enough to avoid.
> > >
> > > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> > > ---
> > > drivers/usb/dwc3/gadget.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index 3f337a0..a56a92a 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -1291,6 +1291,11 @@ static int __dwc3_gadget_ep_queue(struct
> > dwc3_ep *dep, struct dwc3_request *req)
> > > &req->request, req->dep->name))
> > > return -EINVAL;
> > >
> > > + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
> > > + "%s: request %pK already in flight\n",
> > > + dep->name, &req->request))
> > > + return -EINVAL;
> >
> > So we are going to trip syzbot up on this out-of-tree driver? Brave...
>
> I had retained the commit message from the upstream commit.
> However, without this patch, I see issues with adb as well.
> Adb will hang after adb root/unroot command and needs a reboot to recover.
So you see huge WARN dumps in the log?
That's fine, but be aware, if userspace can trigger this, then syzbot
will trigger it, and any system running 'panic on warn' will crash.
If this is something that we normally have to catch and handle, WARN()
is not how to do it. But we should fix that upstream, not here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag
2019-07-29 13:43 [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Saranya Gopal
2019-07-29 13:43 ` [PATCH 4.19.y 1/3] usb: dwc3: gadget: add dwc3_request status tracking Saranya Gopal
2019-07-29 13:43 ` [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice Saranya Gopal
@ 2019-07-29 13:43 ` Saranya Gopal
2019-07-29 16:56 ` Greg KH
2019-07-29 17:34 ` [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Greg KH
3 siblings, 1 reply; 11+ messages in thread
From: Saranya Gopal @ 2019-07-29 13:43 UTC (permalink / raw)
To: stable; +Cc: linux-usb, fei.yang, john.stultz, Felipe Balbi, Saranya Gopal
From: Felipe Balbi <felipe.balbi@linux.intel.com>
[Upstream commit 7c3d7dc89e57a1d43acea935882dd8713c9e639f]
Now that we have req->status, we don't need this extra flag
anymore. It's safe to remove it.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
---
drivers/usb/dwc3/core.h | 2 --
drivers/usb/dwc3/gadget.c | 1 -
drivers/usb/dwc3/gadget.h | 2 --
3 files changed, 5 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 301bf94..6ea3e48 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -852,7 +852,6 @@ struct dwc3_hwparams {
* or unaligned OUT)
* @direction: IN or OUT direction flag
* @mapped: true when request has been dma-mapped
- * @started: request is started
*/
struct dwc3_request {
struct usb_request request;
@@ -881,7 +880,6 @@ struct dwc3_request {
unsigned needs_extra_trb:1;
unsigned direction:1;
unsigned mapped:1;
- unsigned started:1;
};
/*
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a56a92a..f29068d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -174,7 +174,6 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
{
struct dwc3 *dwc = dep->dwc;
- req->started = false;
list_del(&req->list);
req->remaining = 0;
req->needs_extra_trb = false;
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 6aebe8c..3ed738e 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -75,7 +75,6 @@ static inline void dwc3_gadget_move_started_request(struct dwc3_request *req)
{
struct dwc3_ep *dep = req->dep;
- req->started = true;
req->status = DWC3_REQUEST_STATUS_STARTED;
list_move_tail(&req->list, &dep->started_list);
}
@@ -91,7 +90,6 @@ static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request *req)
{
struct dwc3_ep *dep = req->dep;
- req->started = false;
req->status = DWC3_REQUEST_STATUS_CANCELLED;
list_move_tail(&req->list, &dep->cancelled_list);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag
2019-07-29 13:43 ` [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag Saranya Gopal
@ 2019-07-29 16:56 ` Greg KH
2019-07-29 16:59 ` Gopal, Saranya
0 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2019-07-29 16:56 UTC (permalink / raw)
To: Saranya Gopal; +Cc: stable, linux-usb, fei.yang, john.stultz, Felipe Balbi
On Mon, Jul 29, 2019 at 07:13:39PM +0530, Saranya Gopal wrote:
> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> [Upstream commit 7c3d7dc89e57a1d43acea935882dd8713c9e639f]
>
> Now that we have req->status, we don't need this extra flag
> anymore. It's safe to remove it.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> ---
> drivers/usb/dwc3/core.h | 2 --
> drivers/usb/dwc3/gadget.c | 1 -
> drivers/usb/dwc3/gadget.h | 2 --
> 3 files changed, 5 deletions(-)
Why is this patch needed for a stable tree? It just cleans stuff up, it
doesn't actually change any functionality.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag
2019-07-29 16:56 ` Greg KH
@ 2019-07-29 16:59 ` Gopal, Saranya
0 siblings, 0 replies; 11+ messages in thread
From: Gopal, Saranya @ 2019-07-29 16:59 UTC (permalink / raw)
To: Greg KH
Cc: stable@vger.kernel.org, linux-usb@vger.kernel.org, Yang, Fei,
john.stultz@linaro.org, Felipe Balbi
> >
> > [Upstream commit 7c3d7dc89e57a1d43acea935882dd8713c9e639f]
> >
> > Now that we have req->status, we don't need this extra flag
> > anymore. It's safe to remove it.
> >
> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> > ---
> > drivers/usb/dwc3/core.h | 2 --
> > drivers/usb/dwc3/gadget.c | 1 -
> > drivers/usb/dwc3/gadget.h | 2 --
> > 3 files changed, 5 deletions(-)
>
> Why is this patch needed for a stable tree? It just cleans stuff up, it
> doesn't actually change any functionality.
Yes, this can be skipped.
Thanks,
Saranya
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice
2019-07-29 13:43 [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Saranya Gopal
` (2 preceding siblings ...)
2019-07-29 13:43 ` [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag Saranya Gopal
@ 2019-07-29 17:34 ` Greg KH
2019-07-30 9:29 ` Gopal, Saranya
3 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2019-07-29 17:34 UTC (permalink / raw)
To: Saranya Gopal; +Cc: stable, linux-usb, fei.yang, john.stultz
On Mon, Jul 29, 2019 at 07:13:36PM +0530, Saranya Gopal wrote:
> With recent changes in AOSP, adb is now using asynchronous I/O.
> While adb works good for the most part, there have been issues with
> adb root/unroot commands which cause adb hang. The issue is caused
> by a request being queued twice. A series of 3 patches from
> Felipe Balbi in upstream tree fixes this issue.
>
> Felipe Balbi (3):
> usb: dwc3: gadget: add dwc3_request status tracking
> usb: dwc3: gadget: prevent dwc3_request from being queued twice
> usb: dwc3: gadget: remove req->started flag
I would like to get an ack from Felipe before I take these.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice
2019-07-29 17:34 ` [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice Greg KH
@ 2019-07-30 9:29 ` Gopal, Saranya
0 siblings, 0 replies; 11+ messages in thread
From: Gopal, Saranya @ 2019-07-30 9:29 UTC (permalink / raw)
To: Greg KH
Cc: stable@vger.kernel.org, linux-usb@vger.kernel.org, Yang, Fei,
john.stultz@linaro.org
> On Mon, Jul 29, 2019 at 07:13:36PM +0530, Saranya Gopal wrote:
> > With recent changes in AOSP, adb is now using asynchronous I/O.
> > While adb works good for the most part, there have been issues with
> > adb root/unroot commands which cause adb hang. The issue is caused
> > by a request being queued twice. A series of 3 patches from
> > Felipe Balbi in upstream tree fixes this issue.
> >
> > Felipe Balbi (3):
> > usb: dwc3: gadget: add dwc3_request status tracking
> > usb: dwc3: gadget: prevent dwc3_request from being queued twice
> > usb: dwc3: gadget: remove req->started flag
>
> I would like to get an ack from Felipe before I take these.
>
> thanks,
>
> greg k-h
I just realized that I had been testing this patch series with the flag to enable async IO on adb disabled!
It requires a few more patches on top for adb to work properly in async IO mode.
It has been working reliably in our internal tree for some time.
Let me resubmit the whole series after getting it reviewed by Felipe.
Thanks,
Saranya
^ permalink raw reply [flat|nested] 11+ messages in thread