Re: [patch] Real-Time Preemption, -RT-2.6.13-rc6-V0.7.53-11

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/