[PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

This patch fixes an issue that the following error happens on
swiotlb environment:

	xhci-hcd ee000000.usb: swiotlb buffer is full (sz: 524288 bytes), total 32768 (slots), used 1338 (slots)

On the kernel v5.1, block settings of a usb-storage with SuperSpeed
were the following so that the block layer will allocate buffers
up to 64 KiB, and then the issue didn't happen.

	max_segment_size = 65536
	max_hw_sectors_kb = 1024

After the commit 09324d32d2a0 ("block: force an unlimited segment
size on queues with a virt boundary") is applied, the block settings
are the following. So, the block layer will allocate buffers up to
1024 KiB, and then the issue happens:

	max_segment_size = 4294967295
	max_hw_sectors_kb = 1024

To fix the issue, the usb-storage driver checks the maximum size of
a mapping for the device and then adjusts the max_hw_sectors_kb
if required. After this patch is applied, the block settings will
be the following, and then the issue doesn't happen.

	max_segment_size = 4294967295
	max_hw_sectors_kb = 256

Fixes: 09324d32d2a0 ("block: force an unlimited segment size on queues with a virt boundary")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Changes from v1:
 - Call blk_queue_max_hw_sectors() for the maximum size of mapping
   unconditionally to simplify the code by using read the value back
   from the queue in the end.
 - Add a comment on the code.
 - On v1, I got Reviewed-by from Christoph. But, I changed the code a little,
   I removed the Reviewed-by.

 drivers/usb/storage/scsiglue.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 59190d8..556bb4f 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -28,6 +28,8 @@
  * status of a command.
  */
 
+#include <linux/blkdev.h>
+#include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 
@@ -99,6 +101,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;
 
 	/*
 	 * Many devices have trouble transferring more than 32KB at a time,
@@ -129,6 +132,14 @@ static int slave_configure(struct scsi_device *sdev)
 	}
 
 	/*
+	 * 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));
+
+	/*
 	 * Some USB host controllers can't do DMA; they have to use PIO.
 	 * They indicate this by setting their dma_mask to NULL.  For
 	 * such controllers we need to make sure the block layer sets
-- 
2.7.4

Re: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Alan Stern]

On Thu, 13 Jun 2019, Yoshihiro Shimoda wrote:

> This patch fixes an issue that the following error happens on
> swiotlb environment:
> 
> 	xhci-hcd ee000000.usb: swiotlb buffer is full (sz: 524288 bytes), total 32768 (slots), used 1338 (slots)
> 
> On the kernel v5.1, block settings of a usb-storage with SuperSpeed
> were the following so that the block layer will allocate buffers
> up to 64 KiB, and then the issue didn't happen.
> 
> 	max_segment_size = 65536
> 	max_hw_sectors_kb = 1024
> 
> After the commit 09324d32d2a0 ("block: force an unlimited segment
> size on queues with a virt boundary") is applied, the block settings
> are the following. So, the block layer will allocate buffers up to
> 1024 KiB, and then the issue happens:
> 
> 	max_segment_size = 4294967295
> 	max_hw_sectors_kb = 1024
> 
> To fix the issue, the usb-storage driver checks the maximum size of
> a mapping for the device and then adjusts the max_hw_sectors_kb
> if required. After this patch is applied, the block settings will
> be the following, and then the issue doesn't happen.
> 
> 	max_segment_size = 4294967295
> 	max_hw_sectors_kb = 256
> 
> Fixes: 09324d32d2a0 ("block: force an unlimited segment size on queues with a virt boundary")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Changes from v1:
>  - Call blk_queue_max_hw_sectors() for the maximum size of mapping
>    unconditionally to simplify the code by using read the value back
>    from the queue in the end.
>  - Add a comment on the code.
>  - On v1, I got Reviewed-by from Christoph. But, I changed the code a little,
>    I removed the Reviewed-by.
> 
>  drivers/usb/storage/scsiglue.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index 59190d8..556bb4f 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -28,6 +28,8 @@
>   * status of a command.
>   */
>  
> +#include <linux/blkdev.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  
> @@ -99,6 +101,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;
>  
>  	/*
>  	 * Many devices have trouble transferring more than 32KB at a time,
> @@ -129,6 +132,14 @@ static int slave_configure(struct scsi_device *sdev)
>  	}
>  
>  	/*
> +	 * 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));
> +
> +	/*
>  	 * Some USB host controllers can't do DMA; they have to use PIO.
>  	 * They indicate this by setting their dma_mask to NULL.  For
>  	 * such controllers we need to make sure the block layer sets

Hmmm.  Would it be easier instead to copy the DMA mapping parameters
from us->pusb_dev->bus->sysdev into the SCSI host's parent before
calling scsi_add_host()?  That way the correct values would be
available from the beginning, so the existing DMA setup would
automatically use the correct sizes.

In fact, the USB core already sets the dma_mask and dma_pfn_offset 
fields for the SCSI host's parent (search for dma_mask in 
drivers/usb/core/message.c).  Is there something else we need to set?

Alan Stern

Re: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Christoph Hellwig]

On Thu, Jun 13, 2019 at 01:06:40PM -0400, Alan Stern wrote:
> Hmmm.  Would it be easier instead to copy the DMA mapping parameters
> from us->pusb_dev->bus->sysdev into the SCSI host's parent before
> calling scsi_add_host()?  That way the correct values would be
> available from the beginning, so the existing DMA setup would
> automatically use the correct sizes.

It would in theory.  But at usb-storage has a special max_sectors quirk
for tape devices, and as the device type is a per-LU property we'd
still have to override it in ->slave_configure.

Re: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Alan Stern]

On Thu, 13 Jun 2019, Christoph Hellwig wrote:

> On Thu, Jun 13, 2019 at 01:06:40PM -0400, Alan Stern wrote:
> > Hmmm.  Would it be easier instead to copy the DMA mapping parameters
> > from us->pusb_dev->bus->sysdev into the SCSI host's parent before
> > calling scsi_add_host()?  That way the correct values would be
> > available from the beginning, so the existing DMA setup would
> > automatically use the correct sizes.
> 
> It would in theory.  But at usb-storage has a special max_sectors quirk
> for tape devices, and as the device type is a per-LU property we'd
> still have to override it in ->slave_configure.

Not just for tape devices.  But that's okay; those max_sectors
overrides have been present for a long time and we can keep them.  
Getting rid of the virt_boundary_mask stuff would still be a big
improvement.

Alan Stern

RE: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

Hi Alan,

> From: Alan Stern, Sent: Friday, June 14, 2019 2:22 AM
> 
> On Thu, 13 Jun 2019, Christoph Hellwig wrote:
> 
> > On Thu, Jun 13, 2019 at 01:06:40PM -0400, Alan Stern wrote:
> > > Hmmm.  Would it be easier instead to copy the DMA mapping parameters
> > > from us->pusb_dev->bus->sysdev into the SCSI host's parent before
> > > calling scsi_add_host()?  That way the correct values would be
> > > available from the beginning, so the existing DMA setup would
> > > automatically use the correct sizes.
> >
> > It would in theory.  But at usb-storage has a special max_sectors quirk
> > for tape devices, and as the device type is a per-LU property we'd
> > still have to override it in ->slave_configure.
> 
> Not just for tape devices.  But that's okay; those max_sectors
> overrides have been present for a long time and we can keep them.
> Getting rid of the virt_boundary_mask stuff would still be a big
> improvement.

Thank you for the comments. So, should I wait for getting rid of the
virt_boundary_mask stuff? If I revise the commit log of this patch,
is it acceptable for v5.2-stable as a workaround? In other words,
I worry about this issue exists on v5.2-stable.

Best regards,
Yoshihiro Shimoda

> Alan Stern

Re: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Christoph Hellwig]

On Mon, Jun 17, 2019 at 04:17:43AM +0000, Yoshihiro Shimoda wrote:
> Thank you for the comments. So, should I wait for getting rid of the
> virt_boundary_mask stuff? If I revise the commit log of this patch,
> is it acceptable for v5.2-stable as a workaround? In other words,
> I worry about this issue exists on v5.2-stable.

It does exist on 5.2-stable and we should fix it.  I'll plan to resend
my series to fix the virt_boundary issues for the other SCSI driver
soon, but we'll still need to sort out usb-storage.

RE: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

Hi Alan, shuah, Suwan,

> From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:22 PM
> 
> On Mon, Jun 17, 2019 at 04:17:43AM +0000, Yoshihiro Shimoda wrote:
> > Thank you for the comments. So, should I wait for getting rid of the
> > virt_boundary_mask stuff? If I revise the commit log of this patch,
> > is it acceptable for v5.2-stable as a workaround? In other words,
> > I worry about this issue exists on v5.2-stable.
> 
> It does exist on 5.2-stable and we should fix it.  I'll plan to resend
> my series to fix the virt_boundary issues for the other SCSI driver
> soon, but we'll still need to sort out usb-storage.

I guess that getting rid of the virt_boundary_mask stuff [1] needs more time.
So, for v5.2-stable, would you accept my patch as a workaround?
JFYI, v5.2-rc7 still has this "swiotlb buffer is full" issue.

[1]
https://marc.info/?l=linux-kernel&m=156114524808042&w=2

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Suwan Kim]

On Tue, Jul 02, 2019 at 10:07:42AM +0000, Yoshihiro Shimoda wrote:
> Hi Alan, shuah, Suwan,
> 
> > From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:22 PM
> > 
> > On Mon, Jun 17, 2019 at 04:17:43AM +0000, Yoshihiro Shimoda wrote:
> > > Thank you for the comments. So, should I wait for getting rid of the
> > > virt_boundary_mask stuff? If I revise the commit log of this patch,
> > > is it acceptable for v5.2-stable as a workaround? In other words,
> > > I worry about this issue exists on v5.2-stable.
> > 
> > It does exist on 5.2-stable and we should fix it.  I'll plan to resend
> > my series to fix the virt_boundary issues for the other SCSI driver
> > soon, but we'll still need to sort out usb-storage.
> 
> I guess that getting rid of the virt_boundary_mask stuff [1] needs more time.
> So, for v5.2-stable, would you accept my patch as a workaround?
> JFYI, v5.2-rc7 still has this "swiotlb buffer is full" issue.

Hi, Yoshihiro,

I have been implementing v2 of this patch according to Alan's comment
(allocate small buffers and urbs instead of one big buffer) and it
takes some time. The implementation is almost done but I think it
will take a few days or more because of the test and bug fix...

Regards

Suwan Kim

Re: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[shuah]

On 7/2/19 7:49 AM, Suwan Kim wrote:
> On Tue, Jul 02, 2019 at 10:07:42AM +0000, Yoshihiro Shimoda wrote:
>> Hi Alan, shuah, Suwan,
>>
>>> From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:22 PM
>>>
>>> On Mon, Jun 17, 2019 at 04:17:43AM +0000, Yoshihiro Shimoda wrote:
>>>> Thank you for the comments. So, should I wait for getting rid of the
>>>> virt_boundary_mask stuff? If I revise the commit log of this patch,
>>>> is it acceptable for v5.2-stable as a workaround? In other words,
>>>> I worry about this issue exists on v5.2-stable.
>>>
>>> It does exist on 5.2-stable and we should fix it.  I'll plan to resend
>>> my series to fix the virt_boundary issues for the other SCSI driver
>>> soon, but we'll still need to sort out usb-storage.
>>
>> I guess that getting rid of the virt_boundary_mask stuff [1] needs more time.
>> So, for v5.2-stable, would you accept my patch as a workaround?
>> JFYI, v5.2-rc7 still has this "swiotlb buffer is full" issue.
> 
> Hi, Yoshihiro,
> 
> I have been implementing v2 of this patch according to Alan's comment
> (allocate small buffers and urbs instead of one big buffer) and it
> takes some time. The implementation is almost done but I think it
> will take a few days or more because of the test and bug fix...
> 
> Regards
> 
> Suwan Kim
> 

Hi Yoshihiro,

Suwan's patch series will definitely won't make it before 5.2 comes
out. There is not enough time for that. Please send you work-around.


Suwan,

I figured you have been working on Alan's feedback on your patch series.
Thanks for working on the fixing the problem.

thanks,
-- Shuah

RE: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Alan Stern]

On Tue, 2 Jul 2019, Yoshihiro Shimoda wrote:

> Hi Alan, shuah, Suwan,
> 
> > From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:22 PM
> > 
> > On Mon, Jun 17, 2019 at 04:17:43AM +0000, Yoshihiro Shimoda wrote:
> > > Thank you for the comments. So, should I wait for getting rid of the
> > > virt_boundary_mask stuff? If I revise the commit log of this patch,
> > > is it acceptable for v5.2-stable as a workaround? In other words,
> > > I worry about this issue exists on v5.2-stable.
> > 
> > It does exist on 5.2-stable and we should fix it.  I'll plan to resend
> > my series to fix the virt_boundary issues for the other SCSI driver
> > soon, but we'll still need to sort out usb-storage.
> 
> I guess that getting rid of the virt_boundary_mask stuff [1] needs more time.
> So, for v5.2-stable, would you accept my patch as a workaround?
> JFYI, v5.2-rc7 still has this "swiotlb buffer is full" issue.
> 
> [1]
> https://marc.info/?l=linux-kernel&m=156114524808042&w=2

I would really prefer to see a different solution.

The actual problem is that the usb_device and usb_interface structures
are supposed to inherit all of their DMA properties from the bus's host
controller.  But the existing code copies only the dma_mask and
dma_pfn_offset fields in the embedded device structures.  If we copied
all of the important DMA fields then this patch wouldn't be needed; the
max_sectors value for the request queue would be set up correctly to
begin with.

So what I would like to see is a new subroutine -- perhaps in the
driver core -- that copies the DMA fields from one struct device to
another.  Then we could call this subroutine in usb_alloc_dev() and
usb_set_configuration() instead of copying the information manually.

Greg and Christoph, does that make sense?

Yoshihiro, would you like to write a patch that does this?

Alan Stern

RE: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

Hi Alan,

> From: Alan Stern, Sent: Tuesday, July 2, 2019 11:28 PM
> 
> On Tue, 2 Jul 2019, Yoshihiro Shimoda wrote:
> 
> > Hi Alan, shuah, Suwan,
> >
> > > From: Christoph Hellwig, Sent: Monday, June 17, 2019 3:22 PM
> > >
> > > On Mon, Jun 17, 2019 at 04:17:43AM +0000, Yoshihiro Shimoda wrote:
> > > > Thank you for the comments. So, should I wait for getting rid of the
> > > > virt_boundary_mask stuff? If I revise the commit log of this patch,
> > > > is it acceptable for v5.2-stable as a workaround? In other words,
> > > > I worry about this issue exists on v5.2-stable.
> > >
> > > It does exist on 5.2-stable and we should fix it.  I'll plan to resend
> > > my series to fix the virt_boundary issues for the other SCSI driver
> > > soon, but we'll still need to sort out usb-storage.
> >
> > I guess that getting rid of the virt_boundary_mask stuff [1] needs more time.
> > So, for v5.2-stable, would you accept my patch as a workaround?
> > JFYI, v5.2-rc7 still has this "swiotlb buffer is full" issue.
> >
> > [1]
> > https://marc.info/?l=linux-kernel&m=156114524808042&w=2
> 
> I would really prefer to see a different solution.
> 
> The actual problem is that the usb_device and usb_interface structures
> are supposed to inherit all of their DMA properties from the bus's host
> controller.  But the existing code copies only the dma_mask and
> dma_pfn_offset fields in the embedded device structures.  If we copied
> all of the important DMA fields then this patch wouldn't be needed; the
> max_sectors value for the request queue would be set up correctly to
> begin with.

I'm sorry, but I cannot understand what are important DMA fields.
IIUC, usb-storage driver should take care of calling blk_queue_ APIs anyway because:

 - As Christoph mentioned before on the email [1], usb-storage has a special
   max_sectors quirk for tape and SuperSpeed devices.
 - Since blk_queue_* APIs don't take device structure pointer, the block layer
   cannot call any DMA mapping APIs. So, even if any other DMA fields are copied,
   the behavior is not changed.

[1]
https://www.spinics.net/lists/linux-usb/msg181527.html

What do you think?

Best regards,
Yoshihiro Shimoda

> So what I would like to see is a new subroutine -- perhaps in the
> driver core -- that copies the DMA fields from one struct device to
> another.  Then we could call this subroutine in usb_alloc_dev() and
> usb_set_configuration() instead of copying the information manually.
> 
> Greg and Christoph, does that make sense?
> 
> Yoshihiro, would you like to write a patch that does this?
> 
> Alan Stern

RE: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Alan Stern]

On Wed, 3 Jul 2019, Yoshihiro Shimoda wrote:

> > I would really prefer to see a different solution.
> > 
> > The actual problem is that the usb_device and usb_interface structures
> > are supposed to inherit all of their DMA properties from the bus's host
> > controller.  But the existing code copies only the dma_mask and
> > dma_pfn_offset fields in the embedded device structures.  If we copied
> > all of the important DMA fields then this patch wouldn't be needed; the
> > max_sectors value for the request queue would be set up correctly to
> > begin with.
> 
> I'm sorry, but I cannot understand what are important DMA fields.

Probably all of them are important; I don't know.

> IIUC, usb-storage driver should take care of calling blk_queue_ APIs anyway because:
> 
>  - As Christoph mentioned before on the email [1], usb-storage has a special
>    max_sectors quirk for tape and SuperSpeed devices.
>  - Since blk_queue_* APIs don't take device structure pointer, the block layer
>    cannot call any DMA mapping APIs. So, even if any other DMA fields are copied,
>    the behavior is not changed.

Although the blk_queue_* APIs don't take device structure pointers, the
SCSI layer does know about devices.  And since it is the SCSI layer
which creates the request queue, changing the DMA fields should change
the behavior.

However, you are correct that usb-storage has to call the blk_queue_* 
APIs anyway.  So I guess your patch is the right thing to do after all.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

I still think that copying the DMA fields would be a good idea, though.

Alan Stern

> [1]
> https://www.spinics.net/lists/linux-usb/msg181527.html
> 
> What do you think?
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > So what I would like to see is a new subroutine -- perhaps in the
> > driver core -- that copies the DMA fields from one struct device to
> > another.  Then we could call this subroutine in usb_alloc_dev() and
> > usb_set_configuration() instead of copying the information manually.
> > 
> > Greg and Christoph, does that make sense?
> > 
> > Yoshihiro, would you like to write a patch that does this?
> > 
> > Alan Stern

RE: [PATCH v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

Hi Alan,

> From: Alan Stern, Sent: Wednesday, July 3, 2019 11:20 PM
> 
> On Wed, 3 Jul 2019, Yoshihiro Shimoda wrote:
<snip>
> > IIUC, usb-storage driver should take care of calling blk_queue_ APIs anyway because:
> >
> >  - As Christoph mentioned before on the email [1], usb-storage has a special
> >    max_sectors quirk for tape and SuperSpeed devices.
> >  - Since blk_queue_* APIs don't take device structure pointer, the block layer
> >    cannot call any DMA mapping APIs. So, even if any other DMA fields are copied,
> >    the behavior is not changed.
> 
> Although the blk_queue_* APIs don't take device structure pointers, the
> SCSI layer does know about devices.  And since it is the SCSI layer
> which creates the request queue, changing the DMA fields should change
> the behavior.
> 
> However, you are correct that usb-storage has to call the blk_queue_*
> APIs anyway.  So I guess your patch is the right thing to do after all.
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thank you very much for your Acked-by!

Greg, if possible, would you merge this patch into v5.2?

Best regards,
Yoshihiro Shimoda

> I still think that copying the DMA fields would be a good idea, though.
> 
> Alan Stern
> 
> > [1]
> > https://www.spinics.net/lists/linux-usb/msg181527.html
> >
> > What do you think?
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > So what I would like to see is a new subroutine -- perhaps in the
> > > driver core -- that copies the DMA fields from one struct device to
> > > another.  Then we could call this subroutine in usb_alloc_dev() and
> > > usb_set_configuration() instead of copying the information manually.
> > >
> > > Greg and Christoph, does that make sense?
> > >
> > > Yoshihiro, would you like to write a patch that does this?
> > >
> > > Alan Stern