[PATCH] usb: otg: twl4030-usb: spin_unlock_irq in interrupt handler

[PATCH] usb: otg: twl4030-usb: spin_unlock_irq in interrupt handler

[Denis Efremov]

The replacement of spin_lock_irq/spin_unlock_irq pair in
twl4030_usb_linkstat function by
spin_lock_irqsave/spin_lock_irqrestore pair.
The twl4030_usb_linkstat function is called from twl4030_usb_irq
interrupt handler. Therefore reenabling of handler interrupt line
should be avoided.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>
---
 drivers/usb/otg/twl4030-usb.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index c4a86da..c89a2f5 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -251,6 +251,7 @@ static enum usb_phy_events twl4030_usb_linkstat(struct twl4030_usb *twl)
 	int	status;
 	int	linkstat = USB_EVENT_NONE;
 	struct usb_otg *otg = twl->phy.otg;
+	unsigned long flags;
 
 	twl->vbus_supplied = false;
 
@@ -288,7 +289,7 @@ static enum usb_phy_events twl4030_usb_linkstat(struct twl4030_usb *twl)
 	 * are registered, and that both are active...
 	 */
 
-	spin_lock_irq(&twl->lock);
+	spin_lock_irqsave(&twl->lock, flags);
 	twl->linkstat = linkstat;
 	if (linkstat == USB_EVENT_ID) {
 		otg->default_a = true;
@@ -297,7 +298,7 @@ static enum usb_phy_events twl4030_usb_linkstat(struct twl4030_usb *twl)
 		otg->default_a = false;
 		twl->phy.state = OTG_STATE_B_IDLE;
 	}
-	spin_unlock_irq(&twl->lock);
+	spin_unlock_irqrestore(&twl->lock, flags);
 
 	return linkstat;
 }
-- 
1.7.7

Re: [PATCH] usb: otg: twl4030-usb: spin_unlock_irq in interrupt handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

Hi,

On Sat, Jul 21, 2012 at 11:40:18AM +0400, Denis Efremov wrote:
> The replacement of spin_lock_irq/spin_unlock_irq pair in
> twl4030_usb_linkstat function by
> spin_lock_irqsave/spin_lock_irqrestore pair.
> The twl4030_usb_linkstat function is called from twl4030_usb_irq
> interrupt handler. Therefore reenabling of handler interrupt line
> should be avoided.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>

I have pushed a patch which I think solves this issue. Can you test ?

commit 6b03b13336ee5d8da7bda8799c9ed990e3daedcc
Author: Felipe Balbi <balbi@ti.com>
Date:   Thu Jun 14 13:24:42 2012 +0300

    usb: otg: twl: add missing IRQF_ONESHOT
    
    this patch fixes the following warning:
    
    [    2.825378] genirq: Threaded irq requested \
    	with handler=NULL and !ONESHOT for irq 363
    
    Signed-off-by: Felipe Balbi <balbi@ti.com>

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index c4a86da..0297930 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -651,8 +651,8 @@ static int __devinit twl4030_usb_probe(struct platform_device *pdev)
 	 */
 	twl->irq_enabled = true;
 	status = request_threaded_irq(twl->irq, NULL, twl4030_usb_irq,
-			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
-			"twl4030_usb", twl);
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING |
+			IRQF_ONESHOT, "twl4030_usb", twl);
 	if (status < 0) {
 		dev_dbg(&pdev->dev, "can't get IRQ %d, err %d\n",
 			twl->irq, status);

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: otg: twl4030-usb: spin_unlock_irq in interrupt handler

[Denis]

On 09.08.2012 13:37, Felipe Balbi wrote:
> Hi,
>
> I have pushed a patch which I think solves this issue. Can you test ?
Hi,
my patch is unneeded since this is a threaded interrupt handler. And it 
runs with
interrupt line masked globally on the controller(because of IRQF_ONESHOT).
And I think that "sti" instruction can't reenable this line because it 
is cpu related.
The point was that if "local_irq_enable" is called in primary irq 
handler or in
request_irq handler you reenables (maybe accidentally) interrupts on the 
cpu.
Because after patch https://lkml.org/lkml/2010/3/25/434 these handlers 
runs with
interrupts disabled.
Some time ago our static tool didn't make any difference between 
ordinary and
threaded interrupts handlers. At that time I didn't know it and didn't 
check the
code enough. So it was a false positive.