On Fri, 8 Apr 2011 11:00:17 -0400 (EDT)
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 8 Apr 2011, Alan Stern wrote:
>
> > 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.
>
> Whoops, I spoke too soon. The information doesn't get thrown away.
> Forget I mentioned it.
>
> On the other hand, the interrupts need to be re-enabled somewhere. I
> didn't notice that anywhere in the patch.
>
> Also, the patch assumes that ehci_threaded_irq will be called only in
> response to interrupt events. That isn't (or at least, it isn't
> supposed to be) true. There are places where usb_hcd_irq() is invoked
> directly from other code.
This is true, and the xhci driver wants to use it as a regular irq
handler if MSI doesn't work for them. I suggest we leave usb_hcd_irq
unmodified, and add a new irq handler usb_hcd_quickcheck_irq() to be
used if we have threaded irq, and implement host ops to be:
.irq (regular irq handler, no threaded irq available)
.quickcheck_irq (just do the intr mask and nothing else_
.threaded_irq (the threaded irq that goes with the quickcheck irq)
quickcheck and threaded irq would be called at interrupt time only,
the other entry point should work as it did previously. You can
probably share most code between the existing irq and the threaded_irq.
--
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
