Patchwork [TRUSTY] Revert "usb: ehci: fix deadlock when threadirqs option is used"

login
register
mail settings
Submitter Colin King
Date March 6, 2014, 9:20 a.m.
Message ID <1394097635-4433-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/327341/
State New
Headers show

Comments

Colin King - March 6, 2014, 9:20 a.m.
From: Colin Ian King <colin.king@canonical.com>

This patch unfortunately breaks USB on my Lenovo x230 and on a Lenovo
T440. I guess it may break it on a wider range of machines too. I'm
requesting that it is reverted.

This reverts commit 65482e0c807a50dc3ec9d59a7465801f9c56bf52.
---
 drivers/usb/host/ehci-hcd.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)
Tim Gardner - March 6, 2014, 1:28 p.m.
On 03/06/2014 02:20 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> This patch unfortunately breaks USB on my Lenovo x230 and on a Lenovo
> T440. I guess it may break it on a wider range of machines too. I'm
> requesting that it is reverted.
>
> This reverts commit 65482e0c807a50dc3ec9d59a7465801f9c56bf52.
> ---
>   drivers/usb/host/ehci-hcd.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 4dfd6fb..e8ba4c4 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -686,15 +686,8 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
>   	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
>   	u32			status, masked_status, pcd_status = 0, cmd;
>   	int			bh;
> -	unsigned long		flags;
>
> -	/*
> -	 * For threadirqs option we use spin_lock_irqsave() variant to prevent
> -	 * deadlock with ehci hrtimer callback, because hrtimer callbacks run
> -	 * in interrupt context even when threadirqs is specified. We can go
> -	 * back to spin_lock() variant when hrtimer callbacks become threaded.
> -	 */
> -	spin_lock_irqsave(&ehci->lock, flags);
> +	spin_lock (&ehci->lock);
>
>   	status = ehci_readl(ehci, &ehci->regs->status);
>
> @@ -712,7 +705,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
>
>   	/* Shared IRQ? */
>   	if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
> -		spin_unlock_irqrestore(&ehci->lock, flags);
> +		spin_unlock(&ehci->lock);
>   		return IRQ_NONE;
>   	}
>
> @@ -830,7 +823,7 @@ dead:
>
>   	if (bh)
>   		ehci_work (ehci);
> -	spin_unlock_irqrestore(&ehci->lock, flags);
> +	spin_unlock (&ehci->lock);
>   	if (pcd_status)
>   		usb_hcd_poll_rh_status(hcd);
>   	return IRQ_HANDLED;
>

Colin - I'm reluctant to do this since it is a significant divergence 
from upstream. Can you work with the maintainer to figure out why your 
machines break ? Or alternatively the comment above spin_lock_irqsave() 
suggests a revert after hrtimer callbacks become threaded (which ought 
not be too difficult). I could take a shot at it later today.

rtg
Colin King - March 6, 2014, 1:33 p.m.
On 06/03/14 13:28, Tim Gardner wrote:
> On 03/06/2014 02:20 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> This patch unfortunately breaks USB on my Lenovo x230 and on a Lenovo
>> T440. I guess it may break it on a wider range of machines too. I'm
>> requesting that it is reverted.
>>
>> This reverts commit 65482e0c807a50dc3ec9d59a7465801f9c56bf52.
>> ---
>>   drivers/usb/host/ehci-hcd.c | 13 +++----------
>>   1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index 4dfd6fb..e8ba4c4 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -686,15 +686,8 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
>>       struct ehci_hcd        *ehci = hcd_to_ehci (hcd);
>>       u32            status, masked_status, pcd_status = 0, cmd;
>>       int            bh;
>> -    unsigned long        flags;
>>
>> -    /*
>> -     * For threadirqs option we use spin_lock_irqsave() variant to
>> prevent
>> -     * deadlock with ehci hrtimer callback, because hrtimer callbacks
>> run
>> -     * in interrupt context even when threadirqs is specified. We can go
>> -     * back to spin_lock() variant when hrtimer callbacks become
>> threaded.
>> -     */
>> -    spin_lock_irqsave(&ehci->lock, flags);
>> +    spin_lock (&ehci->lock);
>>
>>       status = ehci_readl(ehci, &ehci->regs->status);
>>
>> @@ -712,7 +705,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
>>
>>       /* Shared IRQ? */
>>       if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
>> -        spin_unlock_irqrestore(&ehci->lock, flags);
>> +        spin_unlock(&ehci->lock);
>>           return IRQ_NONE;
>>       }
>>
>> @@ -830,7 +823,7 @@ dead:
>>
>>       if (bh)
>>           ehci_work (ehci);
>> -    spin_unlock_irqrestore(&ehci->lock, flags);
>> +    spin_unlock (&ehci->lock);
>>       if (pcd_status)
>>           usb_hcd_poll_rh_status(hcd);
>>       return IRQ_HANDLED;
>>
> 
> Colin - I'm reluctant to do this since it is a significant divergence
> from upstream. Can you work with the maintainer to figure out why your
> machines break ? Or alternatively the comment above spin_lock_irqsave()
> suggests a revert after hrtimer callbacks become threaded (which ought
> not be too difficult). I could take a shot at it later today.
> 
> rtg

I now see the root cause of my issues, the linux-image-extras* package
was missing causing me not to see the USB devices when I originally
tested the -proposed kernel and when I installed my rebuilt debug kernel
it worked because I installed the *extras.  So, this is a NACK from me.

Colin

Patch

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 4dfd6fb..e8ba4c4 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -686,15 +686,8 @@  static irqreturn_t ehci_irq (struct usb_hcd *hcd)
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	u32			status, masked_status, pcd_status = 0, cmd;
 	int			bh;
-	unsigned long		flags;
 
-	/*
-	 * For threadirqs option we use spin_lock_irqsave() variant to prevent
-	 * deadlock with ehci hrtimer callback, because hrtimer callbacks run
-	 * in interrupt context even when threadirqs is specified. We can go
-	 * back to spin_lock() variant when hrtimer callbacks become threaded.
-	 */
-	spin_lock_irqsave(&ehci->lock, flags);
+	spin_lock (&ehci->lock);
 
 	status = ehci_readl(ehci, &ehci->regs->status);
 
@@ -712,7 +705,7 @@  static irqreturn_t ehci_irq (struct usb_hcd *hcd)
 
 	/* Shared IRQ? */
 	if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
-		spin_unlock_irqrestore(&ehci->lock, flags);
+		spin_unlock(&ehci->lock);
 		return IRQ_NONE;
 	}
 
@@ -830,7 +823,7 @@  dead:
 
 	if (bh)
 		ehci_work (ehci);
-	spin_unlock_irqrestore(&ehci->lock, flags);
+	spin_unlock (&ehci->lock);
 	if (pcd_status)
 		usb_hcd_poll_rh_status(hcd);
 	return IRQ_HANDLED;