Hi, Iago Abal <iago.a...@gmail.com> writes: > Hi, > > This patch basically undoes d3cb25a12138 so, if applied, the problem > reported by Pengyu may need a different fix. > > I can't reproduce the deadlocks, but looking at the code they seem > feasible, let me know otherwise.
I can't take patches which are attached to emails. Can you use git send-email to send your patch to the mailing list? Then I can apply. Other than that, your patch looks okay. > > Cheers, > > Iago > From 75fb3bc167d64715a488bd416f5e08ffdb66e544 Mon Sep 17 00:00:00 2001 > From: Iago Abal <m...@iagoabal.eu> > Date: Mon, 20 Jun 2016 10:40:25 +0200 > Subject: [PATCH] usb: gadget: pch_udc: reorder spin_[un]lock to avoid deadlock > > Fixes: d3cb25a12138 (usb: gadget: udc: fix spin_lock in pch_udc) > > The above commit acquires `&dev->lock' 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 pcd_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. > > Signed-off-by: Iago Abal <m...@iagoabal.eu> > --- > 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 ebc51ec..7175142 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; > } > > @@ -2573,9 +2573,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); > } > } > > @@ -2654,9 +2654,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); > } > > /** > @@ -2691,9 +2691,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); > } > > /** > -- > 1.9.1 > -- balbi
signature.asc
Description: PGP signature
