* [PATCH v3] ehci: fix EHCI host controller initialization sequence
@ 2021-01-09 18:18 Eugene Korenevsky
2021-01-09 21:04 ` Alan Stern
0 siblings, 1 reply; 4+ messages in thread
From: Eugene Korenevsky @ 2021-01-09 18:18 UTC (permalink / raw)
To: linux-usb; +Cc: Alan Stern, Greg Kroah-Hartman
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.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=211095
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Eugene Korenevsky <ekorenevsky@astralinux.ru>
---
drivers/usb/host/ehci-hcd.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index e358ae17d51e..f3b73b5ab6e3 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -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));
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] ehci: fix EHCI host controller initialization sequence
2021-01-09 18:18 [PATCH v3] ehci: fix EHCI host controller initialization sequence Eugene Korenevsky
@ 2021-01-09 21:04 ` Alan Stern
2021-01-10 7:37 ` Eugene Korenevsky
0 siblings, 1 reply; 4+ messages in thread
From: Alan Stern @ 2021-01-09 21:04 UTC (permalink / raw)
To: Eugene Korenevsky; +Cc: linux-usb, Greg Kroah-Hartman
On Sat, Jan 09, 2021 at 09:18:28PM +0300, Eugene Korenevsky wrote:
> 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.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=211095
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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.
> Signed-off-by: Eugene Korenevsky <ekorenevsky@astralinux.ru>
> ---
> drivers/usb/host/ehci-hcd.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index e358ae17d51e..f3b73b5ab6e3 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -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);
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
> +
> 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));
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] ehci: fix EHCI host controller initialization sequence
2021-01-09 21:04 ` Alan Stern
@ 2021-01-10 7:37 ` Eugene Korenevsky
2021-01-10 16:44 ` Alan Stern
0 siblings, 1 reply; 4+ messages in thread
From: Eugene Korenevsky @ 2021-01-10 7:37 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, Greg Kroah-Hartman
> > - ehci_readl(ehci, &ehci->regs->command); /* unblock posted writes */
> 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.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] ehci: fix EHCI host controller initialization sequence
2021-01-10 7:37 ` Eugene Korenevsky
@ 2021-01-10 16:44 ` Alan Stern
0 siblings, 0 replies; 4+ messages in thread
From: Alan Stern @ 2021-01-10 16:44 UTC (permalink / raw)
To: Eugene Korenevsky; +Cc: linux-usb, Greg Kroah-Hartman
On Sun, Jan 10, 2021 at 10:37:37AM +0300, Eugene Korenevsky wrote:
> > > - ehci_readl(ehci, &ehci->regs->command); /* unblock posted writes */
>
> > 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.
>
> 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.
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-10 16:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-09 18:18 [PATCH v3] ehci: fix EHCI host controller initialization sequence Eugene Korenevsky
2021-01-09 21:04 ` Alan Stern
2021-01-10 7:37 ` Eugene Korenevsky
2021-01-10 16:44 ` Alan Stern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.