usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping

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 <[email protected]>

---
 drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
 drivers/usb/storage/uas.c      | 23 ++++++++++++++++-----
 2 files changed, 35 insertions(+), 25 deletions(-)

drivers/usb/storage/scsiglue.c +17 -20

index e5a971b83e3f..804cbc0ba4da 100644
@@ -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,
 

drivers/usb/storage/uas.c +18 -5

index 08f9296431e9..cffa435afd84 100644
@@ -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;
 

Signature · 1 line

2.28.0

Patch submissions should have text lines limited to fewer than 80 columns.

Quoted 4 lines · 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.

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.

Quoted 6 lines

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

Quoted 105 lines

Signed-off-by Tom Yan <[email protected]>

---
 drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
 drivers/usb/storage/uas.c      | 23 ++++++++++++++++-----
 2 files changed, 35 insertions(+), 25 deletions(-)

drivers/usb/storage/scsiglue.c +17 -20

index e5a971b83e3f..804cbc0ba4da 100644
@@ -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,
 

drivers/usb/storage/uas.c +18 -6

index 08f9296431e9..cffa435afd84 100644
@@ -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

Quoted 15 lines · Alan Stern

Patch submissions should have text lines limited to fewer than 80 columns.

Quoted 4 lines · 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.

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 108 lines

Alan Stern

Quoted 105 lines

Signed-off-by Tom Yan <[email protected]>

---
 drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
 drivers/usb/storage/uas.c      | 23 ++++++++++++++++-----
 2 files changed, 35 insertions(+), 25 deletions(-)

drivers/usb/storage/scsiglue.c +17 -20

index e5a971b83e3f..804cbc0ba4da 100644
@@ -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,
 

drivers/usb/storage/uas.c +18 -6

index 08f9296431e9..cffa435afd84 100644
@@ -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

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.

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 <[email protected]>

---
 drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------
 drivers/usb/storage/uas.c      |  2 ++
 2 files changed, 19 insertions(+), 20 deletions(-)

drivers/usb/storage/scsiglue.c +17 -20

index e5a971b83e3f..1f60d777a7e8 100644
@@ -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,
 

drivers/usb/storage/uas.c +2 -1

index 813c49914b9a..592e1358822e 100644
@@ -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

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 <[email protected]>

---
 drivers/usb/storage/uas.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/usb/storage/uas.c +0 -5

index 08f9296431e9..813c49914b9a 100644
@@ -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;
 }
 

Quoted 27 lines · 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 <[email protected]>

---
 drivers/usb/storage/uas.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/usb/storage/uas.c +0 -5

index 08f9296431e9..813c49914b9a 100644
@@ -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;
 }
 

drivers/usb/storage/uas.c static int uas_slave_configure(struct scsi_device *sdev) · line 838

Quoted 5 lines

drivers/usb/storage/uas.c +2 -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.

drivers/usb/storage/uas.c static int uas_slave_configure(struct scsi_device *sdev) · line 844

Quoted 6 lines

drivers/usb/storage/uas.c +6 -0

+
+     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.

drivers/usb/storage/uas.c static int uas_slave_configure(struct scsi_device *sdev) · line 845

Quoted 1 line

drivers/usb/storage/uas.c +1 -0

+             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.

drivers/usb/storage/uas.c static int uas_slave_configure(struct scsi_device *sdev) · line 849

Quoted 5 lines

drivers/usb/storage/uas.c +4 -0

+
+     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

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 <[email protected]>

---
 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(-)

Quoted 9 lines · 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.

Quoted 6 lines

Signed-off-by Tom Yan <[email protected]>

---
 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

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"?

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 <[email protected]>

---
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(-)

drivers/usb/storage/scsiglue.c +1 -1

index e5a971b83e3f..560efd1479ba 100644
@@ -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,

drivers/usb/storage/uas.c +11 -6

index 08f9296431e9..f4beeb8a8adb 100644
@@ -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;
 

drivers/usb/storage/usb.c +3 -2

index 94a64729dc27..c2ef367cf257 100644
@@ -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");

Signature · 1 line

2.28.0

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 <[email protected]>

---
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(-)

drivers/usb/storage/scsiglue.c +1 -1

index e5a971b83e3f..560efd1479ba 100644
@@ -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,

drivers/usb/storage/usb.c +3 -2

index 94a64729dc27..c2ef367cf257 100644
@@ -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");

Signature · 1 line

2.28.0