From 1d23d16a88e6c8143b07339435ba061b131ebb8c Mon Sep 17 00:00:00 2001 From: Iago Abal Date: Tue, 21 Jun 2016 12:01:11 +0200 Subject: [PATCH] usb: gadget: pch_udc: reorder spin_[un]lock to avoid deadlock The above commit reordered spin_lock/unlock and now `&dev->lock' is acquired (rather than released) before calling `dev->driver->disconnect', `dev->driver->setup', `dev->driver->suspend', `usb_gadget_giveback_request', and `usb_gadget_udc_reset'. But this *may* not be the right way to fix the problem pointed by d3cb25a12138. Note that the other usb/gadget/udc drivers do release the lock before calling these functions. There are also inconsistencies within pch_udc.c, where `dev->driver->disconnect' is called while holding `&dev->lock' in lines 613 and 1184, but not in line 2739. Finally, commit d3cb25a12138 may have introduced several potential deadlocks. For instance, EBA (https://github.com/models-team/eba) reports: Double lock in drivers/usb/gadget/udc/pch_udc.c first at 2791: spin_lock(& dev->lock); [pch_udc_isr] second at 2694: spin_lock(& dev->lock); [pch_udc_svc_cfg_interrupt] after calling from 2793: pch_udc_dev_isr(dev, dev_intr); after calling from 2724: pch_udc_svc_cfg_interrupt(dev); Similarly, other potential deadlocks are 2791 -> 2793 -> 2721 -> 2657; and 2791 -> 2793 -> 2711 -> 2573 -> 1499 -> 1480. Fixes: d3cb25a12138 ("usb: gadget: udc: fix spin_lock in pch_udc") Signed-off-by: Iago Abal Signed-off-by: Felipe Balbi --- drivers/usb/gadget/udc/pch_udc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index 8ad847a5d4e4..a97da645c1b9 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1477,11 +1477,11 @@ static void complete_req(struct pch_udc_ep *ep, struct pch_udc_request *req, req->dma_mapped = 0; } ep->halted = 1; - spin_lock(&dev->lock); + spin_unlock(&dev->lock); if (!ep->in) pch_udc_ep_clear_rrdy(ep); usb_gadget_giveback_request(&ep->ep, &req->req); - spin_unlock(&dev->lock); + spin_lock(&dev->lock); ep->halted = halted; } @@ -2567,9 +2567,9 @@ static void pch_udc_svc_ur_interrupt(struct pch_udc_dev *dev) empty_req_queue(ep); } if (dev->driver) { - spin_lock(&dev->lock); - usb_gadget_udc_reset(&dev->gadget, dev->driver); spin_unlock(&dev->lock); + usb_gadget_udc_reset(&dev->gadget, dev->driver); + spin_lock(&dev->lock); } } @@ -2648,9 +2648,9 @@ static void pch_udc_svc_intf_interrupt(struct pch_udc_dev *dev) dev->ep[i].halted = 0; } dev->stall = 0; - spin_lock(&dev->lock); - dev->driver->setup(&dev->gadget, &dev->setup_data); spin_unlock(&dev->lock); + dev->driver->setup(&dev->gadget, &dev->setup_data); + spin_lock(&dev->lock); } /** @@ -2685,9 +2685,9 @@ static void pch_udc_svc_cfg_interrupt(struct pch_udc_dev *dev) dev->stall = 0; /* call gadget zero with setup data received */ - spin_lock(&dev->lock); - dev->driver->setup(&dev->gadget, &dev->setup_data); spin_unlock(&dev->lock); + dev->driver->setup(&dev->gadget, &dev->setup_data); + spin_lock(&dev->lock); } /** -- 2.39.5