Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping

[Alan Stern <stern@rowland.harvard.edu>]

Patch submissions should have text lines limited to fewer than 80 
columns.

On Tue, Sep 01, 2020 at 01:54:17PM +0800, Tom Yan wrote:
> When the scsi request queue is initialized/allocated, the scsi driver 
> clamps hw_max_sectors against the dma max mapping size of 
> sdev->host->dma_dev. The clamping is apparently inappriorate to USB 
> drives.

Wouldn't it be more accurate to say that the clamping _is_ appropriate, 
but it should be performed using the sysdev device rather than the 
nominal parent?  Thus the error lies in allowing shost->dma_dev to be 
set incorrectly.

> Either way we are calling blk_queue_max_hw_sectors() in the usb 
> drivers for some (but not all) cases, which causes the clamping to be 
> overriden (inconsistently) anyway.
> 
> Therefore the usb driver should always set hw_max_sectors and do the 
> clamping against the right device itself.

How about fixing the dma_dev assignment instead?

Alan Stern

> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
>  drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
>  drivers/usb/storage/uas.c      | 23 ++++++++++++++++-----
>  2 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index e5a971b83e3f..804cbc0ba4da 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev)
>  		 * better throughput on most devices.
>  		 */
>  		blk_queue_max_hw_sectors(sdev->request_queue, 2048);
> +	} else {
> +	    /*
> +	     * Some devices are known to choke with anything larger. It seems like
> +	     * the problem stems from the fact that original IDE controllers had
> +	     * only an 8-bit register to hold the number of sectors in one transfer
> +	     * and even those couldn't handle a full 256 sectors.
> +	     *
> +	     * Because we want to make sure we interoperate with as many devices as
> +	     * possible, we will maintain a 240 sector transfer size limit for USB
> +	     * Mass Storage devices.
> +	     *
> +	     * Tests show that other operating have similar limits with Microsoft
> +	     * Windows 7 limiting transfers to 128 sectors for both USB2 and USB3
> +	     * and Apple Mac OS X 10.11 limiting transfers to 256 sectors for USB2
> +	     * and 2048 for USB3 devices.
> +	     */
> +		blk_queue_max_hw_sectors(sdev->request_queue, 240);
>  	}
>  
>  	/*
> @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = {
>  	/* lots of sg segments can be handled */
>  	.sg_tablesize =			SG_MAX_SEGMENTS,
>  
> -
> -	/*
> -	 * Limit the total size of a transfer to 120 KB.
> -	 *
> -	 * Some devices are known to choke with anything larger. It seems like
> -	 * the problem stems from the fact that original IDE controllers had
> -	 * only an 8-bit register to hold the number of sectors in one transfer
> -	 * and even those couldn't handle a full 256 sectors.
> -	 *
> -	 * Because we want to make sure we interoperate with as many devices as
> -	 * possible, we will maintain a 240 sector transfer size limit for USB
> -	 * Mass Storage devices.
> -	 *
> -	 * Tests show that other operating have similar limits with Microsoft
> -	 * Windows 7 limiting transfers to 128 sectors for both USB2 and USB3
> -	 * and Apple Mac OS X 10.11 limiting transfers to 256 sectors for USB2
> -	 * and 2048 for USB3 devices.
> -	 */
> -	.max_sectors =                  240,
> -
>  	/* emulated HBA */
>  	.emulated =			1,
>  
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 08f9296431e9..cffa435afd84 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev)
>  	 */
>  	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>  
> -	if (devinfo->flags & US_FL_MAX_SECTORS_64)
> -		blk_queue_max_hw_sectors(sdev->request_queue, 64);
> -	else if (devinfo->flags & US_FL_MAX_SECTORS_240)
> -		blk_queue_max_hw_sectors(sdev->request_queue, 240);
> -
>  	return 0;
>  }
>  
> @@ -839,6 +834,24 @@ static int uas_slave_configure(struct scsi_device *sdev)
>  {
>  	struct uas_dev_info *devinfo = sdev->hostdata;
>  
> +	struct us_data *us = host_to_us(sdev->host);
> +	struct device *dev = us->pusb_dev->bus->sysdev;
> +
> +	if (devinfo->flags & US_FL_MAX_SECTORS_64)
> +		blk_queue_max_hw_sectors(sdev->request_queue, 64);
> +	else if (devinfo->flags & US_FL_MAX_SECTORS_240)
> +		blk_queue_max_hw_sectors(sdev->request_queue, 240);
> +	else
> +		blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS);
> +
> +	/*
> +	 * The max_hw_sectors should be up to maximum size of a mapping for
> +	 * the device. Otherwise, a DMA API might fail on swiotlb environment.
> +	 */
> +	blk_queue_max_hw_sectors(sdev->request_queue,
> +		min_t(size_t, queue_max_hw_sectors(sdev->request_queue),
> +		      dma_max_mapping_size(dev) >> SECTOR_SHIFT));
> +
>  	if (devinfo->flags & US_FL_NO_REPORT_OPCODES)
>  		sdev->no_report_opcodes = 1;
>  
> -- 
> 2.28.0
>