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(-)
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); }
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;
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...
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
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
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); }
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
[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
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
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
linux-stable-mirror@lists.linaro.org
-
Gopal, Saranya -
Greg KH -
Greg KH -
Saranya Gopal