[PATCH v3 1/2] usb: musb: gadget: count givebacks in busy

[Elvira Khabirova]

The ep->busy flag is used by musb_g_giveback() to signal that
req->complete() is running on an endpoint, the ep is busy and
starting a new request is not appropriate right now.

It is only a flag and not a counter currently;
musb_g_giveback() saves the old ep->busy value, sets busy to 1,
releases musb->lock, runs req->complete(), restores the old ep->busy.
This works when there is no possibility of two musb_g_giveback()s
running in parallel.

This does not work when musb_gadget_dequeue()->...->musb_g_giveback()
happens at the same time as musb_interrupt()->...->musb_g_giveback().
The above save-then-restore can get the busy flag stuck at 1.

  CPU0 (interrupt)                CPU1 (usb_ep_dequeue())
  giveback: saved 0, busy = 1
  unlock, ->complete()
                                  giveback: saved 1, busy = 1
                                  unlock, ->complete()
  lock, busy = 0
                                  lock, busy = 1

...and this gets the entire endpoint stuck.

This was reproduced and the patch was tested on a Pine A64+
(Allwinner A64, four cores, no DMA) with PREEMPT_DYNAMIC and lockdep,
using a specially constructed f_fs function that keeps two AIO requests
queued on a bulk endpoint and cancels the newer one with io_cancel(),
in a loop. Tested with both a bulk IN and a bulk OUT endpoint.

If musb_g_giveback() were to be started from some other context,
the same issue would come up.

"Promote" ep->busy from a flag to a counter.

musb_gadget_enable() should not reset `busy` to 0 anymore because
musb_g_giveback() decrements busy, and if these two run in parallel,
it would be problematic (cause an underflow).

Signed-off-by: Elvira Khabirova <lineprinter0@xxxxxxxxx>
---
 drivers/usb/musb/musb_gadget.c | 10 +++++-----
 drivers/usb/musb/musb_gadget.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 016d3f3fc..b500e0d8b 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -129,7 +129,6 @@ __acquires(ep->musb->lock)
 {
 	struct musb_request	*req;
 	struct musb		*musb;
-	int			busy = ep->busy;
 
 	req = to_musb_request(request);
 
@@ -138,7 +137,7 @@ __acquires(ep->musb->lock)
 		req->request.status = status;
 	musb = req->musb;
 
-	ep->busy = 1;
+	ep->busy++;
 	spin_unlock(&musb->lock);
 
 	if (!dma_mapping_error(&musb->g.dev, request->dma))
@@ -147,7 +146,7 @@ __acquires(ep->musb->lock)
 	trace_musb_req_gb(req);
 	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
 	spin_lock(&musb->lock);
-	ep->busy = busy;
+	ep->busy--;
 }
 
 /* ----------------------------------------------------------------------- */
@@ -162,7 +161,7 @@ static void nuke(struct musb_ep *ep, const int status)
 	struct musb_request	*req = NULL;
 	void __iomem *epio = ep->musb->endpoints[ep->current_epnum].regs;
 
-	ep->busy = 1;
+	ep->busy++;
 
 	if (is_dma_capable() && ep->dma) {
 		struct dma_controller	*c = ep->musb->dma_controller;
@@ -195,6 +194,8 @@ static void nuke(struct musb_ep *ep, const int status)
 		req = list_first_entry(&ep->req_list, struct musb_request, list);
 		musb_g_giveback(ep, &req->request, status);
 	}
+
+	ep->busy--;
 }
 
 /* ----------------------------------------------------------------------- */
@@ -1061,7 +1062,6 @@ static int musb_gadget_enable(struct usb_ep *ep,
 		musb_ep->dma = NULL;
 
 	musb_ep->desc = desc;
-	musb_ep->busy = 0;
 	musb_ep->wedged = 0;
 	status = 0;
 
diff --git a/drivers/usb/musb/musb_gadget.h b/drivers/usb/musb/musb_gadget.h
index f49f25b3b..95bc746f7 100644
--- a/drivers/usb/musb/musb_gadget.h
+++ b/drivers/usb/musb/musb_gadget.h
@@ -90,7 +90,7 @@ struct musb_ep {
 
 	u8				wedged;
 
-	/* true if lock must be dropped but req_list may not be advanced */
+	/* number of givebacks in progress; req_list may not be advanced */
 	u8				busy;
 
 	u8				hb_mult;
-- 
2.53.0