Oh I think I see your point now. If/When dma_dev is fixed / the same
as `bus->sysdev`, it wouldn't hurt / matter if the clamping is done
twice at different stages. max_sectors will be clamped to the same
value no matter if it's set on the SCSI side or the USB side.
But again, fixing the dma_dev is beyond my knowledge. Probably need
someone else to look into it.
On Wed, 2 Sep 2020 at 07:24, Tom Yan <tom.ty89@xxxxxxxxx> wrote:
>
> 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
> > >
