On Thu, 7 Apr 2011, Greg KH wrote:
> Here's Thomas's first cut at doing this, totally untested and written
> during a legal track at the LF Collab summit.
>
> Kristen, can you take this and test and see if it works for you?
>
> thanks,
>
> greg k-h
>
>
> Subject: usb.patch
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Date: Thu, 07 Apr 2011 23:35:16 +0200
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
> drivers/usb/core/hcd.c | 42 ++++++++++++++++++++----------------------
> drivers/usb/host/ehci-hcd.c | 33 +++++++++++++++++++++++++++------
> drivers/usb/host/ehci-pci.c | 1 +
> include/linux/usb/hcd.h | 1 +
> 4 files changed, 49 insertions(+), 28 deletions(-)
This isn't correct, for a couple of reasons. The most important reason
shows up here:
> Index: linux-2.6/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/host/ehci-hcd.c
> +++ linux-2.6/drivers/usb/host/ehci-hcd.c
> @@ -764,10 +764,35 @@ static int ehci_run (struct usb_hcd *hcd
> static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> + u32 status;
> +
> + spin_lock(&ehci->lock);
> +
> + status = ehci_readl(ehci, &ehci->regs->status);
> + if (status & INTR_MASK)
> + ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> +
> + spin_unlock(&ehci->lock);
> +
> + return status & INTR_MASK ? IRQ_WAKE_THREAD : IRQ_NONE;
> +}
The status information gets thrown away. It has to be saved for later
use.
> +
> +static irqreturn_t ehci_thread_irq (struct usb_hcd *hcd)
> +{
> + struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> u32 status, masked_status, pcd_status = 0, cmd;
> int bh;
>
> - spin_lock (&ehci->lock);
> + /*
> + * The scope of this lock wants to be reduced to protect the
> + * status register access. Serializiation at the thread level
> + * should simply use a mutex, which is not simple either as
> + * you have to deal with the watchdog timer stuff. Needs some
> + * thought. Making it spin_lock_bh() now should be fine. We
> + * just need to disable interrupts right before we write the
> + * interrupt mask register.
> + */
> + spin_lock_irq(&ehci->lock);
This comment is also wrong. The spinlock does not merely protect the
hardware registers; it also protects the driver's data structures,
which can be accessed by code running in_interrupt (i.e., within other
non-threaded interrupt handlers).
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
