ehci: fix EHCI host controller initialization sequence
Subsystems (2)
PATCH v3 1/1 ehci: fix EHCI host controller initialization sequence
According to EHCI spec, EHCI HC clears USBSTS.HCHalted whenever USBCMD.RS=1. However, it is a good practice to wait some time after setting USBCMD.RS (approximately 100ms) until USBSTS.HCHalted become zero.
Instead, previous version of ehci_run() wrote 1 to USBCMD.RS, issued read to USBCMD and waited for 5 ms.
That worked incorrectly at least for VirtualBox's EHCI virtual HC and caused accidental hangs.
--- drivers/usb/host/ehci-hcd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/usb/host/ehci-hcd.c +12 -1
@@ -574,6 +574,7 @@ static int ehci_run (struct usb_hcd *hcd)struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 temp;u32 hcc_params; +int rc; hcd->uses_new_polling =1 ;
@@ -629,9 +630,19 @@ static int ehci_run (struct usb_hcd *hcd)down_write (&ehci_cf_port_reset_rwsem); ehci->rh_state =EHCI_RH_RUNNING ;ehci_writel (ehci,FLAG_CF , &ehci->regs->configured_flag); -ehci_readl (ehci, &ehci->regs->command);/* unblock posted writes */ + +/* Wait until HC become operational */ msleep (5 ); + rc =ehci_handshake (ehci, &ehci->regs->status,STS_HALT ,0 ,100 *1000 ); +up_write (&ehci_cf_port_reset_rwsem); + +if (rc) { +ehci_err (ehci,"USB %x.%x, controller refused to start: %d\n" , + ((ehci->sbrn &0xf0 )>>4 ), (ehci->sbrn &0x0f ), rc); +return rc; + } + ehci->last_periodic_enable =ktime_get_real (); temp =HC_VERSION (ehci,ehci_readl (ehci, &ehci->caps->hc_capbase));
Signature · 1 line
2.20.1
Alan Stern
maintainerQuoted 4 lines · Eugene Korenevsky
According to EHCI spec, EHCI HC clears USBSTS.HCHalted whenever USBCMD.RS=1. However, it is a good practice to wait some time after setting USBCMD.RS (approximately 100ms) until USBSTS.HCHalted become zero.
Quoted 8 lines
Instead, previous version of
ehci_run()wrote 1 to USBCMD.RS, issued read to USBCMD and waited for 5 ms.
That worked incorrectly at least for VirtualBox's EHCI virtual HC and caused accidental hangs.Reviewed-by Alan Stern <[email protected]>Reviewed-by Greg Kroah-Hartman <[email protected]>
This is a big no-no! You must not add other people's Reviewed-by or Acked-by tags to a patch unless they give you explicit permission to do so.
Quoted 18 lines
Signed-off-by Eugene Korenevsky <[email protected]>--- drivers/usb/host/ehci-hcd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)drivers/usb/host/ehci-hcd.c +1
-0@@ -574,6 +574,7 @@ static int ehci_run (struct usb_hcd *hcd)struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 temp;u32 hcc_params; +int rc; hcd->uses_new_polling =1 ;
drivers/usb/host/ehci-hcd.c static int ehci_run (struct usb_hcd *hcd) · line 636
Quoted 8 lines
drivers/usb/host/ehci-hcd.c +3
-1@@ -629,9 +630,19 @@ static int ehci_run (struct usb_hcd *hcd)down_write (&ehci_cf_port_reset_rwsem); ehci->rh_state =EHCI_RH_RUNNING ;ehci_writel (ehci,FLAG_CF , &ehci->regs->configured_flag); -ehci_readl (ehci, &ehci->regs->command);/* unblock posted writes */ + +/* Wait until HC become operational */ msleep (5 ); + rc =ehci_handshake (ehci, &ehci->regs->status,STS_HALT ,0 ,100 *1000 );
You should not remove the ehci_readl call above. With that line gone, the PCI bus might not send the new value of configured_flag to the controller until after the 5-ms sleep has ended, which makes the msleep useless.
Alan Stern
Quoted 15 lines
drivers/usb/host/ehci-hcd.c +8
-0+up_write (&ehci_cf_port_reset_rwsem); + +if (rc) { +ehci_err (ehci,"USB %x.%x, controller refused to start: %d\n" , + ((ehci->sbrn &0xf0 )>>4 ), (ehci->sbrn &0x0f ), rc); +return rc; + } + ehci->last_periodic_enable =ktime_get_real (); temp =HC_VERSION (ehci,ehci_readl (ehci, &ehci->caps->hc_capbase));Signature · 1 line
2.20.1
Quoted 6 lines
drivers/usb/host/ehci-hcd.c static int ehci_run (struct usb_hcd *hcd) · line 632
Quoted 1 line
drivers/usb/host/ehci-hcd.c +0
-1-ehci_readl (ehci, &ehci->regs->command);/* unblock posted writes */ You should not remove the
ehci_readlcall above. With that line gone, the PCI bus might not send the new value ofconfigured_flagto the controller until after the 5-ms sleep has ended, which makes the msleep useless.
Could not find reads from USBCMD in similar drivers (for
example here: https://github.com/NetBSD/src/blob/trunk/sys/dev/usb/ehci.c#L625).
Is this feature (reading from USBCMD for unblocking posted writes) documented anywhere or it is found empirically?
--
Eugene
Alan Stern
maintainerQuoted 9 lines · Eugene Korenevsky
Quoted 6 lines
drivers/usb/host/ehci-hcd.c static int ehci_run (struct usb_hcd *hcd) · line 632
Quoted 1 line
drivers/usb/host/ehci-hcd.c +0
-1-ehci_readl (ehci, &ehci->regs->command);/* unblock posted writes */ You should not remove the
ehci_readlcall above. With that line gone, the PCI bus might not send the new value ofconfigured_flagto the controller until after the 5-ms sleep has ended, which makes the msleep useless.Could not find reads from USBCMD in similar drivers (for
example here: https://github.com/NetBSD/src/blob/trunk/sys/dev/usb/ehci.c#L625).
I don't know why NetBSD doesn't do this. However, note that the following poll loop does a read every millisecond, so the first time through the loop, the earlier writes will be unblocked.
Quoted 2 lines
Is this feature (reading from USBCMD for unblocking posted writes) documented anywhere or it is found empirically?
USBCMD is nothing special; it's just a convenient register to read. Any other register in the controller would do just as well.
This is part of the PCI spec. Writes are posted (meaning they aren't delivered to the device right away), but a read will cause all previously posted writes for any address on the same device to be delivered before the read finishes.
Alan Stern