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

[Marek Vasut]

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.