Hi Lin,
Thanks for the analysis and suggestions. However, I cannot take your
patch to modify the Intel port switchover mechanism for several reasons.
I'm sorry for the long email, but this question has been asked before,
and I feel the need to document the reasons not to do this, so I'm Ccing
the public Linux USB mailing list.
On Sun, Aug 04, 2013 at 11:57:07PM -0700, Wang, Lin X wrote:
> Hi Sarah,
>
> I’m Lin in OSV enabling team, and i found some possible issues on ports
> switchover when I tried to verify it(3.11.0 RC2/3/4, Greg’s USB next tree ).
>
> Currently ports are unconditionally switched to xHCI if a Intel product(xHCI)
> is found during the PCI enumeration, i verified this feature on HSW-ULT, it
> works normally except on the following situation:
>
> 1. If xHCI driver is not built into the kernel(using loadable kernel module),
> when you remove the driver module(i.e. modprobe -r xhci_hcd), you lost the
> control of keyboard/mice because all ports died.
Yes, that's expected. That's what happens on an Intel EHCI host with a
rate matching hub when you unload the EHCI driver. That's what will
happen if you unload the xHCI driver on a non-Intel host. Users expect
this behavior when they unload the host controller. I don't want to
confuse users by having different driver unload behavior on Intel xHCI
hosts.
Further, how do you propose to handle the port switchover when the xHCI
driver reloads? If we switch the ports from EHCI to xHCI, we disconnect
USB devices that may have been in the middle of active transfers. It's
a mess, and we'd have to somehow coordinate between the two hosts to
release active USB devices. In short, I don't want to try and attempt
to add new code to the USB core for an Intel xHCI host feature that will
go away in the future. The code will just rot, and I don't want to
maintain it.
> Currently xHCI may not be capable of hot plug, but its driver could be
> removable.
>
> 2. If (unlikely) xHCI died without switching ports back to EHCI, we may
> encounter the same situation as above; but, if ports have been switched
> back to EHCI, we may not need to reboot the machine.
The xHCI host dying is a corner case that I do not want to optimize for.
We should fix the driver if this case pops up more often, so please let
me know if you have been seeing this issue.
Also, switching the ports to EHCI when the xHCI host dies won't help USB
3.0 devices. If a USB 3.0 device is attached to the xHCI host, and we
remove SuperSpeed terminations, the device will not notice this until it
sees a bus reset. It will not reconnect on the USB 2.0 bus until it
sees this reset, and thus it will remain attached to the non-responsive
xHCI host. Since the xHCI host is dead, software cannot issue
the bus reset. We don't know if the host will drive the reset down the
bus lines, and it certainly won't send an interrupt when the port reset
is done, which is what the USB core expects.
Therefore, we cannot safely switch over USB 3.0 devices to the EHCI host
when the xHCI host dies.
> So I think it doesn’t make sense if all ports died just because a Intel’s USB
> 3.0 host controller stop works, switching the ports back to EHCI at the right
> time and place might be a good choice.
Users are going to be very confused if some of their USB devices work,
but their USB 3.0 devices don't. Worse, if they switch machines to a
non-Intel xHCI host, if they can reproduce the behavior that caused the
host to die, their USB devices won't switch over to EHCI. They will
simply stop working when the xHCI host dies. That makes debugging these
kinds of issues a giant pain.
In summary, I don't think this is a good idea because:
1. The port switchover mechanism is fragile, and I don't want to use it
more than necessary. BIOS engineers can and will mess this up, and I
don't want to exercise their code more than necessary.
2. The xHCI port switchover will be going away in future Intel chipsets.
I don't want to add new code that won't be maintained in the future.
3. Switching the ports back to EHCI on driver unload is not advisable,
since the xHCI driver will switch the ports back on reload, and that
would disconnect active USB devices under EHCI.
4. We should fix the cases where the xHCI driver dies, rather than
hacking around.
5. Switching the ports to EHCI when the xHCI host dies won't allow us to
switchover connected USB 3.0 devices.
6. When an xHCI host dies, the behavior should remain the same across
multiple systems. Having USB 2.0 devices switch back to EHCI when an
Intel xHCI host dies will confuse users who are comparing debugging
logs.
> I made some modification and verified it in my machine, it seems workable; for
> my modification, a rough patch based on it is also available at the bottom.
Again, thank you for the time you put into this patch and testing it. I
appreciate the effort.
Let me know if you have been running into the case where the xHCI host
is dying, so that I can attempt to reproduce it and fix it.
Sarah Sharp
> root@test-hsw-ult:~# lsusb -t
> /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M
> |__ Port 1: Dev 2, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
> /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/9p, 480M
> |__ Port 5: Dev 3, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
> |__ Port 6: Dev 4, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
> |__ Port 8: Dev 5, If 0, Class=Wireless, Driver=btusb, 12M
> |__ Port 8: Dev 5, If 1, Class=Wireless, Driver=btusb, 12M
> /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/8p, 480M
>
> root@test-hsw-ult:~# modprobe -r xhci_hcd /* without ports switching back
> here, all ports died. */
> root@test-hsw-ult:~# lsusb -t
> /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/8p, 480M
> |__ Port 1: Dev 10, If 0, Class=Mass Storage, Driver=usb-storage, 480M
> |__ Port 5: Dev 7, If 0, Class=Human Interface Device, Driver=usbhid,
> 1.5M
> |__ Port 6: Dev 8, If 0, Class=Human Interface Device, Driver=usbhid,
> 1.5M
> |__ Port 8: Dev 9, If 0, Class=Wireless, Driver=btusb, 12M
> |__ Port 8: Dev 9, If 1, Class=Wireless, Driver=btusb, 12M
>
> root@test-hsw-ult:~# modprobe xhci-hcd
> root@test-hsw-ult:~# lsusb -t
> /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 5000M
> |__ Port 1: Dev 2, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
> /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/9p, 480M
> |__ Port 5: Dev 4, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
> |__ Port 6: Dev 5, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
> |__ Port 8: Dev 6, If 0, Class=Wireless, Driver=btusb, 12M
> |__ Port 8: Dev 6, If 1, Class=Wireless, Driver=btusb, 12M
> /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/8p, 480M
>
> Thanks & Br,
>
> Lin
>
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 7296068..57075b4 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -150,11 +150,25 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
> */
> static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> {
> +#define USB_INTEL_USB3_PSSEN 0xD8
> + u32 pssen;
> int retval;
> struct xhci_hcd *xhci;
> struct hc_driver *driver;
> struct usb_hcd *hcd;
>
> + /*
> + * Switch ports from EHCI to xHCI when xHCI driver module loaded;
> ignore
> + * it if USB 3.0 port super speed enable register has been set during
> the
> + * PCI enumeration.
> + */
> + if (dev->vendor == PCI_VENDOR_ID_INTEL) {
> + /* USB_INTEL_XUSB2PR should be also checked here? */
> + pci_read_config_dword(dev, USB_INTEL_USB3_PSSEN, &pssen);
> + if (!pssen)
> + usb_enable_intel_xhci_ports(dev);
> + }
> +
> driver = (struct hc_driver *)id->driver_data;
> /* Register the USB 2.0 roothub.
> * FIXME: USB core must know to register the USB 2.0 roothub first.
> @@ -199,14 +213,25 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> struct pci_device_id *id)
> put_usb3_hcd:
> usb_put_hcd(xhci->shared_hcd);
> dealloc_usb2_hcd:
> + if (dev->vendor == PCI_VENDOR_ID_INTEL)
> + usb_disable_xhci_ports(dev);
> usb_hcd_pci_remove(dev);
> return retval;
> +#undef USB_INTEL_USB3_PSSEN
> }
>
> static void xhci_pci_remove(struct pci_dev *dev)
> {
> struct xhci_hcd *xhci;
>
> + /*
> + * For Intel product, if ports have been switched to xHCI during PCI
> enumeration or
> + * on driver module loading, it should be switched back to EHCI on
> module unloading
> + * while the system is still running, or all external ports will die.
> + */
> + if (dev->vendor == PCI_VENDOR_ID_INTEL)
> + usb_disable_xhci_ports(dev);
> +
> xhci = hcd_to_xhci(pci_get_drvdata(dev));
> if (xhci->shared_hcd) {
> usb_remove_hcd(xhci->shared_hcd);
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 5b08cd8..6182e0b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -64,6 +64,7 @@
> * endpoint rings; it generates events on the event ring for these.
> */
>
> +#include <linux/pci.h>
> #include <linux/scatterlist.h>
> #include <linux/slab.h>
> #include "xhci.h"
> @@ -363,6 +364,7 @@ int xhci_cancel_cmd(struct xhci_hcd *xhci, struct
> xhci_command *command,
> {
> int retval = 0;
> unsigned long flags;
> + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
>
> spin_lock_irqsave(&xhci->lock, flags);
>
> @@ -387,7 +389,12 @@ int xhci_cancel_cmd(struct xhci_hcd *xhci, struct
> xhci_command *command,
> if (unlikely(retval == -ESHUTDOWN)) {
> spin_unlock_irqrestore(&xhci->lock, flags);
> usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
> - xhci_dbg(xhci, "xHCI host controller is dead.\n");
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> + usb_disable_xhci_ports(pdev);
> + xhci_dbg(xhci, "xHCI host controller is dead,"
> + "switch ports back to EHCI.\n");
> + } else
> + xhci_dbg(xhci, "xHCI host controller is dead.\
> n");
> return retval;
> }
> }
> @@ -903,11 +910,13 @@ void xhci_stop_endpoint_command_watchdog(unsigned long
> arg)
> struct xhci_virt_ep *temp_ep;
> struct xhci_ring *ring;
> struct xhci_td *cur_td;
> + struct pci_dev *pdev;
> int ret, i, j;
> unsigned long flags;
>
> ep = (struct xhci_virt_ep *) arg;
> xhci = ep->xhci;
> + pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
>
> spin_lock_irqsave(&xhci->lock, flags);
>
> @@ -988,7 +997,12 @@ void xhci_stop_endpoint_command_watchdog(unsigned long
> arg)
> spin_unlock_irqrestore(&xhci->lock, flags);
> xhci_dbg(xhci, "Calling usb_hc_died()\n");
> usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
> - xhci_dbg(xhci, "xHCI host controller is dead.\n");
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> + usb_disable_xhci_ports(pdev);
> + xhci_dbg(xhci, "xHCI host controller is dead,"
> + "switch ports back to EHCI.\n");
> + } else
> + xhci_dbg(xhci, "xHCI host controller is dead.\n");
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
