| Message ID | 20200902000937.2204-1-tom.ty89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives | expand |
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@gmail.com> > --- > 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. > + > + 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. Alan Stern
On Wed, 2 Sep 2020 at 23:30, Alan Stern <stern@rowland.harvard.edu> 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@gmail.com> > > --- > > 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
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; + + 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) + blk_queue_max_hw_sectors(sdev->request_queue, 2048); + + 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;
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@gmail.com> --- drivers/usb/storage/uas.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)