| Message ID | 20200322130031.10455-6-lukma@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | usb: Improve robustness of ehci-hcd controller operation | expand |
On 3/22/20 2:00 PM, Lukasz Majewski wrote: > This code adds check if QT_TOKEN_STATUS_XACTERR error occurred. When it is > detected the token is reconfigured and transmission is retried. > > This code is the port to newest U-Boot of the fix from - "rayvt" (from [1]). > > Links: > [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 > [2] - https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 > > > Signed-off-by: Lukasz Majewski <lukma at denx.de> > [Unfortunately, the original patch [2] did not contain S-o-B from the original > author - "rayvt"] > --- > > drivers/usb/host/ehci-hcd.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > index 0a77111f80..45eda7ad24 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -315,6 +315,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, > int timeout; > int ret = 0; > struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); > + int trynum; > > debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", dev, pipe, > buffer, length, req); > @@ -560,6 +561,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, > ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); > > /* Enable async. schedule. */ > + trynum = 1; /* No more than 2 tries, in case of XACTERR. */ > +retry_xacterr:; > + vtd = &qtd[qtd_counter - 1]; > + > cmd = ehci_readl(&ctrl->hcor->or_usbcmd); > cmd |= CMD_ASE; > ehci_writel(&ctrl->hcor->or_usbcmd, cmd); Are you sure always retrying the transfer if you get XACTERR is the right thing to do, even if you get that e.g. on filesystem writes ?
Hi Marek, > On 3/22/20 2:00 PM, Lukasz Majewski wrote: > > This code adds check if QT_TOKEN_STATUS_XACTERR error occurred. > > When it is detected the token is reconfigured and transmission is > > retried. > > > > This code is the port to newest U-Boot of the fix from - "rayvt" > > (from [1]). > > > > Links: > > [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 > > [2] - > > https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 > > > > > > Signed-off-by: Lukasz Majewski <lukma at denx.de> > > [Unfortunately, the original patch [2] did not contain S-o-B from > > the original author - "rayvt"] > > --- > > > > drivers/usb/host/ehci-hcd.c | 27 ++++++++++++++++++++++++++- > > 1 file changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/ehci-hcd.c > > b/drivers/usb/host/ehci-hcd.c index 0a77111f80..45eda7ad24 100644 > > --- a/drivers/usb/host/ehci-hcd.c > > +++ b/drivers/usb/host/ehci-hcd.c > > @@ -315,6 +315,7 @@ ehci_submit_async(struct usb_device *dev, > > unsigned long pipe, void *buffer, int timeout; > > int ret = 0; > > struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); > > + int trynum; > > > > debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", > > dev, pipe, buffer, length, req); > > @@ -560,6 +561,10 @@ ehci_submit_async(struct usb_device *dev, > > unsigned long pipe, void *buffer, > > ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); > > /* Enable async. schedule. */ > > + trynum = 1; /* No more than 2 tries, in case of > > XACTERR. */ +retry_xacterr:; > > + vtd = &qtd[qtd_counter - 1]; > > + > > cmd = ehci_readl(&ctrl->hcor->or_usbcmd); > > cmd |= CMD_ASE; > > ehci_writel(&ctrl->hcor->or_usbcmd, cmd); > > Are you sure always retrying the transfer if you get XACTERR is the > right thing to do, even if you get that e.g. on filesystem writes ? Please correct me if I'm wrong, but this function - ehci_submit_async - doesn't work with filesystem. It just setups proper EHCI descriptor and checks if it was sent with or without errors. When the XACTERR happens, proper flag is cleared and the transmission is retried. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200323/d6546ec7/attachment.sig>
On 3/23/20 8:18 AM, Lukasz Majewski wrote: > Hi Marek, Hi, >> On 3/22/20 2:00 PM, Lukasz Majewski wrote: >>> This code adds check if QT_TOKEN_STATUS_XACTERR error occurred. >>> When it is detected the token is reconfigured and transmission is >>> retried. >>> >>> This code is the port to newest U-Boot of the fix from - "rayvt" >>> (from [1]). >>> >>> Links: >>> [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 >>> [2] - >>> https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 >>> >>> >>> Signed-off-by: Lukasz Majewski <lukma at denx.de> >>> [Unfortunately, the original patch [2] did not contain S-o-B from >>> the original author - "rayvt"] >>> --- >>> >>> drivers/usb/host/ehci-hcd.c | 27 ++++++++++++++++++++++++++- >>> 1 file changed, 26 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/ehci-hcd.c >>> b/drivers/usb/host/ehci-hcd.c index 0a77111f80..45eda7ad24 100644 >>> --- a/drivers/usb/host/ehci-hcd.c >>> +++ b/drivers/usb/host/ehci-hcd.c >>> @@ -315,6 +315,7 @@ ehci_submit_async(struct usb_device *dev, >>> unsigned long pipe, void *buffer, int timeout; >>> int ret = 0; >>> struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); >>> + int trynum; >>> >>> debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", >>> dev, pipe, buffer, length, req); >>> @@ -560,6 +561,10 @@ ehci_submit_async(struct usb_device *dev, >>> unsigned long pipe, void *buffer, >>> ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); >>> /* Enable async. schedule. */ >>> + trynum = 1; /* No more than 2 tries, in case of >>> XACTERR. */ +retry_xacterr:; >>> + vtd = &qtd[qtd_counter - 1]; >>> + >>> cmd = ehci_readl(&ctrl->hcor->or_usbcmd); >>> cmd |= CMD_ASE; >>> ehci_writel(&ctrl->hcor->or_usbcmd, cmd); >> >> Are you sure always retrying the transfer if you get XACTERR is the >> right thing to do, even if you get that e.g. on filesystem writes ? > > Please correct me if I'm wrong, but this function - ehci_submit_async > - doesn't work with filesystem. It just setups proper EHCI descriptor > and checks if it was sent with or without errors. If you're writing into a block device, this function is used. If you get XACTERR during the transfer and you retry, is there a chance the data get actually written to the device ? > When the XACTERR happens, proper flag is cleared and the transmission > is retried. What happens to the payload if it's a host-to-device transfer, do the data get discarded or do they get written to the storage ?
Hi Marek, > On 3/23/20 8:18 AM, Lukasz Majewski wrote: > > Hi Marek, > > Hi, > > >> On 3/22/20 2:00 PM, Lukasz Majewski wrote: > >>> This code adds check if QT_TOKEN_STATUS_XACTERR error occurred. > >>> When it is detected the token is reconfigured and transmission is > >>> retried. > >>> > >>> This code is the port to newest U-Boot of the fix from - "rayvt" > >>> (from [1]). > >>> > >>> Links: > >>> [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 > >>> [2] - > >>> https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 > >>> > >>> > >>> Signed-off-by: Lukasz Majewski <lukma at denx.de> > >>> [Unfortunately, the original patch [2] did not contain S-o-B from > >>> the original author - "rayvt"] > >>> --- > >>> > >>> drivers/usb/host/ehci-hcd.c | 27 ++++++++++++++++++++++++++- > >>> 1 file changed, 26 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/usb/host/ehci-hcd.c > >>> b/drivers/usb/host/ehci-hcd.c index 0a77111f80..45eda7ad24 100644 > >>> --- a/drivers/usb/host/ehci-hcd.c > >>> +++ b/drivers/usb/host/ehci-hcd.c > >>> @@ -315,6 +315,7 @@ ehci_submit_async(struct usb_device *dev, > >>> unsigned long pipe, void *buffer, int timeout; > >>> int ret = 0; > >>> struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); > >>> + int trynum; > >>> > >>> debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", > >>> dev, pipe, buffer, length, req); > >>> @@ -560,6 +561,10 @@ ehci_submit_async(struct usb_device *dev, > >>> unsigned long pipe, void *buffer, > >>> ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); > >>> /* Enable async. schedule. */ > >>> + trynum = 1; /* No more than 2 tries, in case of > >>> XACTERR. */ +retry_xacterr:; > >>> + vtd = &qtd[qtd_counter - 1]; > >>> + > >>> cmd = ehci_readl(&ctrl->hcor->or_usbcmd); > >>> cmd |= CMD_ASE; > >>> ehci_writel(&ctrl->hcor->or_usbcmd, cmd); > >> > >> Are you sure always retrying the transfer if you get XACTERR is the > >> right thing to do, even if you get that e.g. on filesystem writes > >> ? > > > > Please correct me if I'm wrong, but this function - > > ehci_submit_async > > - doesn't work with filesystem. It just setups proper EHCI > > descriptor and checks if it was sent with or without errors. > > If you're writing into a block device, this function is used. If you > get XACTERR during the transfer and you retry, is there a chance the > data get actually written to the device ? As fair as I can tell this code snippet doesn't touch file systems. > > > When the XACTERR happens, proper flag is cleared and the > > transmission is retried. > > What happens to the payload if it's a host-to-device transfer, do the > data get discarded or do they get written to the storage ? > As state above - the resent shall only result is data being to some in-memory buffer. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200323/c11fbcea/attachment.sig>
On 3/23/20 1:58 PM, Lukasz Majewski wrote: > Hi Marek, Hi, >> On 3/23/20 8:18 AM, Lukasz Majewski wrote: >>> Hi Marek, >> >> Hi, >> >>>> On 3/22/20 2:00 PM, Lukasz Majewski wrote: >>>>> This code adds check if QT_TOKEN_STATUS_XACTERR error occurred. >>>>> When it is detected the token is reconfigured and transmission is >>>>> retried. >>>>> >>>>> This code is the port to newest U-Boot of the fix from - "rayvt" >>>>> (from [1]). >>>>> >>>>> Links: >>>>> [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 >>>>> [2] - >>>>> https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 >>>>> >>>>> >>>>> Signed-off-by: Lukasz Majewski <lukma at denx.de> >>>>> [Unfortunately, the original patch [2] did not contain S-o-B from >>>>> the original author - "rayvt"] >>>>> --- >>>>> >>>>> drivers/usb/host/ehci-hcd.c | 27 ++++++++++++++++++++++++++- >>>>> 1 file changed, 26 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/usb/host/ehci-hcd.c >>>>> b/drivers/usb/host/ehci-hcd.c index 0a77111f80..45eda7ad24 100644 >>>>> --- a/drivers/usb/host/ehci-hcd.c >>>>> +++ b/drivers/usb/host/ehci-hcd.c >>>>> @@ -315,6 +315,7 @@ ehci_submit_async(struct usb_device *dev, >>>>> unsigned long pipe, void *buffer, int timeout; >>>>> int ret = 0; >>>>> struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); >>>>> + int trynum; >>>>> >>>>> debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", >>>>> dev, pipe, buffer, length, req); >>>>> @@ -560,6 +561,10 @@ ehci_submit_async(struct usb_device *dev, >>>>> unsigned long pipe, void *buffer, >>>>> ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); >>>>> /* Enable async. schedule. */ >>>>> + trynum = 1; /* No more than 2 tries, in case of >>>>> XACTERR. */ +retry_xacterr:; >>>>> + vtd = &qtd[qtd_counter - 1]; >>>>> + >>>>> cmd = ehci_readl(&ctrl->hcor->or_usbcmd); >>>>> cmd |= CMD_ASE; >>>>> ehci_writel(&ctrl->hcor->or_usbcmd, cmd); >>>> >>>> Are you sure always retrying the transfer if you get XACTERR is the >>>> right thing to do, even if you get that e.g. on filesystem writes >>>> ? >>> >>> Please correct me if I'm wrong, but this function - >>> ehci_submit_async >>> - doesn't work with filesystem. It just setups proper EHCI >>> descriptor and checks if it was sent with or without errors. >> >> If you're writing into a block device, this function is used. If you >> get XACTERR during the transfer and you retry, is there a chance the >> data get actually written to the device ? > > As fair as I can tell this code snippet doesn't touch file systems. It could be any sort of write into block storage, it does not have to be filesystem. The filesystem was just an example of write type we trigger more often. >>> When the XACTERR happens, proper flag is cleared and the >>> transmission is retried. >> >> What happens to the payload if it's a host-to-device transfer, do the >> data get discarded or do they get written to the storage ? >> > > As state above - the resent shall only result is data being to some > in-memory buffer. If you trigger a write from device memory to remote storage, then it is the other way around.
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 0a77111f80..45eda7ad24 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -315,6 +315,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, int timeout; int ret = 0; struct ehci_ctrl *ctrl = ehci_get_ctrl(dev); + int trynum; debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", dev, pipe, buffer, length, req); @@ -560,6 +561,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, ehci_writel(&ctrl->hcor->or_usbsts, (usbsts & 0x3f)); /* Enable async. schedule. */ + trynum = 1; /* No more than 2 tries, in case of XACTERR. */ +retry_xacterr:; + vtd = &qtd[qtd_counter - 1]; + cmd = ehci_readl(&ctrl->hcor->or_usbcmd); cmd |= CMD_ASE; ehci_writel(&ctrl->hcor->or_usbcmd, cmd); @@ -573,8 +578,8 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, /* Wait for TDs to be processed. */ ts = get_timer(0); - vtd = &qtd[qtd_counter - 1]; timeout = USB_TIMEOUT_MS(pipe); + timeout += dev->extra_timeout; do { /* Invalidate dcache */ invalidate_dcache_range((unsigned long)&ctrl->qh_list, @@ -589,6 +594,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, break; WATCHDOG_RESET(); } while (get_timer(ts) < timeout); + debug("took %4lu ms of %4d\n", get_timer(ts), timeout); /* * Invalidate the memory area occupied by buffer @@ -622,6 +628,25 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, token = hc32_to_cpu(qh->qh_overlay.qt_token); if (!(QT_TOKEN_GET_STATUS(token) & QT_TOKEN_STATUS_ACTIVE)) { debug("TOKEN=%#x\n", token); + if (token & QT_TOKEN_STATUS_XACTERR) { + if (--trynum >= 0) { + /* + * It is necessary to do this, otherwise the + * disk is clagged. + */ + debug("reset the TD and redo, because of XACTERR\n"); + token &= ~QT_TOKEN_STATUS_HALTED; + token |= QT_TOKEN_STATUS_ACTIVE | + QT_TOKEN_CERR(2); + vtd->qt_token = cpu_to_hc32(token); + qh->qh_overlay.qt_token = cpu_to_hc32(token); + goto retry_xacterr; + } + dev->status = USB_ST_XACTERR; + dev->act_len = length - QT_TOKEN_GET_TOTALBYTES(token); + goto fail; + } + switch (QT_TOKEN_GET_STATUS(token) & ~(QT_TOKEN_STATUS_SPLITXSTATE | QT_TOKEN_STATUS_PERR)) { case 0:
This code adds check if QT_TOKEN_STATUS_XACTERR error occurred. When it is detected the token is reconfigured and transmission is retried. This code is the port to newest U-Boot of the fix from - "rayvt" (from [1]). Links: [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 [2] - https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 Signed-off-by: Lukasz Majewski <lukma at denx.de> [Unfortunately, the original patch [2] did not contain S-o-B from the original author - "rayvt"] --- drivers/usb/host/ehci-hcd.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)