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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut