usb: gadget: pch-udc: fix lock

Message ID 1443455350-3905-1-git-send-email-balbi@ti.com
State Accepted
Commit 9903b6bedd389a033a3da0308f220571c7f68e7a
Headers show

Commit Message

Felipe Balbi Sept. 28, 2015, 3:49 p.m.
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(-)

Comments

Alexey Khoroshilov Sept. 29, 2015, 7:38 p.m. | #1
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
Felipe Balbi Sept. 29, 2015, 11:49 p.m. | #2
(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)
> > 
>

Patch

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)