On Tuesday 24 October 2006 9:05 am, Alan Stern wrote:
> This patch (as808) fixes a minor race in ohci-hcd's root-hub code. If
> we have to turn off the Root-Hub-Status-Change bit in the Interrupt
> Status register, better to do it _before_ looking for status changes
Patch looks OK, but the point is "disable the RHSC IRQ" ... not
"turn off the RHSC bit in interrrupt status register" (a.k.a.
"ack the RHSC IRQ"), which was already done in the IRQ handler.
Plus, when you moved this code from the IRQ handler, it lost the
context: that disabling that IRQ is part of IRQ handling logic,
because of the level/edge triggering choice noted in the comment.
That's no longer clear from the comment ... can you update it?
Or better yet, just move this block of code back to the IRQ handler,
which is where IMO it really belongs.
In general I find such changes to be racey ... lots of silicon gets
rather uppity when drivers modify irq signal paths outside IRQ handlers,
as you changed it to do. There are delays between signaling an IRQ
and delivering it, and by moving this code you **created** a window
whereby non-IRQ code could turn off an IRQ after it's been signaled
yet before it was delivered. That creates a type of controller state
which has unfortunately been very good at making trouble. And for
that matter the move also slows down irq and other code paths by
adding an extra register read ...
(It's different when the IRQ is being masked off at the IRQ controller
level, since those controllers are a lot simpler, but even there the
processing of edge triggers is tricky. One of the bigger updates in
the genirq patches was the edge triggering support.)
- Dave
> rather than _after_.
>
> Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
>
> ---
>
> Index: usb-2.6/drivers/usb/host/ohci-hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/host/ohci-hub.c
> +++ usb-2.6/drivers/usb/host/ohci-hub.c
> @@ -362,6 +362,21 @@ ohci_hub_status_data (struct usb_hcd *hc
> goto done;
> }
>
> + /* NOTE: Vendors didn't always make the same implementation
> + * choices for RHSC. Sometimes it triggers on an edge (like
> + * setting and maybe clearing a port status change bit); and
> + * it's level-triggered on other silicon, active until khubd
> + * clears all active port status change bits. If it's still
> + * set (level-triggered) we must disable it and rely on
> + * polling until khubd re-enables it.
> + */
> + if (ohci_readl(ohci, &ohci->regs->intrstatus) & OHCI_INTR_RHSC) {
> + ohci_writel(ohci, OHCI_INTR_RHSC, &ohci->regs->intrdisable);
> + (void) ohci_readl(ohci, &ohci->regs->intrdisable);
> + rhsc_enabled = 0;
> + }
> + hcd->poll_rh = 1;
> +
> /* init status */
> if (roothub_status (ohci) & (RH_HS_LPSC | RH_HS_OCIC))
> buf [0] = changed = 1;
> @@ -389,21 +404,6 @@ ohci_hub_status_data (struct usb_hcd *hc
> }
> }
>
> - /* NOTE: vendors didn't always make the same implementation
> - * choices for RHSC. Sometimes it triggers on an edge (like
> - * setting and maybe clearing a port status change bit); and
> - * it's level-triggered on other silicon, active until khubd
> - * clears all active port status change bits. If it's still
> - * set (level-triggered) we must disable it and rely on
> - * polling until khubd re-enables it.
> - */
> - if (ohci_readl (ohci, &ohci->regs->intrstatus) & OHCI_INTR_RHSC) {
> - ohci_writel (ohci, OHCI_INTR_RHSC, &ohci->regs->intrdisable);
> - (void) ohci_readl (ohci, &ohci->regs->intrdisable);
> - rhsc_enabled = 0;
> - }
> - hcd->poll_rh = 1;
> -
> /* carry out appropriate state changes */
> switch (ohci->hc_control & OHCI_CTRL_HCFS) {
>
>
>
> -------------------------------------------------------------------------
> Using Tomcat but need to do more? Need to support web services, security?
> Get stuff done quickly with pre-integrated technology to make your job easier
> Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
> _______________________________________________
> [email protected]
> To unsubscribe, use the last form field at:
> https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
>
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
