| Message ID | 20200322130031.10455-3-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 change brings support for handling XACTERR error during DATA phase > of USB BBB (bulk) transmission. > > The fix is to clear stall'ed endpoint and reset recovery on the USB mass > storage class. > > 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"] > --- > > common/usb_storage.c | 10 ++++++++++ > include/usb_defs.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/common/usb_storage.c b/common/usb_storage.c > index 097b6729c1..92e1e54d1c 100644 > --- a/common/usb_storage.c > +++ b/common/usb_storage.c > @@ -740,6 +740,16 @@ static int usb_stor_BBB_transport(struct scsi_cmd *srb, struct us_data *us) > > result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, srb->datalen, > &data_actlen, USB_CNTL_TIMEOUT * 5); > + > + /* special handling of XACTERR in DATA phase */ > + if (result < 0 && (us->pusb_dev->status & USB_ST_XACTERR)) { > + debug("XACTERR in data phase - clr, reset, and return fail.\n"); > + usb_stor_BBB_clear_endpt_stall(us, dir_in ? > + us->ep_in : us->ep_out); > + usb_stor_BBB_reset(us); > + return USB_STOR_TRANSPORT_FAILED; > + } > + Can resetting the endpoint result in data corruption of some sort here ? Especially if this happens on filesystem write for example ?
Hi Marek, > On 3/22/20 2:00 PM, Lukasz Majewski wrote: > > This change brings support for handling XACTERR error during DATA > > phase of USB BBB (bulk) transmission. > > > > The fix is to clear stall'ed endpoint and reset recovery on the USB > > mass storage class. > > > > 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"] > > --- > > > > common/usb_storage.c | 10 ++++++++++ > > include/usb_defs.h | 1 + > > 2 files changed, 11 insertions(+) > > > > diff --git a/common/usb_storage.c b/common/usb_storage.c > > index 097b6729c1..92e1e54d1c 100644 > > --- a/common/usb_storage.c > > +++ b/common/usb_storage.c > > @@ -740,6 +740,16 @@ static int usb_stor_BBB_transport(struct > > scsi_cmd *srb, struct us_data *us) > > result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, > > srb->datalen, &data_actlen, USB_CNTL_TIMEOUT * 5); > > + > > + /* special handling of XACTERR in DATA phase */ > > + if (result < 0 && (us->pusb_dev->status & USB_ST_XACTERR)) > > { > > + debug("XACTERR in data phase - clr, reset, and > > return fail.\n"); > > + usb_stor_BBB_clear_endpt_stall(us, dir_in ? > > + us->ep_in : > > us->ep_out); > > + usb_stor_BBB_reset(us); > > + return USB_STOR_TRANSPORT_FAILED; > > + } > > + > > Can resetting the endpoint result in data corruption of some sort > here ? Please correct me if I'm wrong, but this code is executed when we do receive data, not writing them. Those operations shall (and are?) orthogonal. > Especially if this happens on filesystem write for example ? Do we write data here? 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/fe801a2d/attachment.sig>
On 3/23/20 8:00 AM, Lukasz Majewski wrote: > Hi Marek, > >> On 3/22/20 2:00 PM, Lukasz Majewski wrote: >>> This change brings support for handling XACTERR error during DATA >>> phase of USB BBB (bulk) transmission. >>> >>> The fix is to clear stall'ed endpoint and reset recovery on the USB >>> mass storage class. >>> >>> 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"] >>> --- >>> >>> common/usb_storage.c | 10 ++++++++++ >>> include/usb_defs.h | 1 + >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/common/usb_storage.c b/common/usb_storage.c >>> index 097b6729c1..92e1e54d1c 100644 >>> --- a/common/usb_storage.c >>> +++ b/common/usb_storage.c >>> @@ -740,6 +740,16 @@ static int usb_stor_BBB_transport(struct >>> scsi_cmd *srb, struct us_data *us) >>> result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, >>> srb->datalen, &data_actlen, USB_CNTL_TIMEOUT * 5); >>> + >>> + /* special handling of XACTERR in DATA phase */ >>> + if (result < 0 && (us->pusb_dev->status & USB_ST_XACTERR)) >>> { >>> + debug("XACTERR in data phase - clr, reset, and >>> return fail.\n"); >>> + usb_stor_BBB_clear_endpt_stall(us, dir_in ? >>> + us->ep_in : >>> us->ep_out); >>> + usb_stor_BBB_reset(us); >>> + return USB_STOR_TRANSPORT_FAILED; >>> + } >>> + >> >> Can resetting the endpoint result in data corruption of some sort >> here ? > > Please correct me if I'm wrong, but this code is executed when we do > receive data, not writing them. Those operations shall (and are?) > orthogonal. > >> Especially if this happens on filesystem write for example ? > > Do we write data here? I only did a very quick look into the code, but there I see 1082 static int usb_write_10(struct scsi_cmd *srb, struct us_data *ss, ... 1096 return ss->transport(srb, ss); 1338 case US_PR_BULK: 1339 debug("Bulk/Bulk/Bulk\n"); 1340 ss->transport = usb_stor_BBB_transport; So I would say, the answer is -- yes.
Hi Marek, > On 3/23/20 8:00 AM, Lukasz Majewski wrote: > > Hi Marek, > > > >> On 3/22/20 2:00 PM, Lukasz Majewski wrote: > >>> This change brings support for handling XACTERR error during DATA > >>> phase of USB BBB (bulk) transmission. > >>> > >>> The fix is to clear stall'ed endpoint and reset recovery on the > >>> USB mass storage class. > >>> > >>> 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"] > >>> --- > >>> > >>> common/usb_storage.c | 10 ++++++++++ > >>> include/usb_defs.h | 1 + > >>> 2 files changed, 11 insertions(+) > >>> > >>> diff --git a/common/usb_storage.c b/common/usb_storage.c > >>> index 097b6729c1..92e1e54d1c 100644 > >>> --- a/common/usb_storage.c > >>> +++ b/common/usb_storage.c > >>> @@ -740,6 +740,16 @@ static int usb_stor_BBB_transport(struct > >>> scsi_cmd *srb, struct us_data *us) > >>> result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, > >>> srb->datalen, &data_actlen, USB_CNTL_TIMEOUT * 5); > >>> + > >>> + /* special handling of XACTERR in DATA phase */ > >>> + if (result < 0 && (us->pusb_dev->status & > >>> USB_ST_XACTERR)) { > >>> + debug("XACTERR in data phase - clr, reset, and > >>> return fail.\n"); > >>> + usb_stor_BBB_clear_endpt_stall(us, dir_in ? > >>> + us->ep_in : > >>> us->ep_out); > >>> + usb_stor_BBB_reset(us); > >>> + return USB_STOR_TRANSPORT_FAILED; > >>> + } > >>> + > >> > >> Can resetting the endpoint result in data corruption of some sort > >> here ? > > > > Please correct me if I'm wrong, but this code is executed when we do > > receive data, not writing them. Those operations shall (and are?) > > orthogonal. > > > >> Especially if this happens on filesystem write for example ? > > > > Do we write data here? > > I only did a very quick look into the code, but there I see > > 1082 static int usb_write_10(struct scsi_cmd *srb, struct us_data *ss, > ... > 1096 return ss->transport(srb, ss); > > 1338 case US_PR_BULK: > 1339 debug("Bulk/Bulk/Bulk\n"); > 1340 ss->transport = usb_stor_BBB_transport; > > So I would say, the answer is -- yes. > A few lines down (usb_storage.c @ L757) you have the USB_ST_STALLED status handled in the same way (it also aborts after a single retry). 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/ddac4d0d/attachment.sig>
On 3/23/20 2:03 PM, Lukasz Majewski wrote: > Hi Marek, Hi, >> On 3/23/20 8:00 AM, Lukasz Majewski wrote: >>> Hi Marek, >>> >>>> On 3/22/20 2:00 PM, Lukasz Majewski wrote: >>>>> This change brings support for handling XACTERR error during DATA >>>>> phase of USB BBB (bulk) transmission. >>>>> >>>>> The fix is to clear stall'ed endpoint and reset recovery on the >>>>> USB mass storage class. >>>>> >>>>> 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"] >>>>> --- >>>>> >>>>> common/usb_storage.c | 10 ++++++++++ >>>>> include/usb_defs.h | 1 + >>>>> 2 files changed, 11 insertions(+) >>>>> >>>>> diff --git a/common/usb_storage.c b/common/usb_storage.c >>>>> index 097b6729c1..92e1e54d1c 100644 >>>>> --- a/common/usb_storage.c >>>>> +++ b/common/usb_storage.c >>>>> @@ -740,6 +740,16 @@ static int usb_stor_BBB_transport(struct >>>>> scsi_cmd *srb, struct us_data *us) >>>>> result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, >>>>> srb->datalen, &data_actlen, USB_CNTL_TIMEOUT * 5); >>>>> + >>>>> + /* special handling of XACTERR in DATA phase */ >>>>> + if (result < 0 && (us->pusb_dev->status & >>>>> USB_ST_XACTERR)) { >>>>> + debug("XACTERR in data phase - clr, reset, and >>>>> return fail.\n"); >>>>> + usb_stor_BBB_clear_endpt_stall(us, dir_in ? >>>>> + us->ep_in : >>>>> us->ep_out); >>>>> + usb_stor_BBB_reset(us); >>>>> + return USB_STOR_TRANSPORT_FAILED; >>>>> + } >>>>> + >>>> >>>> Can resetting the endpoint result in data corruption of some sort >>>> here ? >>> >>> Please correct me if I'm wrong, but this code is executed when we do >>> receive data, not writing them. Those operations shall (and are?) >>> orthogonal. >>> >>>> Especially if this happens on filesystem write for example ? >>> >>> Do we write data here? >> >> I only did a very quick look into the code, but there I see >> >> 1082 static int usb_write_10(struct scsi_cmd *srb, struct us_data *ss, >> ... >> 1096 return ss->transport(srb, ss); >> >> 1338 case US_PR_BULK: >> 1339 debug("Bulk/Bulk/Bulk\n"); >> 1340 ss->transport = usb_stor_BBB_transport; >> >> So I would say, the answer is -- yes. >> > > A few lines down (usb_storage.c @ L757) you have the USB_ST_STALLED > status handled in the same way (it also aborts after a single retry). But stall isn't xfer error. Which makes me wonder, why is the xfer error here handled the same way as a stall, shouldn't the handling be different ? Also, this doesn't answer the question whether such handling can cause data corruption during write.
diff --git a/common/usb_storage.c b/common/usb_storage.c index 097b6729c1..92e1e54d1c 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -740,6 +740,16 @@ static int usb_stor_BBB_transport(struct scsi_cmd *srb, struct us_data *us) result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, srb->datalen, &data_actlen, USB_CNTL_TIMEOUT * 5); + + /* special handling of XACTERR in DATA phase */ + if (result < 0 && (us->pusb_dev->status & USB_ST_XACTERR)) { + debug("XACTERR in data phase - clr, reset, and return fail.\n"); + usb_stor_BBB_clear_endpt_stall(us, dir_in ? + us->ep_in : us->ep_out); + usb_stor_BBB_reset(us); + return USB_STOR_TRANSPORT_FAILED; + } + /* special handling of STALL in DATA phase */ if ((result < 0) && (us->pusb_dev->status & USB_ST_STALLED)) { debug("DATA:stall\n"); diff --git a/include/usb_defs.h b/include/usb_defs.h index 6dd2c997f9..572f6ab296 100644 --- a/include/usb_defs.h +++ b/include/usb_defs.h @@ -197,6 +197,7 @@ #define USB_ST_NAK_REC 0x10 /* NAK Received*/ #define USB_ST_CRC_ERR 0x20 /* CRC/timeout Error */ #define USB_ST_BIT_ERR 0x40 /* Bitstuff error */ +#define USB_ST_XACTERR 0x80 /* XACTERR error */ #define USB_ST_NOT_PROC 0x80000000L /* Not yet processed */
This change brings support for handling XACTERR error during DATA phase of USB BBB (bulk) transmission. The fix is to clear stall'ed endpoint and reset recovery on the USB mass storage class. 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"] --- common/usb_storage.c | 10 ++++++++++ include/usb_defs.h | 1 + 2 files changed, 11 insertions(+)