* Peter Zijlstra <[email protected]> wrote:
> --- linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c~ 2005-08-15 21:23:45.000000000 +0200
> +++ linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c 2005-08-15 22:03:33.000000000 +0200
> @@ -506,13 +506,11 @@ error:
> }
>
> /* any errors get returned through the urb completion */
> - local_irq_save (flags);
> + local_irq_save_nort (flags);
> spin_lock (&urb->lock);
> if (urb->status == -EINPROGRESS)
> urb->status = status;
> spin_unlock (&urb->lock);
> usb_hcd_giveback_urb (hcd, urb, NULL);
> - local_irq_restore (flags);
> + local_irq_restore_nort (flags);
> return 0;
> }
i'm wondering whether we could/should also fix this upstream - and
whether this [making the IRQ flags disabling a NOP on -RT] is the right
fix. Why does the USB hcd.c code do this in the first place? Disabling
interrupts during usb_hcd_giveback_urb() [but not holding the urb->lock]
might serialize on UP, but it has no serialization effect on SMP and is
hence potentially buggy. Is there something i'm missing about this code?
the normal way of using urb->lock would be spin_lock_irqsave() and
spin_lock_irqrestore(), not the 'detached' method seen above.
> similar fix, completions need not have irqs disabled on PREEMPT_RT
> right?
correct, PREEMPT_RT is very strict about the use of the interrupt flags.
A fair portion of the now-illegal API uses are also SMP bugs on
upstream, so these details are worth pursuing.
> --- linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c~ 2005-08-15 22:03:33.000000000 +0200
> +++ linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c 2005-08-15 22:32:54.000000000 +0200
> @@ -538,7 +538,7 @@ void usb_hcd_poll_rh_status(struct usb_h
> if (length > 0) {
>
> /* try to complete the status urb */
> - local_irq_save (flags);
> + local_irq_save_nort (flags);
> spin_lock(&hcd_root_hub_lock);
> urb = hcd->status_urb;
> if (urb) {
> @@ -562,7 +562,7 @@ void usb_hcd_poll_rh_status(struct usb_h
> usb_hcd_giveback_urb (hcd, urb, NULL);
> else
> hcd->poll_pending = 1;
> - local_irq_restore (flags);
> + local_irq_restore_nort (flags);
same question: why are interrupts being kept disabled longer, and why is
usb_hcd_giveback_urb() called with interrupts disabled? (I tried to use
spin_lock_irqsave/irqrestore() in earlier -RT versions, but people
reported hangs and USB misbehavior, which might be related. I'm worried
that your _nort patch could cause similar misbehavior.)
how about (naively) extending the urb->lock to cover
usb_hcd_giveback_urb() calls too - does that cause a deadlock or is it
unsafe in some other way?
Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |