[RFT PATCH v1 2/5] usb: Handle XACTERR error in DATA phase of USB storage

[Lukasz Majewski]

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>