| Message ID | 1443455350-3905-1-git-send-email-balbi@ti.com |
|---|---|
| State | Accepted |
| Commit | 9903b6bedd389a033a3da0308f220571c7f68e7a |
| Headers | show |
Looks good, but
1. We still have
spin_lock(&dev->lock);
dev->driver->disconnect(&dev->gadget);
spin_unlock(&dev->lock);
in
pch_udc_vbus_session()
pch_udc_pcd_pullup().
And also there is lock around dev->driver->setup() in
pch_udc_svc_control_out()
pch_udc_svc_intf_interrupt()
pch_udc_svc_cfg_interrupt().
Is it ok?
2. Deadlocks mentioned in my original report are still there:
pch_udc_isr()
spin_lock(&dev->lock);
pch_udc_dev_isr(dev, dev_intr);
pch_udc_svc_ur_interrupt(dev);
empty_req_queue(ep);
complete_req(ep, req, -ESHUTDOWN);
spin_lock(&dev->lock); <--- deadlock
if (dev->driver) { spin_lock(&dev->lock); } <--- deadlock
--
Alexey
On 28.09.2015 18:49, Felipe Balbi wrote:
> gadget methods should be called without
> spinlocks held.
>
> Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> drivers/usb/gadget/udc/pch_udc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index e5f4c5274298..3181fc9c1c49 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -2747,18 +2747,18 @@ static void pch_udc_dev_isr(struct pch_udc_dev *dev, u32 dev_intr)
> if (dev_intr & UDC_DEVINT_US) {
> if (dev->driver
> && dev->driver->suspend) {
> - spin_lock(&dev->lock);
> - dev->driver->suspend(&dev->gadget);
> spin_unlock(&dev->lock);
> + dev->driver->suspend(&dev->gadget);
> + spin_lock(&dev->lock);
> }
>
> vbus = pch_vbus_gpio_get_value(dev);
> if ((dev->vbus_session == 0)
> && (vbus != 1)) {
> if (dev->driver && dev->driver->disconnect) {
> - spin_lock(&dev->lock);
> - dev->driver->disconnect(&dev->gadget);
> spin_unlock(&dev->lock);
> + dev->driver->disconnect(&dev->gadget);
> + spin_lock(&dev->lock);
> }
> pch_udc_reconnect(dev);
> } else if ((dev->vbus_session == 0)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
(avoid top-posting. If you want to know the reasons, google is your friend) On Tue, Sep 29, 2015 at 10:38:05PM +0300, Alexey Khoroshilov wrote: > Looks good, but > 1. We still have > spin_lock(&dev->lock); > dev->driver->disconnect(&dev->gadget); > spin_unlock(&dev->lock); > in > pch_udc_vbus_session() > pch_udc_pcd_pullup(). both are fine. > And also there is lock around dev->driver->setup() in > pch_udc_svc_control_out() > pch_udc_svc_intf_interrupt() > pch_udc_svc_cfg_interrupt(). > Is it ok? yes > 2. Deadlocks mentioned in my original report are still there: > pch_udc_isr() > spin_lock(&dev->lock); > pch_udc_dev_isr(dev, dev_intr); > pch_udc_svc_ur_interrupt(dev); > empty_req_queue(ep); > complete_req(ep, req, -ESHUTDOWN); > spin_lock(&dev->lock); <--- deadlock no they're not. this is now an unlock. > if (dev->driver) { spin_lock(&dev->lock); } <--- deadlock > > > -- > Alexey > > > On 28.09.2015 18:49, Felipe Balbi wrote: > > gadget methods should be called without > > spinlocks held. > > > > Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > --- > > drivers/usb/gadget/udc/pch_udc.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c > > index e5f4c5274298..3181fc9c1c49 100644 > > --- a/drivers/usb/gadget/udc/pch_udc.c > > +++ b/drivers/usb/gadget/udc/pch_udc.c > > @@ -2747,18 +2747,18 @@ static void pch_udc_dev_isr(struct pch_udc_dev *dev, u32 dev_intr) > > if (dev_intr & UDC_DEVINT_US) { > > if (dev->driver > > && dev->driver->suspend) { > > - spin_lock(&dev->lock); > > - dev->driver->suspend(&dev->gadget); > > spin_unlock(&dev->lock); > > + dev->driver->suspend(&dev->gadget); > > + spin_lock(&dev->lock); > > } > > > > vbus = pch_vbus_gpio_get_value(dev); > > if ((dev->vbus_session == 0) > > && (vbus != 1)) { > > if (dev->driver && dev->driver->disconnect) { > > - spin_lock(&dev->lock); > > - dev->driver->disconnect(&dev->gadget); > > spin_unlock(&dev->lock); > > + dev->driver->disconnect(&dev->gadget); > > + spin_lock(&dev->lock); > > } > > pch_udc_reconnect(dev); > > } else if ((dev->vbus_session == 0) > > >
diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index e5f4c5274298..3181fc9c1c49 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -2747,18 +2747,18 @@ static void pch_udc_dev_isr(struct pch_udc_dev *dev, u32 dev_intr) if (dev_intr & UDC_DEVINT_US) { if (dev->driver && dev->driver->suspend) { - spin_lock(&dev->lock); - dev->driver->suspend(&dev->gadget); spin_unlock(&dev->lock); + dev->driver->suspend(&dev->gadget); + spin_lock(&dev->lock); } vbus = pch_vbus_gpio_get_value(dev); if ((dev->vbus_session == 0) && (vbus != 1)) { if (dev->driver && dev->driver->disconnect) { - spin_lock(&dev->lock); - dev->driver->disconnect(&dev->gadget); spin_unlock(&dev->lock); + dev->driver->disconnect(&dev->gadget); + spin_lock(&dev->lock); } pch_udc_reconnect(dev); } else if ((dev->vbus_session == 0)
gadget methods should be called without spinlocks held. Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Signed-off-by: Felipe Balbi <balbi@ti.com> --- drivers/usb/gadget/udc/pch_udc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)