Commit 991145e0 authored by Iago Abal's avatar Iago Abal Committed by Kleber Sacilotto de Souza

usb: gadget: pch_udc: reorder spin_[un]lock to avoid deadlock

BugLink: https://bugs.launchpad.net/bugs/1878232

commit 1d23d16a upstream.

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

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 d3cb25a1 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: d3cb25a1 ("usb: gadget: udc: fix spin_lock in pch_udc")
Signed-off-by: default avatarIago Abal <mail@iagoabal.eu>
Signed-off-by: default avatarFelipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: default avatarIan May <ian.may@canonical.com>
Signed-off-by: default avatarKleber Sacilotto de Souza <kleber.souza@canonical.com>
parent e2595238
...@@ -1488,11 +1488,11 @@ static void complete_req(struct pch_udc_ep *ep, struct pch_udc_request *req, ...@@ -1488,11 +1488,11 @@ static void complete_req(struct pch_udc_ep *ep, struct pch_udc_request *req,
req->dma_mapped = 0; req->dma_mapped = 0;
} }
ep->halted = 1; ep->halted = 1;
spin_lock(&dev->lock); spin_unlock(&dev->lock);
if (!ep->in) if (!ep->in)
pch_udc_ep_clear_rrdy(ep); pch_udc_ep_clear_rrdy(ep);
usb_gadget_giveback_request(&ep->ep, &req->req); usb_gadget_giveback_request(&ep->ep, &req->req);
spin_unlock(&dev->lock); spin_lock(&dev->lock);
ep->halted = halted; ep->halted = halted;
} }
...@@ -2583,9 +2583,9 @@ static void pch_udc_svc_ur_interrupt(struct pch_udc_dev *dev) ...@@ -2583,9 +2583,9 @@ static void pch_udc_svc_ur_interrupt(struct pch_udc_dev *dev)
empty_req_queue(ep); empty_req_queue(ep);
} }
if (dev->driver) { if (dev->driver) {
spin_lock(&dev->lock);
usb_gadget_udc_reset(&dev->gadget, dev->driver);
spin_unlock(&dev->lock); spin_unlock(&dev->lock);
usb_gadget_udc_reset(&dev->gadget, dev->driver);
spin_lock(&dev->lock);
} }
} }
...@@ -2664,9 +2664,9 @@ static void pch_udc_svc_intf_interrupt(struct pch_udc_dev *dev) ...@@ -2664,9 +2664,9 @@ static void pch_udc_svc_intf_interrupt(struct pch_udc_dev *dev)
dev->ep[i].halted = 0; dev->ep[i].halted = 0;
} }
dev->stall = 0; dev->stall = 0;
spin_lock(&dev->lock);
dev->driver->setup(&dev->gadget, &dev->setup_data);
spin_unlock(&dev->lock); spin_unlock(&dev->lock);
dev->driver->setup(&dev->gadget, &dev->setup_data);
spin_lock(&dev->lock);
} }
/** /**
...@@ -2701,9 +2701,9 @@ static void pch_udc_svc_cfg_interrupt(struct pch_udc_dev *dev) ...@@ -2701,9 +2701,9 @@ static void pch_udc_svc_cfg_interrupt(struct pch_udc_dev *dev)
dev->stall = 0; dev->stall = 0;
/* call gadget zero with setup data received */ /* call gadget zero with setup data received */
spin_lock(&dev->lock);
dev->driver->setup(&dev->gadget, &dev->setup_data);
spin_unlock(&dev->lock); spin_unlock(&dev->lock);
dev->driver->setup(&dev->gadget, &dev->setup_data);
spin_lock(&dev->lock);
} }
/** /**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment