USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce power consumption if USB PLL is not already necessary for OHCI or EHCI. If USB host is not connected we can sleep with USB PLL stopped.

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

drivers/usb/gadget/udc/atmel_usba_udc.c +0 -0

 			if (udc->driver->disconnect) {
 				spin_unlock(&udc->lock);
 				udc->driver->disconnect(&udc->gadget);

@@ -1772,15 +1786,6 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 	udc->driver = driver;
 	spin_unlock_irqrestore(&udc->lock, flags);

drivers/usb/gadget/udc/atmel_usba_udc.c +0 -0

 	udc->vbus_prev = 0;
 	if (gpio_is_valid(udc->vbus_pin))
 		enable_irq(gpio_to_irq(udc->vbus_pin));

@@ -1788,13 +1793,27 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 	/* If Vbus is present, enable the controller and wait for reset */
 	spin_lock_irqsave(&udc->lock, flags);

drivers/usb/gadget/udc/atmel_usba_udc.c +5 -0

 		toggle_bias(1);
 		usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 		usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+
+		udc->vbus_prev = 1;
 	}
 	spin_unlock_irqrestore(&udc->lock, flags);
 
 	return 0;
+out:
+	spin_unlock_irqrestore(&udc->lock, flags);
+	return ret;
 }
 

Hi,

Quoted 9 lines · Sylvain Rochet

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce power consumption if USB PLL is not already necessary for OHCI or EHCI. If USB host is not connected we can sleep with USB PLL stopped.

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Same as your previous patch, the maintainers are:
Felipe Balbi <[email protected]> (maintainer:USB GADGET/PERIPH...) Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)

drivers/usb/gadget/udc/atmel_usba_udc.c static irqreturn_t usba_vbus_irq(int irq, void *devid) · line 1746

Quoted 11 lines

drivers/usb/gadget/udc/atmel_usba_udc.c +7 -0

index ce88237..8ea0a63 100644
+			ret = clk_prepare_enable(udc->pclk);
+			if (ret)
+				goto out;
+			ret = clk_prepare_enable(udc->hclk);
+			if (ret) {
+				clk_disable_unprepare(udc->pclk);
+				goto out;

You probably got a warning at some point in time, you can't use clk_prepare or clk_unprepare in an irq handler as they may sleep (that is exactly the point of having clk_prepare ans clk_enable)

So, use clk_enable and clk_disable here.

You're right, except that calling enable/disable on the PLL clk in irq context is pretty much useless since the activation/deactivation code of the PLL is in prepare/unprepare, so you won't save much power by doing that (gating the peripheral clk should save a bit though).

To solve that issue I thought we could move to a threaded_irq (where we can safely sleep), but you'll also have to take of not calling prepare/unprepare in sections where at least one spinlock is taken (for the same reason => you cannot sleep with while you hold spinlocks).

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce power consumption if USB PLL is not already necessary for OHCI or EHCI. If USB host is not connected we can sleep with USB PLL stopped.

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Signed-off-by Sylvain Rochet <[email protected]>

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 95 ++++++++++++++++++++++++---------
 drivers/usb/gadget/udc/atmel_usba_udc.h |  4 ++
 2 files changed, 73 insertions(+), 26 deletions(-)

drivers/usb/gadget/udc/atmel_usba_udc.c +34 -1

index e207d75..986677b 100644
@@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
 }
 #endif
 
+static int start_clock(struct usba_udc *udc)
+{
+	int ret;
+
+	if (udc->clocked)
+		return 0;
+
+	ret = clk_prepare_enable(udc->pclk);
+	if (ret)
+		return ret;
+	ret = clk_prepare_enable(udc->hclk);
+	if (ret) {
+		clk_disable_unprepare(udc->pclk);
+		return ret;
+	}
+
+	udc->clocked = true;
+	return ret;
+}
+
+static int stop_clock(struct usba_udc *udc)
+{
+	if (!udc->clocked)
+		return 0;
+
+	clk_disable_unprepare(udc->hclk);
+	clk_disable_unprepare(udc->pclk);
+
+	udc->clocked = false;
+	return 0;
+}
+
 static int vbus_is_present(struct usba_udc *udc)
 {
 	if (gpio_is_valid(udc->vbus_pin))

@@ -1719,42 +1751,55 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t usba_vbus_irq(int irq, void *devid)
+static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
 {
 	struct usba_udc *udc = devid;
 	int vbus;
+	int ret;

If vbus gpio is high at init, we should set vbus_prev to true accordingly to the current vbus state. Without that, we skip the first vbus interrupt because the saved vbus state is not consistent.

Signed-off-by Sylvain Rochet <[email protected]>

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/usb/gadget/udc/atmel_usba_udc.c +2 -0

index ce88237..e207d75 100644
@@ -1791,6 +1791,8 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 		toggle_bias(1);
 		usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 		usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+
+		udc->vbus_prev = 1;
 	}
 	spin_unlock_irqrestore(&udc->lock, flags);
 

Le 18/01/2015 18:24, Sylvain Rochet a ?crit :

Quoted 4 lines

If vbus gpio is high at init, we should set vbus_prev to true accordingly to the current vbus state. Without that, we skip the first vbus interrupt because the saved vbus state is not consistent.

Quoted 1 line

Signed-off-by Sylvain Rochet <[email protected]>

Indeed:

Acked-by Nicolas Ferre <[email protected]>

We can also add the following tags:

Fixes 914a3f3b3754 ("USB: add atmel_usba_udc driver")

Cc <[email protected]> # 2.6.x-ish

Quoted 10 lines · Nicolas Ferre

Le 18/01/2015 18:24, Sylvain Rochet a ?crit :

Quoted 5 lines

If vbus gpio is high at init, we should set vbus_prev to true accordingly to the current vbus state. Without that, we skip the first vbus interrupt because the saved vbus state is not consistent.

Signed-off-by Sylvain Rochet <[email protected]>

Indeed:

Acked-by Nicolas Ferre <[email protected]>

Quoted 3 lines

We can also add the following tags:

Fixes 914a3f3b3754 ("USB: add atmel_usba_udc driver")

Cc <[email protected]> # 2.6.x-ish

Please resend with the proper stable format. Also, this 2.6.x-ish should be 2.6.24+, git describe helps a lot figuring these out.

Hello,

Quoted 11 lines · Felipe Balbi

Quoted 10 lines · Nicolas Ferre

Le 18/01/2015 18:24, Sylvain Rochet a ?crit :

Quoted 5 lines

If vbus gpio is high at init, we should set vbus_prev to true accordingly to the current vbus state. Without that, we skip the first vbus interrupt because the saved vbus state is not consistent.

Signed-off-by Sylvain Rochet <[email protected]>

Indeed:

Acked-by Nicolas Ferre <[email protected]>

Quoted 6 lines

Quoted 3 lines

We can also add the following tags:

Fixes 914a3f3b3754 ("USB: add atmel_usba_udc driver")

Cc <[email protected]> # 2.6.x-ish

Please resend with the proper stable format. Also, this 2.6.x-ish should be 2.6.24+, git describe helps a lot figuring these out.

Unfortunately I failed to understand what I should do. The "Also" keyword means for me that there are two differents things to fix:

1. proper stable format
2. 2.6.x-ish ? 2.6.24+

2. does not imply fixing 1., isn'it ?

I read SubmittingPatches thoroughly and I can't find a reference to stable format.

Sylvain

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce

power consumption if USB PLL is not already necessary for OHCI or EHCI.

If USB host is not connected we can sleep with USB PLL stopped.

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Signed-off-by Sylvain Rochet <[email protected]>

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 96 ++++++++++++++++++++++++---------
 drivers/usb/gadget/udc/atmel_usba_udc.h |  4 ++
 2 files changed, 74 insertions(+), 26 deletions(-)

drivers/usb/gadget/udc/atmel_usba_udc.c +35 -2

index e207d75..9cce50a 100644
@@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
 }
 #endif
 
+static int start_clock(struct usba_udc *udc)
+{
+	int ret;
+
+	if (udc->clocked)
+		return 0;
+
+	ret = clk_prepare_enable(udc->pclk);
+	if (ret)
+		return ret;
+	ret = clk_prepare_enable(udc->hclk);
+	if (ret) {
+		clk_disable_unprepare(udc->pclk);
+		return ret;
+	}
+
+	udc->clocked = true;
+	return ret;
+}
+
+static int stop_clock(struct usba_udc *udc)
+{
+	if (!udc->clocked)
+		return 0;
+
+	clk_disable_unprepare(udc->hclk);
+	clk_disable_unprepare(udc->pclk);
+
+	udc->clocked = false;
+	return 0;
+}
+
 static int vbus_is_present(struct usba_udc *udc)
 {
 	if (gpio_is_valid(udc->vbus_pin))

@@ -1719,42 +1751,56 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t usba_vbus_irq(int irq, void *devid)
+static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
 {
 	struct usba_udc *udc = devid;
 	int vbus;
+	int ret;
+	unsigned long flags;
 
 	/* debounce */
 	udelay(10);
 
-	spin_lock(&udc->lock);

drivers/usb/gadget/udc/atmel_usba_udc.c +29 -23

 	vbus = vbus_is_present(udc);
 	if (vbus != udc->vbus_prev) {
 		if (vbus) {
+			ret = start_clock(udc);
+			if (ret)
+				goto out;
+
+			spin_lock_irqsave(&udc->lock, flags);
 			toggle_bias(1);
 			usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 			usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+			spin_unlock_irqrestore(&udc->lock, flags);
 		} else {
+			spin_lock_irqsave(&udc->lock, flags);
 			udc->gadget.speed = USB_SPEED_UNKNOWN;
 			reset_all_endpoints(udc);
 			toggle_bias(0);
 			usba_writel(udc, CTRL, USBA_DISABLE_MASK);
-			if (udc->driver->disconnect) {
-				spin_unlock(&udc->lock);
+			spin_unlock_irqrestore(&udc->lock, flags);
+
+			stop_clock(udc);
+
+			if (udc->driver->disconnect)
 				udc->driver->disconnect(&udc->gadget);
-				spin_lock(&udc->lock);
-			}
 		}
 		udc->vbus_prev = vbus;
 	}
 
 out:
-	spin_unlock(&udc->lock);
+	mutex_unlock(&udc->vbus_mutex);
 
 	return IRQ_HANDLED;
 }

@@ -1762,7 +1808,7 @@ out:
 static int atmel_usba_start(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver)
 {
-	int ret;
+	int ret = 0;
 	struct usba_udc *udc = container_of(gadget, struct usba_udc, gadget);
 	unsigned long flags;
 

@@ -1772,31 +1818,29 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 	udc->driver = driver;
 	spin_unlock_irqrestore(&udc->lock, flags);
 
-	ret = clk_prepare_enable(udc->pclk);
-	if (ret)
-		return ret;
-	ret = clk_prepare_enable(udc->hclk);
-	if (ret) {
-		clk_disable_unprepare(udc->pclk);
-		return ret;
-	}
-
+	mutex_lock(&udc->vbus_mutex);
 	udc->vbus_prev = 0;
 	if (gpio_is_valid(udc->vbus_pin))
 		enable_irq(gpio_to_irq(udc->vbus_pin));
 
 	/* If Vbus is present, enable the controller and wait for reset */
-	spin_lock_irqsave(&udc->lock, flags);
 	if (vbus_is_present(udc) && udc->vbus_prev == 0) {
+		ret = start_clock(udc);
+		if (ret)
+			goto out;
+
+		spin_lock_irqsave(&udc->lock, flags);
 		toggle_bias(1);
 		usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 		usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
+		spin_unlock_irqrestore(&udc->lock, flags);
 
 		udc->vbus_prev = 1;
 	}
-	spin_unlock_irqrestore(&udc->lock, flags);
 
-	return 0;
+out:
+	mutex_unlock(&udc->vbus_mutex);
+	return ret;
 }
 
 static int atmel_usba_stop(struct usb_gadget *gadget)

@@ -1816,8 +1860,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 	toggle_bias(0);
 	usba_writel(udc, CTRL, USBA_DISABLE_MASK);
 
-	clk_disable_unprepare(udc->hclk);
-	clk_disable_unprepare(udc->pclk);
+	stop_clock(udc);
 
 	udc->driver = NULL;
 

@@ -1997,6 +2040,7 @@ static int usba_udc_probe(struct platform_device *pdev)
 		return PTR_ERR(hclk);
 
 	spin_lock_init(&udc->lock);
+	mutex_init(&udc->vbus_mutex);
 	udc->pdev = pdev;
 	udc->pclk = pclk;
 	udc->hclk = hclk;

@@ -2049,9 +2093,9 @@ static int usba_udc_probe(struct platform_device *pdev)
 
 	if (gpio_is_valid(udc->vbus_pin)) {
 		if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
-			ret = devm_request_irq(&pdev->dev,
-					gpio_to_irq(udc->vbus_pin),
-					usba_vbus_irq, 0,
+			ret = devm_request_threaded_irq(&pdev->dev,
+					gpio_to_irq(udc->vbus_pin), NULL,
+					usba_vbus_irq_thread, IRQF_ONESHOT,
 					"atmel_usba_udc", udc);
 			if (ret) {
 				udc->vbus_pin = -ENODEV;

drivers/usb/gadget/udc/atmel_usba_udc.h +3 -0

index a70706e..3ceed76 100644
@@ -308,6 +308,9 @@ struct usba_udc {
 	/* Protect hw registers from concurrent modifications */
 	spinlock_t lock;
 
+	/* Mutex to prevent concurrent start or stop */
+	struct mutex vbus_mutex;
+
 	void __iomem *regs;
 	void __iomem *fifo;
 

Le 18/01/2015 18:24, Sylvain Rochet a ?crit :

Quoted 1 line

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce

Please re-write which "edge" we are talking about: "... falling edge of the Vbus signal" for example.

Quoted 1 line

power consumption if USB PLL is not already necessary for OHCI or EHCI.

Is a verb missing in the previous sentence?

Quoted 2 lines

If USB host is not connected we can sleep with USB PLL stopped.

Quoted 4 lines

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Can you please make it more clear. Several separated sentences seem necessary here.

drivers/usb/gadget/udc/atmel_usba_udc.c static irqreturn_t usba_udc_irq(int irq, void *devid) · line 1772

Quoted 8 lines

drivers/usb/gadget/udc/atmel_usba_udc.c +5 -1

 	/* May happen if Vbus pin toggles during probe() */
-	if (!udc->driver)
+	spin_lock_irqsave(&udc->lock, flags);
+	if (!udc->driver) {
+		spin_unlock_irqrestore(&udc->lock, flags);
 		goto out;
+	}
+	spin_unlock_irqrestore(&udc->lock, flags);

I'm not sure that the protection by spin_lock is needed above.

Hello Nicolas,

Quoted 12 lines · Nicolas Ferre

Le 18/01/2015 18:24, Sylvain Rochet a ?crit :

Quoted 1 line

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce

Please re-write which "edge" we are talking about: "... falling edge of the Vbus signal" for example.

Quoted 1 line

power consumption if USB PLL is not already necessary for OHCI or EHCI.

Is a verb missing in the previous sentence?

Quoted 2 lines

If USB host is not connected we can sleep with USB PLL stopped.

Quoted 7 lines

Quoted 4 lines

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Can you please make it more clear. Several separated sentences seem necessary here.

Maybe :)

Proposal:

Start clocks on rising edge of the Vbus signal, stop clocks on falling edge of the Vbus signal.

If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI) it will reduce power consumption by switching off the USB PLL if no USB Host is currently connected to this USB Device.

We are using Vbus GPIO signal to detect Host presence. If Vbus signal is not available then the device stay continuously clocked.

Note this driver does not support suspend/resume yet, it may stay clocked if USB Host is still connected when suspending. For what I need, forbidding suspend from userland if we are still attached to an USB host is fine, but we might as well add suspend/resume to this driver in the future.

Quoted 10 lines

drivers/usb/gadget/udc/atmel_usba_udc.c static irqreturn_t usba_udc_irq(int irq, void *devid) · line 1772

Quoted 8 lines

drivers/usb/gadget/udc/atmel_usba_udc.c +5 -1

 	/* May happen if Vbus pin toggles during probe() */
-	if (!udc->driver)
+	spin_lock_irqsave(&udc->lock, flags);
+	if (!udc->driver) {
+		spin_unlock_irqrestore(&udc->lock, flags);
 		goto out;
+	}
+	spin_unlock_irqrestore(&udc->lock, flags);

I'm not sure that the protection by spin_lock is needed above.

I'm not sure too, it was already in a spinlock area, I obviously kept it because it was not the purpose of this patch.

This seem to be in mirror of atmel_usba_start() which does:

  spin_lock_irqsave(&udc->lock, flags);
  udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
  udc->driver = driver;
  spin_unlock_irqrestore(&udc->lock, flags);

? but vbus_pin IRQ is not yet enabled.

Same for atmel_usba_stop() which disable vbus_pin IRQ before setting udc->driver to NULL, but without spinlock this time (why?, this should be consistent IMHO).

I don't know if it is guaranteed that IRQ does not fire nor continue to run after disable_irq() returns, especially for threaded IRQ.

If the following sequence might happen:
atmel_usba_stop()

    disable_irq(vbus)
                                  usba_vbus_irq_thread() called lately
                                    check for (!udc->driver) and continue
    udc->driver = NULL;
                                    if (udc->driver->disconnect)
                                 *CRASH*

Then the patch should be modified to protect udc->driver with the vbus mutex.

In this case the previous implementation wasn't perfect too, the atmel_usba_stop() does not lock around the NULLification of driver,

Moreover the spinlock is (and was) unlocked in VBUS interrupt just before calling udc->driver->disconnect, which makes udc->driver actually not locked anywere.

If the previous sequence possible ? If no, udc->driver does not need locking, if yes, it is currently not locked enough.

Quoted 32 lines · On Mon, 19 Jan 2015 18:46:31 +0100 Sylvain Rochet…

Hello Nicolas,

Quoted 12 lines · Nicolas Ferre

Le 18/01/2015 18:24, Sylvain Rochet a ?crit :

Quoted 1 line

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce

Please re-write which "edge" we are talking about: "... falling edge of the Vbus signal" for example.

Quoted 1 line

power consumption if USB PLL is not already necessary for OHCI or EHCI.

Is a verb missing in the previous sentence?

Quoted 2 lines

If USB host is not connected we can sleep with USB PLL stopped.

Quoted 7 lines

Quoted 4 lines

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Can you please make it more clear. Several separated sentences seem necessary here.

Maybe :)

Proposal:

Start clocks on rising edge of the Vbus signal, stop clocks on falling edge of the Vbus signal.

Quoted 6 lines

If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI) it will reduce power consumption by switching off the USB PLL if no USB Host is currently connected to this USB Device.

We are using Vbus GPIO signal to detect Host presence. If Vbus signal is not available then the device stay continuously clocked.

Quoted 5 lines

Note this driver does not support suspend/resume yet, it may stay clocked if USB Host is still connected when suspending. For what I need, forbidding suspend from userland if we are still attached to an USB host is fine, but we might as well add suspend/resume to this driver in the future.

Quoted 11 lines

Quoted 10 lines

drivers/usb/gadget/udc/atmel_usba_udc.c static irqreturn_t usba_udc_irq(int irq, void *devid) · line 1772

Quoted 8 lines

drivers/usb/gadget/udc/atmel_usba_udc.c +5 -1

 	/* May happen if Vbus pin toggles during probe() */
-	if (!udc->driver)
+	spin_lock_irqsave(&udc->lock, flags);
+	if (!udc->driver) {
+		spin_unlock_irqrestore(&udc->lock, flags);
 		goto out;
+	}
+	spin_unlock_irqrestore(&udc->lock, flags);

I'm not sure that the protection by spin_lock is needed above.

Quoted 2 lines

I'm not sure too, it was already in a spinlock area, I obviously kept it because it was not the purpose of this patch.

According to the comment placed above this test it's here to prevent any action triggered by the vbus pin irq while the driver is not properly probed yet.
You could use:

set_irq_flags(vbus_irq, IRQF_NOAUTOEN);

before calling devm_request_threaded_irq.
This will keep the irq disabled instead of enabling it at request time. Moreover, this simplify the vbus_irq request code (which disables the irq just after requesting it).

Quoted 11 lines

This seem to be in mirror of atmel_usba_start() which does:

  spin_lock_irqsave(&udc->lock, flags);
  udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
  udc->driver = driver;
  spin_unlock_irqrestore(&udc->lock, flags);

? but vbus_pin IRQ is not yet enabled.

Quoted 7 lines

Same for atmel_usba_stop() which disable vbus_pin IRQ before setting udc->driver to NULL, but without spinlock this time (why?, this should be consistent IMHO).

I don't know if it is guaranteed that IRQ does not fire nor continue to run after disable_irq() returns, especially for threaded IRQ.

And yes, disable_irq waits for all irq handler termination, including threaded irqs: see [1], [2].

Quoted 17 lines

If the following sequence might happen:
atmel_usba_stop()

    disable_irq(vbus)
                                  usba_vbus_irq_thread() called lately
                                    check for (!udc->driver) and continue
    udc->driver = NULL;
                                    if (udc->driver->disconnect)
                                 *CRASH*

Then the patch should be modified to protect udc->driver with the vbus mutex.

In this case the previous implementation wasn't perfect too, the atmel_usba_stop() does not lock around the NULLification of driver,

Quoted 6 lines

Moreover the spinlock is (and was) unlocked in VBUS interrupt just before calling udc->driver->disconnect, which makes udc->driver actually not locked anywere.

If the previous sequence possible ? If no, udc->driver does not need locking, if yes, it is currently not locked enough.

If you rework the vbus_irq request as I suggested I think you can get rid of this !udc->driver test, and thus drop the locking around it ;-).

Best Regards,

Boris

[1]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L432 [2]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L92

Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce power consumption if USB PLL is not already necessary for OHCI or EHCI. If USB host is not connected we can sleep with USB PLL stopped.

This driver does not support suspend/resume yet, not suspending if we are still attached to an USB host is fine for what I need, this patch allow suspending with USB PLL stopped when USB device is not currently used.

Signed-off-by Sylvain Rochet <[email protected]>

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 95 ++++++++++++++++++++++++---------
 drivers/usb/gadget/udc/atmel_usba_udc.h |  4 ++
 2 files changed, 73 insertions(+), 26 deletions(-)

drivers/usb/gadget/udc/atmel_usba_udc.c +0 -0

index e207d75..986677b 100644
@@ -315,6 +315,38 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
 }
 #endif