[PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice

[PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice

[Saranya Gopal]

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

 drivers/usb/dwc3/core.h   | 11 +++++++++--
 drivers/usb/dwc3/gadget.c |  9 ++++++++-
 drivers/usb/dwc3/gadget.h |  4 ++--
 3 files changed, 19 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH 4.19.y 1/3] usb: dwc3: gadget: add dwc3_request status tracking

[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

[PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice

[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

[PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag

[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

Re: [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag

[Greg KH]

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

Re: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice

[Greg KH]

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

RE: [PATCH 4.19.y 3/3] usb: dwc3: gadget: remove req->started flag

[Gopal, Saranya]

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

RE: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice

[Gopal, Saranya]

> 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

Re: [PATCH 4.19.y 2/3] usb: dwc3: gadget: prevent dwc3_request from being queued twice

[Greg KH]

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

Re: [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice

[Greg KH]

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

RE: [PATCH 4.19.y 0/3] usb: dwc3: Prevent requests from being queued twice

[Gopal, Saranya]

> 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