| Message ID | 1560418611-10239-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v2] usb-storage: Add a limitation for blk_queue_max_hw_sectors() | expand |
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
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.
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
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
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.
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
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
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
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
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
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
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
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
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(+)