On Tue, 1 Sep 2020 at 22:55, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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?
That won't really help unless we also do the clamping on the USB side
*conditionally* (i.e. only after when we call
`blk_queue_max_hw_sectors()` again there). I'm not sure there's a good
reason for doing so.
Although this might seem a bit redundant/clumsy at first glance, it's
also much easier to follow (e.g. we don't have to dive into the SCSI
side to know where the hw_max_sectors came from).
Besides, the whole DMA thing is quite beyond my knowledge.
I'm going to fix the commit message, and use 2048 blocks for SS UAS drives.
>
> Alan Stern
>
> > Signed-off-by: Tom Yan <tom.ty89@xxxxxxxxx>
> > ---
> > 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
> >
