[PATCH] Updated: Fix soft lockup in at91 udc driver

[PATCH] Updated: Fix soft lockup in at91 udc driver

[Ryan Mallon]

Fix a potential soft lockup in the AT91 UDC driver by ensuring that the
UDC clock is enabled inside the interrupt handler. If the UDC clock is
not enabled then the UDC registers cannot be written to and the
interrupt cannot be cleared or masked.

Note that this patch (and other parts of the existing AT91 UDC driver)
is potentially racy for preempt-rt kernels, but is okay for mainline.

Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
Tested-by: Remy Bohmer <linux@bohmer.net>
Cc: Andrew Victor <avictor.za@gmail.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>

----

diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
index c4997e9..5ba8319 100644
--- a/drivers/usb/gadget/at91_udc.c
+++ b/drivers/usb/gadget/at91_udc.c
@@ -1370,6 +1370,12 @@ static irqreturn_t at91_udc_irq (int irq, void *_udc)
 {
 	struct at91_udc		*udc = _udc;
 	u32			rescans = 5;
+	int			disable_clock = 0;
+
+	if (!udc->clocked) {
+		clk_on(udc);
+		disable_clock = 1;
+	}
 
 	while (rescans--) {
 		u32 status;
@@ -1458,6 +1464,9 @@ static irqreturn_t at91_udc_irq (int irq, void *_udc)
 		}
 	}
 
+	if (disable_clock)
+		clk_off(udc);
+
 	return IRQ_HANDLED;
 }
 

[PATCH] Updated: Fix soft lockup in at91 udc driver

[Ryan Mallon]

Ryan Mallon wrote:
> Fix a potential soft lockup in the AT91 UDC driver by ensuring that the
> UDC clock is enabled inside the interrupt handler. If the UDC clock is
> not enabled then the UDC registers cannot be written to and the
> interrupt cannot be cleared or masked.
> 
> Note that this patch (and other parts of the existing AT91 UDC driver)
> is potentially racy for preempt-rt kernels, but is okay for mainline.
> 
> Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
> Tested-by: Remy Bohmer <linux@bohmer.net>
> Cc: Andrew Victor <avictor.za@gmail.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>

Is somebody able to give me an Acked-by for this patch. It has been
verified to correct a soft-lockup issue, and I don't want it getting
lost for another year :-).

Thanks,
~Ryan

> ----
> 
> diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
> index c4997e9..5ba8319 100644
> --- a/drivers/usb/gadget/at91_udc.c
> +++ b/drivers/usb/gadget/at91_udc.c
> @@ -1370,6 +1370,12 @@ static irqreturn_t at91_udc_irq (int irq, void *_udc)
>  {
>  	struct at91_udc		*udc = _udc;
>  	u32			rescans = 5;
> +	int			disable_clock = 0;
> +
> +	if (!udc->clocked) {
> +		clk_on(udc);
> +		disable_clock = 1;
> +	}
>  
>  	while (rescans--) {
>  		u32 status;
> @@ -1458,6 +1464,9 @@ static irqreturn_t at91_udc_irq (int irq, void *_udc)
>  		}
>  	}
>  
> +	if (disable_clock)
> +		clk_off(udc);
> +
>  	return IRQ_HANDLED;
>  }
>  
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

[PATCH] Updated: Fix soft lockup in at91 udc driver

[Uwe Kleine-König]

Hello,

On Fri, Jan 15, 2010 at 08:50:14AM +1300, Ryan Mallon wrote:
> Fix a potential soft lockup in the AT91 UDC driver by ensuring that the
> UDC clock is enabled inside the interrupt handler. If the UDC clock is
> not enabled then the UDC registers cannot be written to and the
> interrupt cannot be cleared or masked.
Wouldn't it be cleaner to assert that the clock is already on when an
irq comes in?  That is enable the clock and let it enabled when
configuring the device to generate irqs?

Just my 2?
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] Updated: Fix soft lockup in at91 udc driver

[Remy Bohmer]

Hi,

2010/1/19 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Fri, Jan 15, 2010 at 08:50:14AM +1300, Ryan Mallon wrote:
>> Fix a potential soft lockup in the AT91 UDC driver by ensuring that the
>> UDC clock is enabled inside the interrupt handler. If the UDC clock is
>> not enabled then the UDC registers cannot be written to and the
>> interrupt cannot be cleared or masked.
> Wouldn't it be cleaner to assert that the clock is already on when an
> irq comes in? ?That is enable the clock and let it enabled when
> configuring the device to generate irqs?
>
> Just my 2?

What we have seen here is that there is a condition during host-suspend.
When the host-suspends (3ms idle) the clocks are being disabled, after
a short time the host wakes up again and does a device reset.
This reset triggers a resume interrupt with the clocks disabled. BUT,
it only behaves like this with only a few host controllers. This
results in a hanging interrupt.
The locking code of this driver is not completely clean and that needs
some rework, especially for preempt-rt.
We want to rework this, but that takes more time. Until then this
patch at least solves the issue and can be used safely.

Kind regards,

Remy