[RFT PATCH v1 4/5] usb: Provide code to handle spinup of USB usb devices (mostly HDDs)

[Marek Vasut]

On 3/23/20 8:53 AM, Lukasz Majewski wrote:
> Hi Marek,

Hi,

>> On 3/22/20 2:00 PM, Lukasz Majewski wrote:
>> [...]
>>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>>> index 92e1e54d1c..3c2324fa1a 100644
>>> --- a/common/usb_storage.c
>>> +++ b/common/usb_storage.c
>>> @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct
>>> scsi_cmd *srb, struct us_data *us) pipeout =
>>> usb_sndbulkpipe(us->pusb_dev, us->ep_out); /* DATA phase + error
>>> handling */ data_actlen = 0;
>>> +	mdelay(10);		/* Like linux does. */  
>>
>> Does this add delay to every single transfer ?
> 
> It brings the slowdown, yes.
> 
> However, without it I very often see the error that the USB address
> couldn't be assigned.

Seems like this is hiding some real error then.

If I do basic math, then I reach a conclusion that the comment is bogus.
Look, assume the transfer itself takes 0 time, then if you have 10 mS
delays between transfers, you can do 100 transfer per second. If one
transfer is 240 blocks * 512 bytes , then you are limited to 12.2 MB/s.
And I am positive USB 2.0 sticks in Linux can transfer faster than that.

>> That would mean a massive slowdown if you use short data transfers,
>> which is needed for old/limited USB sticks.
>>
>>>  	/* no data, go immediately to the STATUS phase */
>>>  	if (srb->datalen == 0)
>>>  		goto st;
>>> @@ -1023,9 +1024,32 @@ static int usb_request_sense(struct scsi_cmd
>>> *srb, struct us_data *ss) return 0;
>>>  }
>>>  
>>> +/*
>>> + * This spins up the disk and also consumes the time that the
>>> + * disk takes to become active and ready to read data.
>>> + * Some drives (like Western Digital) can take more than 5 seconds.
>>> + * The delay occurs on the 1st data read from the disk.
>>> + * Extending the timeout here works better than handling the
>>> timeout
>>> + * as an error on a "real" read.
>>> + */
>>> +static int usb_spinup(struct scsi_cmd *srb, struct us_data *ss)
>>> +{
>>> +	memset(&srb->cmd[0], 0, 12);
>>> +	srb->cmd[0] = SCSI_START_STP;
>>> +	srb->cmd[1] = srb->lun << 5;
>>> +	srb->cmd[4] = 1;	/* Start spinup. */
>>> +	srb->datalen = 0;
>>> +	srb->cmdlen = 6;
>>> +	ss->pusb_dev->extra_timeout = 9876;  
>>
>> What is this magic number ?
> 
> This number is added to the timeout up to which ehci controller waits
> for EHCI TD to be sent.

This is generic code and has to work with OHCI/UHCI/xHCI too.

> Why there is 9876 - I do guess that it was took from Linux in some
> point in time.

Please, research it.

>>> +	ss->transport(srb, ss);
>>> +	ss->pusb_dev->extra_timeout = 0;
>>> +	return 0;
>>> +}  
>>
>> [...]
>>
>>> diff --git a/include/usb.h b/include/usb.h
>>> index efb67ea33f..5b0f5ce5d6 100644
>>> --- a/include/usb.h
>>> +++ b/include/usb.h
>>> @@ -140,6 +140,7 @@ struct usb_device {
>>>  	int act_len;			/* transferred bytes */
>>>  	int maxchild;			/* Number of ports if
>>> hub */ int portnr;			/* Port number, 1=first */
>>> +	int extra_timeout; /* Add to timeout in ehci_submit_async
>>> or spinup */  
>>
>> Does this work with xhci too ?
> 
> Yes, it is used in patch 5/5.

Does xhci need it ?

-- 
Best regards,
Marek Vasut