* [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
@ 2020-09-01 5:54 Tom Yan
2020-09-01 14:55 ` Alan Stern
0 siblings, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-01 5:54 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, Tom Yan
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.
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.
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
2020-09-01 5:54 [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan
@ 2020-09-01 14:55 ` Alan Stern
2020-09-01 23:24 ` Tom Yan
2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
0 siblings, 2 replies; 24+ messages in thread
From: Alan Stern @ 2020-09-01 14:55 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, Yoshihiro Shimoda
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
2020-09-01 14:55 ` Alan Stern
@ 2020-09-01 23:24 ` Tom Yan
2020-09-01 23:44 ` Tom Yan
2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
1 sibling, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-01 23:24 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda
On Tue, 1 Sep 2020 at 22:55, Alan Stern <stern@rowland.harvard.edu> 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@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
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
2020-09-01 23:24 ` Tom Yan
@ 2020-09-01 23:44 ` Tom Yan
2020-09-02 15:24 ` Alan Stern
0 siblings, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-01 23:44 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda
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@gmail.com> wrote:
>
> On Tue, 1 Sep 2020 at 22:55, Alan Stern <stern@rowland.harvard.edu> 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@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
> > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-01 14:55 ` Alan Stern
2020-09-01 23:24 ` Tom Yan
@ 2020-09-02 0:09 ` Tom Yan
2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan
2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern
1 sibling, 2 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-02 0:09 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
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;
+
+ 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;
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
@ 2020-09-02 0:09 ` Tom Yan
2020-09-02 0:20 ` Tom Yan
2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern
1 sibling, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-02 0:09 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
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
device is currently inappriorate to use for USB drives.
Therefore, always (re)set hw_max_sectors in the usb drivers to invalidate the
clamping.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
drivers/usb/storage/uas.c | 2 ++
2 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index e5a971b83e3f..1f60d777a7e8 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 813c49914b9a..592e1358822e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -843,6 +843,8 @@ static int uas_slave_configure(struct scsi_device *sdev)
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);
+ else
+ blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS);
blk_queue_max_hw_sectors(sdev->request_queue,
min_t(size_t, queue_max_hw_sectors(sdev->request_queue),
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan
@ 2020-09-02 0:20 ` Tom Yan
0 siblings, 0 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-02 0:20 UTC (permalink / raw)
To: linux-usb, gregkh, Alan Stern, Arnd Bergmann; +Cc: cyrozap, Yoshihiro Shimoda
I split it into two patches:
1. bump hw_max_sectors to 2048 for SS UAS drives and do the clamping
for all UAS drives
2. make sure there's a "fallback" hw_max_sectors "change" so that the
clamping on the SCSI side is invalidated.
If anyone can help fix the dma_dev, the second one can be omitted
(better make the clamping on the USB side to use shost->dev->dma_dev
though).
On Wed, 2 Sep 2020 at 08:09, Tom Yan <tom.ty89@gmail.com> 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
> device is currently inappriorate to use for USB drives.
>
> Therefore, always (re)set hw_max_sectors in the usb drivers to invalidate the
> clamping.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
> drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
> drivers/usb/storage/uas.c | 2 ++
> 2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index e5a971b83e3f..1f60d777a7e8 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 813c49914b9a..592e1358822e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -843,6 +843,8 @@ static int uas_slave_configure(struct scsi_device *sdev)
> 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);
> + else
> + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS);
>
> blk_queue_max_hw_sectors(sdev->request_queue,
> min_t(size_t, queue_max_hw_sectors(sdev->request_queue),
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping
2020-09-01 23:44 ` Tom Yan
@ 2020-09-02 15:24 ` Alan Stern
0 siblings, 0 replies; 24+ messages in thread
From: Alan Stern @ 2020-09-02 15:24 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda
On Wed, Sep 02, 2020 at 07:44:42AM +0800, Tom Yan wrote:
> 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.
It's straightforward. All you have to do is convert the calls to
scsi_add_host() in usb.c and uas.c. Make them call
scsi_add_host_with_dma() instead. The extra third argument should be
a pointer to the sysdev device.
Once you do this, presumably your second patch will become unnecessary.
Instead, maybe you can remove the clamping code in usb-storage.
Alan Stern
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan
@ 2020-09-02 15:30 ` Alan Stern
2020-09-03 6:57 ` Tom Yan
1 sibling, 1 reply; 24+ messages in thread
From: Alan Stern @ 2020-09-02 15:30 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, yoshihiro.shimoda.uh
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern
@ 2020-09-03 6:57 ` Tom Yan
2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
0 siblings, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-03 6:57 UTC (permalink / raw)
To: Alan Stern; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 6:57 ` Tom Yan
@ 2020-09-03 7:56 ` Tom Yan
2020-09-03 7:56 ` [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH
0 siblings, 2 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 7:56 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c.
When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
to the dma max mapping size. Therefore, the correct device that should be used
for the clamping needs to be set.
The same clamping is still needed in the USB drivers as hw_max_sectors could be
changed there. The original clamping would be invalidated in such cases.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/scsiglue.c | 2 +-
drivers/usb/storage/uas.c | 17 +++++++++++------
drivers/usb/storage/usb.c | 5 +++--
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index e5a971b83e3f..560efd1479ba 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev)
static int slave_configure(struct scsi_device *sdev)
{
struct us_data *us = host_to_us(sdev->host);
- struct device *dev = us->pusb_dev->bus->sysdev;
+ struct device *dev = sdev->host->dma_dev;
/*
* Many devices have trouble transferring more than 32KB at a time,
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08f9296431e9..f4beeb8a8adb 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -827,17 +827,22 @@ 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;
}
static int uas_slave_configure(struct scsi_device *sdev)
{
struct uas_dev_info *devinfo = sdev->hostdata;
+ struct device *dev = sdev->host->dma_dev;
+
+ 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);
+
+ 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;
@@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
shost->can_queue = devinfo->qdepth - 2;
usb_set_intfdata(intf, shost);
- result = scsi_add_host(shost, &intf->dev);
+ result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev);
if (result)
goto free_streams;
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 94a64729dc27..c2ef367cf257 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us)
goto BadDevice;
usb_autopm_get_interface_no_resume(us->pusb_intf);
snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
- dev_name(&us->pusb_intf->dev));
- result = scsi_add_host(us_to_host(us), dev);
+ dev_name(dev));
+ result = scsi_add_host_with_dma(us_to_host(us), dev,
+ us->pusb_dev->bus->sysdev);
if (result) {
dev_warn(dev,
"Unable to add the scsi host\n");
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
@ 2020-09-03 7:56 ` Tom Yan
2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH
1 sibling, 0 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 7:56 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
There's no reason for uas to use a smaller value of max_sectors than
usb-storage.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/uas.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f4beeb8a8adb..c1123da43407 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev)
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 (devinfo->udev->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),
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
2020-09-03 7:56 ` [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
@ 2020-09-03 8:20 ` Greg KH
2020-09-03 8:28 ` Tom Yan
1 sibling, 1 reply; 24+ messages in thread
From: Greg KH @ 2020-09-03 8:20 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, stern, arnd, cyrozap, yoshihiro.shimoda.uh
On Thu, Sep 03, 2020 at 03:56:38PM +0800, Tom Yan wrote:
> Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c.
>
> When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
> to the dma max mapping size. Therefore, the correct device that should be used
> for the clamping needs to be set.
>
> The same clamping is still needed in the USB drivers as hw_max_sectors could be
> changed there. The original clamping would be invalidated in such cases.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
> drivers/usb/storage/scsiglue.c | 2 +-
> drivers/usb/storage/uas.c | 17 +++++++++++------
> drivers/usb/storage/usb.c | 5 +++--
> 3 files changed, 15 insertions(+), 9 deletions(-)
What changed from the previous version? Always include that below the
--- line as is documented to do so.
Please fix up and resend.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH
@ 2020-09-03 8:28 ` Tom Yan
2020-09-03 8:34 ` Greg KH
0 siblings, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-03 8:28 UTC (permalink / raw)
To: Greg KH; +Cc: linux-usb, Alan Stern, Arnd Bergmann, cyrozap, Yoshihiro Shimoda
Hmm, actually the older versions were pretty much an entirely
different approach/patch (not involving fixing the dma_dev at all).
Should I just resend without the "v3"?
On Thu, 3 Sep 2020 at 16:19, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 03, 2020 at 03:56:38PM +0800, Tom Yan wrote:
> > Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c.
> >
> > When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
> > to the dma max mapping size. Therefore, the correct device that should be used
> > for the clamping needs to be set.
> >
> > The same clamping is still needed in the USB drivers as hw_max_sectors could be
> > changed there. The original clamping would be invalidated in such cases.
> >
> > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > ---
> > drivers/usb/storage/scsiglue.c | 2 +-
> > drivers/usb/storage/uas.c | 17 +++++++++++------
> > drivers/usb/storage/usb.c | 5 +++--
> > 3 files changed, 15 insertions(+), 9 deletions(-)
>
> What changed from the previous version? Always include that below the
> --- line as is documented to do so.
>
> Please fix up and resend.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 8:28 ` Tom Yan
@ 2020-09-03 8:34 ` Greg KH
2020-09-03 8:46 ` [PATCH v4 " Tom Yan
2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
0 siblings, 2 replies; 24+ messages in thread
From: Greg KH @ 2020-09-03 8:34 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, Alan Stern, Arnd Bergmann, cyrozap, Yoshihiro Shimoda
On Thu, Sep 03, 2020 at 04:28:53PM +0800, Tom Yan wrote:
> Hmm, actually the older versions were pretty much an entirely
> different approach/patch (not involving fixing the dma_dev at all).
> Should I just resend without the "v3"?
As you already sent a v3 patch series, the next one would be v4, right?
:)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 8:34 ` Greg KH
@ 2020-09-03 8:46 ` Tom Yan
2020-09-03 8:46 ` [PATCH v4 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
1 sibling, 1 reply; 24+ messages in thread
From: Tom Yan @ 2020-09-03 8:46 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c.
When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
to the dma max mapping size. Therefore, the correct device that should be used
for the clamping needs to be set.
The same clamping is still needed in the USB drivers as hw_max_sectors could be
changed there. The original clamping would be invalidated in such cases.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS drives; split the "fallback" hw_max_sectors setting into another patch
v3: use a different approach: fix the dma_dev instead
v4: add the changelog of the patch series
drivers/usb/storage/scsiglue.c | 2 +-
drivers/usb/storage/uas.c | 17 +++++++++++------
drivers/usb/storage/usb.c | 5 +++--
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index e5a971b83e3f..560efd1479ba 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev)
static int slave_configure(struct scsi_device *sdev)
{
struct us_data *us = host_to_us(sdev->host);
- struct device *dev = us->pusb_dev->bus->sysdev;
+ struct device *dev = sdev->host->dma_dev;
/*
* Many devices have trouble transferring more than 32KB at a time,
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08f9296431e9..f4beeb8a8adb 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -827,17 +827,22 @@ 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;
}
static int uas_slave_configure(struct scsi_device *sdev)
{
struct uas_dev_info *devinfo = sdev->hostdata;
+ struct device *dev = sdev->host->dma_dev;
+
+ 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);
+
+ 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;
@@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
shost->can_queue = devinfo->qdepth - 2;
usb_set_intfdata(intf, shost);
- result = scsi_add_host(shost, &intf->dev);
+ result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev);
if (result)
goto free_streams;
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 94a64729dc27..c2ef367cf257 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us)
goto BadDevice;
usb_autopm_get_interface_no_resume(us->pusb_intf);
snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
- dev_name(&us->pusb_intf->dev));
- result = scsi_add_host(us_to_host(us), dev);
+ dev_name(dev));
+ result = scsi_add_host_with_dma(us_to_host(us), dev,
+ us->pusb_dev->bus->sysdev);
if (result) {
dev_warn(dev,
"Unable to add the scsi host\n");
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-03 8:46 ` [PATCH v4 " Tom Yan
@ 2020-09-03 8:46 ` Tom Yan
0 siblings, 0 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 8:46 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
There's no reason for uas to use a smaller value of max_sectors than
usb-storage.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/uas.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f4beeb8a8adb..c1123da43407 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev)
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 (devinfo->udev->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),
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 8:34 ` Greg KH
2020-09-03 8:46 ` [PATCH v4 " Tom Yan
@ 2020-09-03 8:50 ` Tom Yan
2020-09-03 8:50 ` [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern
1 sibling, 2 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 8:50 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c.
When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
to the dma max mapping size. Therefore, the correct device that should be used
for the clamping needs to be set.
The same clamping is still needed in the USB drivers as hw_max_sectors could be
changed there. The original clamping would be invalidated in such cases.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS
drives; split the "fallback" hw_max_sectors setting into another patch
v3: use a different approach: fix the dma_dev instead
v4: add the changelog of the patch series
v5: fix changelog line length
drivers/usb/storage/scsiglue.c | 2 +-
drivers/usb/storage/uas.c | 17 +++++++++++------
drivers/usb/storage/usb.c | 5 +++--
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index e5a971b83e3f..560efd1479ba 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev)
static int slave_configure(struct scsi_device *sdev)
{
struct us_data *us = host_to_us(sdev->host);
- struct device *dev = us->pusb_dev->bus->sysdev;
+ struct device *dev = sdev->host->dma_dev;
/*
* Many devices have trouble transferring more than 32KB at a time,
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08f9296431e9..f4beeb8a8adb 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -827,17 +827,22 @@ 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;
}
static int uas_slave_configure(struct scsi_device *sdev)
{
struct uas_dev_info *devinfo = sdev->hostdata;
+ struct device *dev = sdev->host->dma_dev;
+
+ 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);
+
+ 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;
@@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
shost->can_queue = devinfo->qdepth - 2;
usb_set_intfdata(intf, shost);
- result = scsi_add_host(shost, &intf->dev);
+ result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev);
if (result)
goto free_streams;
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 94a64729dc27..c2ef367cf257 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us)
goto BadDevice;
usb_autopm_get_interface_no_resume(us->pusb_intf);
snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
- dev_name(&us->pusb_intf->dev));
- result = scsi_add_host(us_to_host(us), dev);
+ dev_name(dev));
+ result = scsi_add_host_with_dma(us_to_host(us), dev,
+ us->pusb_dev->bus->sysdev);
if (result) {
dev_warn(dev,
"Unable to add the scsi host\n");
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
@ 2020-09-03 8:50 ` Tom Yan
2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern
1 sibling, 0 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 8:50 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
There's no reason for uas to use a smaller value of max_sectors than
usb-storage.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/uas.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f4beeb8a8adb..c1123da43407 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev)
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 (devinfo->udev->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),
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev
2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
2020-09-03 8:50 ` [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
@ 2020-09-03 15:54 ` Alan Stern
2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan
1 sibling, 1 reply; 24+ messages in thread
From: Alan Stern @ 2020-09-03 15:54 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, yoshihiro.shimoda.uh
On Thu, Sep 03, 2020 at 04:50:34PM +0800, Tom Yan wrote:
> Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c.
>
> When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
> to the dma max mapping size. Therefore, the correct device that should be used
> for the clamping needs to be set.
>
> The same clamping is still needed in the USB drivers as hw_max_sectors could be
> changed there. The original clamping would be invalidated in such cases.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
> v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS
> drives; split the "fallback" hw_max_sectors setting into another patch
> v3: use a different approach: fix the dma_dev instead
> v4: add the changelog of the patch series
> v5: fix changelog line length
> drivers/usb/storage/scsiglue.c | 2 +-
> drivers/usb/storage/uas.c | 17 +++++++++++------
> drivers/usb/storage/usb.c | 5 +++--
> 3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index e5a971b83e3f..560efd1479ba 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev)
> static int slave_configure(struct scsi_device *sdev)
> {
> struct us_data *us = host_to_us(sdev->host);
> - struct device *dev = us->pusb_dev->bus->sysdev;
> + struct device *dev = sdev->host->dma_dev;
>
> /*
> * Many devices have trouble transferring more than 32KB at a time,
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 08f9296431e9..f4beeb8a8adb 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -827,17 +827,22 @@ 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;
> }
>
> static int uas_slave_configure(struct scsi_device *sdev)
> {
> struct uas_dev_info *devinfo = sdev->hostdata;
> + struct device *dev = sdev->host->dma_dev;
> +
> + 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);
> +
> + 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;
> @@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
> shost->can_queue = devinfo->qdepth - 2;
>
> usb_set_intfdata(intf, shost);
> - result = scsi_add_host(shost, &intf->dev);
> + result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev);
> if (result)
> goto free_streams;
>
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 94a64729dc27..c2ef367cf257 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us)
> goto BadDevice;
> usb_autopm_get_interface_no_resume(us->pusb_intf);
> snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
> - dev_name(&us->pusb_intf->dev));
> - result = scsi_add_host(us_to_host(us), dev);
> + dev_name(dev));
> + result = scsi_add_host_with_dma(us_to_host(us), dev,
> + us->pusb_dev->bus->sysdev);
> if (result) {
> dev_warn(dev,
> "Unable to add the scsi host\n");
> --
> 2.28.0
These changes look fine.
However, usb-storage and uas are two separate drivers. Could you split
the patch into two, one containing the changes to usb.c and scsiglue.c
(those are part of usb-storage) and the other containing the changes to
uas.c?
Alan Stern
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev
2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern
@ 2020-09-03 18:17 ` Tom Yan
2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 18:17 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
Use scsi_add_host_with_dma() instead of scsi_add_host().
When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
to the dma max mapping size. Therefore, the correct device that should be used
for the clamping needs to be set.
The same clamping is still needed in usb-storage as hw_max_sectors could be
changed there. The original clamping would be invalidated in such cases.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS
drives; split the "fallback" hw_max_sectors setting into another patch
v3: use a different approach: fix the dma_dev instead
v4: add the changelog of the patch series
v5: fix changelog line length
v6: split dma_dev fix for usb-storage and uas
drivers/usb/storage/scsiglue.c | 2 +-
drivers/usb/storage/usb.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index e5a971b83e3f..560efd1479ba 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev)
static int slave_configure(struct scsi_device *sdev)
{
struct us_data *us = host_to_us(sdev->host);
- struct device *dev = us->pusb_dev->bus->sysdev;
+ struct device *dev = sdev->host->dma_dev;
/*
* Many devices have trouble transferring more than 32KB at a time,
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 94a64729dc27..c2ef367cf257 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us)
goto BadDevice;
usb_autopm_get_interface_no_resume(us->pusb_intf);
snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
- dev_name(&us->pusb_intf->dev));
- result = scsi_add_host(us_to_host(us), dev);
+ dev_name(dev));
+ result = scsi_add_host_with_dma(us_to_host(us), dev,
+ us->pusb_dev->bus->sysdev);
if (result) {
dev_warn(dev,
"Unable to add the scsi host\n");
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 2/3] uas: fix sdev->host->dma_dev
2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan
@ 2020-09-03 18:17 ` Tom Yan
2020-09-03 18:17 ` [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 18:31 ` [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev Alan Stern
2 siblings, 0 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 18:17 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
Use scsi_add_host_with_dma() instead of scsi_add_host().
When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
to the dma max mapping size. Therefore, the correct device that should be used
for the clamping needs to be set.
The same clamping is still needed in uas as hw_max_sectors could be changed
there. The original clamping would be invalidated in such cases.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/uas.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08f9296431e9..f4beeb8a8adb 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -827,17 +827,22 @@ 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;
}
static int uas_slave_configure(struct scsi_device *sdev)
{
struct uas_dev_info *devinfo = sdev->hostdata;
+ struct device *dev = sdev->host->dma_dev;
+
+ 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);
+
+ 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;
@@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
shost->can_queue = devinfo->qdepth - 2;
usb_set_intfdata(intf, shost);
- result = scsi_add_host(shost, &intf->dev);
+ result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev);
if (result)
goto free_streams;
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives
2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan
2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan
@ 2020-09-03 18:17 ` Tom Yan
2020-09-03 18:31 ` [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev Alan Stern
2 siblings, 0 replies; 24+ messages in thread
From: Tom Yan @ 2020-09-03 18:17 UTC (permalink / raw)
To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan
There's no reason for uas to use a smaller value of max_sectors than
usb-storage.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
drivers/usb/storage/uas.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f4beeb8a8adb..c1123da43407 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev)
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 (devinfo->udev->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),
--
2.28.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev
2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan
2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan
2020-09-03 18:17 ` [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
@ 2020-09-03 18:31 ` Alan Stern
2 siblings, 0 replies; 24+ messages in thread
From: Alan Stern @ 2020-09-03 18:31 UTC (permalink / raw)
To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, yoshihiro.shimoda.uh
On Fri, Sep 04, 2020 at 02:17:23AM +0800, Tom Yan wrote:
> Use scsi_add_host_with_dma() instead of scsi_add_host().
>
> When the scsi request queue is initialized/allocated, hw_max_sectors is clamped
> to the dma max mapping size. Therefore, the correct device that should be used
> for the clamping needs to be set.
>
> The same clamping is still needed in usb-storage as hw_max_sectors could be
> changed there. The original clamping would be invalidated in such cases.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
> v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS
> drives; split the "fallback" hw_max_sectors setting into another patch
> v3: use a different approach: fix the dma_dev instead
> v4: add the changelog of the patch series
> v5: fix changelog line length
> v6: split dma_dev fix for usb-storage and uas
> drivers/usb/storage/scsiglue.c | 2 +-
> drivers/usb/storage/usb.c | 5 +++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index e5a971b83e3f..560efd1479ba 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev)
> static int slave_configure(struct scsi_device *sdev)
> {
> struct us_data *us = host_to_us(sdev->host);
> - struct device *dev = us->pusb_dev->bus->sysdev;
> + struct device *dev = sdev->host->dma_dev;
>
> /*
> * Many devices have trouble transferring more than 32KB at a time,
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 94a64729dc27..c2ef367cf257 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us)
> goto BadDevice;
> usb_autopm_get_interface_no_resume(us->pusb_intf);
> snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
> - dev_name(&us->pusb_intf->dev));
> - result = scsi_add_host(us_to_host(us), dev);
> + dev_name(dev));
> + result = scsi_add_host_with_dma(us_to_host(us), dev,
> + us->pusb_dev->bus->sysdev);
> if (result) {
> dev_warn(dev,
> "Unable to add the scsi host\n");
> --
> 2.28.0
For all three patches:
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-09-03 18:31 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-01 5:54 [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan
2020-09-01 14:55 ` Alan Stern
2020-09-01 23:24 ` Tom Yan
2020-09-01 23:44 ` Tom Yan
2020-09-02 15:24 ` Alan Stern
2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan
2020-09-02 0:20 ` Tom Yan
2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern
2020-09-03 6:57 ` Tom Yan
2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
2020-09-03 7:56 ` [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH
2020-09-03 8:28 ` Tom Yan
2020-09-03 8:34 ` Greg KH
2020-09-03 8:46 ` [PATCH v4 " Tom Yan
2020-09-03 8:46 ` [PATCH v4 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan
2020-09-03 8:50 ` [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern
2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan
2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan
2020-09-03 18:17 ` [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan
2020-09-03 18:31 ` [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).