On Wed, 2 Sep 2020 at 23:30, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Sep 02, 2020 at 08:09:36AM +0800, Tom Yan wrote:
> > There's no reason for uas to use a smaller value of max_sectors than
> > usb-storage.
> >
> > Also copying the dma max mapping size clamping from usb-storage.
> >
> > Signed-off-by: Tom Yan <tom.ty89@xxxxxxxxx>
> > ---
> > drivers/usb/storage/uas.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> > index 08f9296431e9..813c49914b9a 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,20 @@ 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;
>
> Yeah, this won't work. The uas driver doesn't use struct us_data at
> all. Instead you should have:
>
> struct device *dev = devinfo->udev->bus->sysdev;
>
> except that now you probably don't need it.
>
> > +
> > + 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 if (us->pusb_dev->speed >= USB_SPEED_SUPER)
>
> Same thing here: devinfo->udev->speed.
>
> > + blk_queue_max_hw_sectors(sdev->request_queue, 2048);
>
> Also, you might want to check before doing this. If it would decrease
> the max_hw_sectors value, there's no point doing it.
It wouldn't. Current SCSI_DEFAULT_MAX_SECTORS (which is used for
"normal" uas drives) is 1024.
I'm not entirely sure if there's a point in doing more than that,
although BLK_DEF_MAX_SECTORS is 2560 (which originates from an odd
context: https://github.com/torvalds/linux/commit/d2be537c3ba3568acd79cd178327b842e60d035e).
It just seems odd to me that we do less for SS UAS drives anyway.
(Maybe it was never necessary to go up to 2048 for SS BOT drives, I
don't know.)
>
> > +
> > + 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));
>
> And presumably this will be unnecessary.
It will still be. Whenever we change hw_max_sectors, it needs to be
clamped again afterwards, because the original clamping would be
invalidated.
>
> Alan Stern
