[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [RFC 11/22] USB: Set usb_hcd->state and flags for shared roothubs.
From:       Alan Stern <stern () rowland ! harvard ! edu>
Date:       2010-12-31 17:53:24
Message-ID: Pine.LNX.4.44L0.1012311238520.7732-100000 () netrider ! rowland ! org
[Download RAW message or body]

On Thu, 30 Dec 2010, Sarah Sharp wrote:

> The hcd->flags are in a sorry state.  Some of them are clearly specific to
> the particular roothub (HCD_POLL_RH, HCD_POLL_PENDING, and
> HCD_WAKEUP_PENDING), but some flags are related to PCI device state
> (HCD_HW_ACCESSIBLE and HCD_SAW_IRQ).  This is an issue when one PCI device
> can have two roothubs that share the same IRQ line and hardware.
> 
> Make sure to set HCD_FLAG_SAW_IRQ for both roothubs when an interrupt is
> serviced, or an URB is unlinked without an interrupt.  (We can't tell if
> the host actually serviced an interrupt for a particular bus, but we can
> tell it serviced some interrupt.)

You know, SAW_IRQ isn't used much any more.  It was originally added to 
help debug problems involving IRQ routing, but those have been pretty 
much all fixed up by now.  Instead of worrying about it, you can simply 
get rid of this flag entirely.

> HCD_HW_ACCESSIBLE is set once by usb_add_hcd(), which is set for both
> roothubs as they are added, so it doesn't need to be modified.
> HCD_POLL_RH and HCD_POLL_PENDING are only checked by the USB core, and
> they are never set by the xHCI driver, since the roothub never needs to be
> polled.
> 
> The usb_hcd's state field is a similar mess.  Sometimes the state applies
> to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and
> HC_STATE_QUIESCING.  But sometimes the state refers to the roothub state:
> HC_STATE_RESUMING and HC_STATE_SUSPENDED.

I'm not sure this distinction makes any sense.  But in any case, it 
certainly is true that hcd->state is a terrible mess.

> This poses an issue with the xHCI split roothub, where two buses are
> registered for one PCI device.  Each bus in the xHCI split roothub can be
> suspended separately, but both buses must be suspended before the PCI
> device can be suspended.  Therefore, make sure that the USB core checks
> hcd->state equals HC_STATE_SUSPENDED for both roothubs before suspending
> the PCI host.

This is tricky.  Normally both buses will indeed be suspended before
the controller; the only situation where that wouldn't be true is if a
remote wakeup races with the controller suspend.  I wonder if we can't
detect these races in a better way.

> Make sure to kill off the shared roothub when the PCI resume fails.

You can do this in the xhci resume routine instead of in the core.

> The xHCI driver will need to ensure that HC_STATE_HALT, HC_STATE_RUNNING,
> and HC_STATE_QUIESCING will be set for both the roothubs.
> 
> I'm not quite sure if the code in hcd_pci_suspend_noirq() is correct,
> please check.  If one roothub is halted, then both roothubs should be
> halted (since they share the same hardware).  But I suppose there could be
> a race condition where one usb_hcd->state is set to HC_STATE_HALT, but the
> other isn't yet?

That business about not allowing remote wakeup if the controller is 
HALTed can be handled elsewhere, such as in usb_hc_died().

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]