On Wednesday 05 December 2007, Alan Stern wrote:
> On Wed, 5 Dec 2007, David Brownell wrote:
>
> > On Wednesday 05 December 2007, Alan Stern wrote:
> > > But it will be easy enough to add IRQF_DISABLED in. Only a few drivers
> > > omit it.
> >
> > I forgot that the flags are passed in to usb_add_hcd()... if we
> > go that route, the drivers that don't pass it should be updated
> > and so should hcd-pci.c; that seems the safest option to me.
>
> The proposed patch is below.
OK by me ... ack. And schedule merge for 2.6.24-final ...
Re Pete's complaint, I can sort of understand not wanting IRQF_DISABLED;
but that's why I commented that this option seems "safest". Alternative
solutions to this little surprise would need more investigation, on more
varied hardware, than I think we can deliver just now. This way has the
advantage of being "obviously correct".
> > This looks like a longstanding bug/oversight. Color me surprised.
>
> It's possible that it has caused undiagnosed lockups in the past. The
> recent changes for urb->status removal must have made them much more
> likely. For instance, the urb_unlink() routine formerly in hcd.c
> used spin_lock_irqsave() but its replacement
> usb_hcd_unlink_urb_from_ep() uses only spin_lock(). That particular
> spinlock is what led to the bug report.
Exactly.
- Dave
> Alan Stern
>
>
> Host controller IRQs are supposed to be serviced with interrupts
> disabled. This patch (as1025) adds an IRQF_DISABLED flag to all the
> controller drivers that lack it. It also replaces the
> spin_lock_irqsave() and spin_unlock_irqrestore() calls in uhci_irq()
> with simple spin_lock() and spin_unlock().
>
> This fixes Bugzilla #9335.
>
> Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
>
> ---
>
> Index: usb-2.6/drivers/usb/core/hcd-pci.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hcd-pci.c
> +++ usb-2.6/drivers/usb/core/hcd-pci.c
> @@ -125,7 +125,7 @@ int usb_hcd_pci_probe (struct pci_dev *d
>
> pci_set_master (dev);
>
> - retval = usb_add_hcd (hcd, dev->irq, IRQF_SHARED);
> + retval = usb_add_hcd(hcd, dev->irq, IRQF_DISABLED | IRQF_SHARED);
> if (retval != 0)
> goto err4;
> return retval;
> Index: usb-2.6/drivers/usb/host/ehci-fsl.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ehci-fsl.c
> +++ usb-2.6/drivers/usb/host/ehci-fsl.c
> @@ -122,7 +122,7 @@ int usb_hcd_fsl_probe(const struct hc_dr
> temp = in_le32(hcd->regs + 0x1a8);
> out_le32(hcd->regs + 0x1a8, temp | 0x3);
>
> - retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
> + retval = usb_add_hcd(hcd, irq, IRQF_DISABLED | IRQF_SHARED);
> if (retval != 0)
> goto err4;
> return retval;
> Index: usb-2.6/drivers/usb/host/ohci-ppc-of.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ohci-ppc-of.c
> +++ usb-2.6/drivers/usb/host/ohci-ppc-of.c
> @@ -142,7 +142,7 @@ ohci_hcd_ppc_of_probe(struct of_device *
>
> ohci_hcd_init(ohci);
>
> - rv = usb_add_hcd(hcd, irq, 0);
> + rv = usb_add_hcd(hcd, irq, IRQF_DISABLED);
> if (rv == 0)
> return 0;
>
> Index: usb-2.6/drivers/usb/host/ohci-ssb.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ohci-ssb.c
> +++ usb-2.6/drivers/usb/host/ohci-ssb.c
> @@ -160,7 +160,7 @@ static int ssb_ohci_attach(struct ssb_de
> hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
> if (!hcd->regs)
> goto err_put_hcd;
> - err = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
> + err = usb_add_hcd(hcd, dev->irq, IRQF_DISABLED | IRQF_SHARED);
> if (err)
> goto err_iounmap;
>
> Index: usb-2.6/drivers/usb/host/r8a66597-hcd.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/r8a66597-hcd.c
> +++ usb-2.6/drivers/usb/host/r8a66597-hcd.c
> @@ -2197,7 +2197,7 @@ static int __init r8a66597_probe(struct
> INIT_LIST_HEAD(&r8a66597->child_device);
>
> hcd->rsrc_start = res->start;
> - ret = usb_add_hcd(hcd, irq, 0);
> + ret = usb_add_hcd(hcd, irq, IRQF_DISABLED);
> if (ret != 0) {
> err("Failed to add hcd");
> goto clean_up;
> Index: usb-2.6/drivers/usb/host/uhci-hcd.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/uhci-hcd.c
> +++ usb-2.6/drivers/usb/host/uhci-hcd.c
> @@ -378,7 +378,6 @@ static irqreturn_t uhci_irq(struct usb_h
> {
> struct uhci_hcd *uhci = hcd_to_uhci(hcd);
> unsigned short status;
> - unsigned long flags;
>
> /*
> * Read the interrupt status, and write it back to clear the
> @@ -398,7 +397,7 @@ static irqreturn_t uhci_irq(struct usb_h
> dev_err(uhci_dev(uhci), "host controller process "
> "error, something bad happened!\n");
> if (status & USBSTS_HCH) {
> - spin_lock_irqsave(&uhci->lock, flags);
> + spin_lock(&uhci->lock);
> if (uhci->rh_state >= UHCI_RH_RUNNING) {
> dev_err(uhci_dev(uhci),
> "host controller halted, "
> @@ -415,16 +414,16 @@ static irqreturn_t uhci_irq(struct usb_h
> * pending unlinks */
> mod_timer(&hcd->rh_timer, jiffies);
> }
> - spin_unlock_irqrestore(&uhci->lock, flags);
> + spin_unlock(&uhci->lock);
> }
> }
>
> if (status & USBSTS_RD)
> usb_hcd_poll_rh_status(hcd);
> else {
> - spin_lock_irqsave(&uhci->lock, flags);
> + spin_lock(&uhci->lock);
> uhci_scan_schedule(uhci);
> - spin_unlock_irqrestore(&uhci->lock, flags);
> + spin_unlock(&uhci->lock);
> }
>
> return IRQ_HANDLED;
>
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
