| Message ID | 20201128154849.3193-2-tom.ty89@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] uas: revert from scsi_add_host_with_dma() to scsi_add_host() | expand |
Hi, On 11/28/20 4:48 PM, Tom Yan wrote: > While the change only seemed to have caused issue on uas drives, we > probably want to avoid it in the usb-storage driver as well, until > we are sure it is the right thing to do. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> This seems to do a whole lot more then just dropping back from scsi_add_host_with_dma() to scsi_add_host(). This has way more lines then the orginal commit. IMHO it would be best to just revert commit 0154012f8018bba4d9971d1007c12ffd48539ddb and then submit these changes as a separate patch (which would be 5.11 material then). That separate patch could then also have a proper commit message explaining the other changes you are making, which is also not unimportant. Regards, Hans > --- > drivers/usb/storage/scsiglue.c | 40 +++++++++++++++++----------------- > drivers/usb/storage/usb.c | 3 +-- > 2 files changed, 21 insertions(+), 22 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index 560efd1479ba..6539bae1c188 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 = sdev->host->dma_dev; > + struct device *dev = us->pusb_dev->bus->sysdev; > > /* > * Many devices have trouble transferring more than 32KB at a time, > @@ -120,6 +120,25 @@ static int slave_configure(struct scsi_device *sdev) > * better throughput on most devices. > */ > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > + } else { > + /* > + * 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. > + */ > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > } > > /* > @@ -627,25 +646,6 @@ static const struct scsi_host_template usb_stor_host_template = { > .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/usb.c b/drivers/usb/storage/usb.c > index c2ef367cf257..f177da4ff1bc 100644 > --- a/drivers/usb/storage/usb.c > +++ b/drivers/usb/storage/usb.c > @@ -1050,8 +1050,7 @@ int usb_stor_probe2(struct us_data *us) > usb_autopm_get_interface_no_resume(us->pusb_intf); > snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", > dev_name(dev)); > - result = scsi_add_host_with_dma(us_to_host(us), dev, > - us->pusb_dev->bus->sysdev); > + result = scsi_add_host(us_to_host(us), dev); > if (result) { > dev_warn(dev, > "Unable to add the scsi host\n"); >
It's merely a moving of comment moving for/and a no-behavioral-change adaptation for the reversion. Similar has been done in the equivalent patch for the UAS driver (and the reason is stated there). On Mon, 30 Nov 2020 at 17:50, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 11/28/20 4:48 PM, Tom Yan wrote: > > While the change only seemed to have caused issue on uas drives, we > > probably want to avoid it in the usb-storage driver as well, until > > we are sure it is the right thing to do. > > > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > This seems to do a whole lot more then just dropping back from > scsi_add_host_with_dma() to scsi_add_host(). This has way more > lines then the orginal commit. > > IMHO it would be best to just revert commit 0154012f8018bba4d9971d1007c12ffd48539ddb > and then submit these changes as a separate patch (which would be > 5.11 material then). > > That separate patch could then also have a proper commit message > explaining the other changes you are making, which is also not > unimportant. > > Regards, > > Hans > > > > > > --- > > drivers/usb/storage/scsiglue.c | 40 +++++++++++++++++----------------- > > drivers/usb/storage/usb.c | 3 +-- > > 2 files changed, 21 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > > index 560efd1479ba..6539bae1c188 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 = sdev->host->dma_dev; > > + struct device *dev = us->pusb_dev->bus->sysdev; > > > > /* > > * Many devices have trouble transferring more than 32KB at a time, > > @@ -120,6 +120,25 @@ static int slave_configure(struct scsi_device *sdev) > > * better throughput on most devices. > > */ > > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > > + } else { > > + /* > > + * 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. > > + */ > > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > > } > > > > /* > > @@ -627,25 +646,6 @@ static const struct scsi_host_template usb_stor_host_template = { > > .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/usb.c b/drivers/usb/storage/usb.c > > index c2ef367cf257..f177da4ff1bc 100644 > > --- a/drivers/usb/storage/usb.c > > +++ b/drivers/usb/storage/usb.c > > @@ -1050,8 +1050,7 @@ int usb_stor_probe2(struct us_data *us) > > usb_autopm_get_interface_no_resume(us->pusb_intf); > > snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", > > dev_name(dev)); > > - result = scsi_add_host_with_dma(us_to_host(us), dev, > > - us->pusb_dev->bus->sysdev); > > + result = scsi_add_host(us_to_host(us), dev); > > if (result) { > > dev_warn(dev, > > "Unable to add the scsi host\n"); > > >
Hi, On 11/30/20 1:58 PM, Tom Yan wrote: > It's merely a moving of comment moving for/and a no-behavioral-change > adaptation for the reversion.> IMHO the revert of the troublesome commit and the other/new changes really should be 2 separate commits. But I will let Alan and Greg have the final verdict on this. p.s. Why did you not send this patch-series to Alan Stern, the maintainer of the usb-storage driver? > Similar has been done in the equivalent > patch for the UAS driver (and the reason is stated there). In the UAS driver the code setting max-hw-sectors was already moved to its new place and another patch was added on top, so that is different. Regards, Hans > > On Mon, 30 Nov 2020 at 17:50, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 11/28/20 4:48 PM, Tom Yan wrote: >>> While the change only seemed to have caused issue on uas drives, we >>> probably want to avoid it in the usb-storage driver as well, until >>> we are sure it is the right thing to do. >>> >>> Signed-off-by: Tom Yan <tom.ty89@gmail.com> >> >> This seems to do a whole lot more then just dropping back from >> scsi_add_host_with_dma() to scsi_add_host(). This has way more >> lines then the orginal commit. >> >> IMHO it would be best to just revert commit 0154012f8018bba4d9971d1007c12ffd48539ddb >> and then submit these changes as a separate patch (which would be >> 5.11 material then). >> >> That separate patch could then also have a proper commit message >> explaining the other changes you are making, which is also not >> unimportant. >> >> Regards, >> >> Hans >> >> >> >> >>> --- >>> drivers/usb/storage/scsiglue.c | 40 +++++++++++++++++----------------- >>> drivers/usb/storage/usb.c | 3 +-- >>> 2 files changed, 21 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c >>> index 560efd1479ba..6539bae1c188 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 = sdev->host->dma_dev; >>> + struct device *dev = us->pusb_dev->bus->sysdev; >>> >>> /* >>> * Many devices have trouble transferring more than 32KB at a time, >>> @@ -120,6 +120,25 @@ static int slave_configure(struct scsi_device *sdev) >>> * better throughput on most devices. >>> */ >>> blk_queue_max_hw_sectors(sdev->request_queue, 2048); >>> + } else { >>> + /* >>> + * 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. >>> + */ >>> + blk_queue_max_hw_sectors(sdev->request_queue, 240); >>> } >>> >>> /* >>> @@ -627,25 +646,6 @@ static const struct scsi_host_template usb_stor_host_template = { >>> .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/usb.c b/drivers/usb/storage/usb.c >>> index c2ef367cf257..f177da4ff1bc 100644 >>> --- a/drivers/usb/storage/usb.c >>> +++ b/drivers/usb/storage/usb.c >>> @@ -1050,8 +1050,7 @@ int usb_stor_probe2(struct us_data *us) >>> usb_autopm_get_interface_no_resume(us->pusb_intf); >>> snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", >>> dev_name(dev)); >>> - result = scsi_add_host_with_dma(us_to_host(us), dev, >>> - us->pusb_dev->bus->sysdev); >>> + result = scsi_add_host(us_to_host(us), dev); >>> if (result) { >>> dev_warn(dev, >>> "Unable to add the scsi host\n"); >>> >> >
On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > Hi, > > On 11/30/20 1:58 PM, Tom Yan wrote: > > It's merely a moving of comment moving for/and a no-behavioral-change > > adaptation for the reversion.> > > IMHO the revert of the troublesome commit and the other/new changes really > should be 2 separate commits. But I will let Alan and Greg have the final > verdict on this. I would prefer to just revert the commits and not do anything different/special here so late in the release cycle. So, if Alan agrees, I'll be glad to do them on my end, I just need the commit ids for them. thanks, greg k-h
Hi, On 11/30/20 2:30 PM, Greg KH wrote: > On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: >> Hi, >> >> On 11/30/20 1:58 PM, Tom Yan wrote: >>> It's merely a moving of comment moving for/and a no-behavioral-change >>> adaptation for the reversion.> >> >> IMHO the revert of the troublesome commit and the other/new changes really >> should be 2 separate commits. But I will let Alan and Greg have the final >> verdict on this. > > I would prefer to just revert the commits and not do anything > different/special here so late in the release cycle. > > So, if Alan agrees, I'll be glad to do them on my end, I just need the > commit ids for them. The troublesome commit are (in reverse, so revert, order): 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") 558033c2828f ("uas: fix sdev->host->dma_dev") 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the last 2 patches do, with the dmadev argument of that call pointing to the device for the XHCI controller is causing changes to the DMA settings of the XHCI controller itself which is causing regressions in 5.10, see this email thread: https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t Regards, Hans
On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > Hi, > > On 11/30/20 2:30 PM, Greg KH wrote: > > On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > >> Hi, > >> > >> On 11/30/20 1:58 PM, Tom Yan wrote: > >>> It's merely a moving of comment moving for/and a no-behavioral-change > >>> adaptation for the reversion.> > >> > >> IMHO the revert of the troublesome commit and the other/new changes really > >> should be 2 separate commits. But I will let Alan and Greg have the final > >> verdict on this. > > > > I would prefer to just revert the commits and not do anything > > different/special here so late in the release cycle. > > > > So, if Alan agrees, I'll be glad to do them on my end, I just need the > > commit ids for them. > > The troublesome commit are (in reverse, so revert, order): > > 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > 558033c2828f ("uas: fix sdev->host->dma_dev") > 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > > Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > last 2 patches do, with the dmadev argument of that call pointing to the device > for the XHCI controller is causing changes to the DMA settings of the XHCI controller > itself which is causing regressions in 5.10, see this email thread: > > https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t Thanks, I'll wait for Alan to respond, but I think just reverting these is the best solution at this point in time. You have tested those reverts, solve this, right? If so, can I get a "Tested-by:"? thanks, greg k-h
Hi, On 11/30/20 2:53 PM, Greg KH wrote: > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: >> Hi, >> >> On 11/30/20 2:30 PM, Greg KH wrote: >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 11/30/20 1:58 PM, Tom Yan wrote: >>>>> It's merely a moving of comment moving for/and a no-behavioral-change >>>>> adaptation for the reversion.> >>>> >>>> IMHO the revert of the troublesome commit and the other/new changes really >>>> should be 2 separate commits. But I will let Alan and Greg have the final >>>> verdict on this. >>> >>> I would prefer to just revert the commits and not do anything >>> different/special here so late in the release cycle. >>> >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the >>> commit ids for them. >> >> The troublesome commit are (in reverse, so revert, order): >> >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") >> 558033c2828f ("uas: fix sdev->host->dma_dev") >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") >> >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the >> last 2 patches do, with the dmadev argument of that call pointing to the device >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller >> itself which is causing regressions in 5.10, see this email thread: >> >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > Thanks, I'll wait for Alan to respond, but I think just reverting these > is the best solution at this point in time. You have tested those > reverts, solve this, right? If so, can I get a "Tested-by:"? Yes that was my first solution to this problem and I can confirm that that fixes the regression: Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans
On Mon, 30 Nov 2020 at 21:23, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 11/30/20 1:58 PM, Tom Yan wrote: > > IMHO the revert of the troublesome commit and the other/new changes really > should be 2 separate commits. But I will let Alan and Greg have the final > verdict on this. They are not "other/new" changes. The same thing was done in the earlier version of the problematic commits, before I was given the idea that we can/should set dma_dev to the "chosen device". With the dma_dev setting approach the exact same clamping will be applied twice at different points so we don't have to invalidate the earlier one. But now since we no longer do so, the two clamping are / can be different, so we need to invalidate the earlier one when we are not overriding the default max_sectors in each case. > > p.s. Why did you not send this patch-series to Alan Stern, the maintainer of > the usb-storage driver? Either I accidentally missed him or the list I copied from did that already. Sorry. > > > Similar has been done in the equivalent > > patch for the UAS driver (and the reason is stated there). > > In the UAS driver the code setting max-hw-sectors was already moved to its > new place and another patch was added on top, so that is different. If you are referring to the alloc/configure move in the problematic commit, it was a trivial / code consistency change. It has nothing to do with what I'm talking about. What I'm talking about is the else-clause add in the first patch of the current series. I don't know if you simply missed it or it just seemed much trivial to you, either way it was "simple" there merely because the uas driver doesn't set its own "default" max_sectors (and hence has no comment for it) but use the one set in the SCSI layer. Most importantly, it's an *adapation* that makes these patches change *nothing* from the current behaviour but only switches back to scsi_add_host(). > > Regards, > > Hans > > > > > > On Mon, 30 Nov 2020 at 17:50, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi, > >> > >> On 11/28/20 4:48 PM, Tom Yan wrote: > >>> While the change only seemed to have caused issue on uas drives, we > >>> probably want to avoid it in the usb-storage driver as well, until > >>> we are sure it is the right thing to do. > >>> > >>> Signed-off-by: Tom Yan <tom.ty89@gmail.com> > >> > >> This seems to do a whole lot more then just dropping back from > >> scsi_add_host_with_dma() to scsi_add_host(). This has way more > >> lines then the orginal commit. > >> > >> IMHO it would be best to just revert commit 0154012f8018bba4d9971d1007c12ffd48539ddb > >> and then submit these changes as a separate patch (which would be > >> 5.11 material then). > >> > >> That separate patch could then also have a proper commit message > >> explaining the other changes you are making, which is also not > >> unimportant. > >> > >> Regards, > >> > >> Hans > >> > >> > >> > >> > >>> --- > >>> drivers/usb/storage/scsiglue.c | 40 +++++++++++++++++----------------- > >>> drivers/usb/storage/usb.c | 3 +-- > >>> 2 files changed, 21 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > >>> index 560efd1479ba..6539bae1c188 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 = sdev->host->dma_dev; > >>> + struct device *dev = us->pusb_dev->bus->sysdev; > >>> > >>> /* > >>> * Many devices have trouble transferring more than 32KB at a time, > >>> @@ -120,6 +120,25 @@ static int slave_configure(struct scsi_device *sdev) > >>> * better throughput on most devices. > >>> */ > >>> blk_queue_max_hw_sectors(sdev->request_queue, 2048); > >>> + } else { > >>> + /* > >>> + * 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. > >>> + */ > >>> + blk_queue_max_hw_sectors(sdev->request_queue, 240); > >>> } > >>> > >>> /* > >>> @@ -627,25 +646,6 @@ static const struct scsi_host_template usb_stor_host_template = { > >>> .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/usb.c b/drivers/usb/storage/usb.c > >>> index c2ef367cf257..f177da4ff1bc 100644 > >>> --- a/drivers/usb/storage/usb.c > >>> +++ b/drivers/usb/storage/usb.c > >>> @@ -1050,8 +1050,7 @@ int usb_stor_probe2(struct us_data *us) > >>> usb_autopm_get_interface_no_resume(us->pusb_intf); > >>> snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", > >>> dev_name(dev)); > >>> - result = scsi_add_host_with_dma(us_to_host(us), dev, > >>> - us->pusb_dev->bus->sysdev); > >>> + result = scsi_add_host(us_to_host(us), dev); > >>> if (result) { > >>> dev_warn(dev, > >>> "Unable to add the scsi host\n"); > >>> > >> > > >
On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > Hi, > > On 11/30/20 2:30 PM, Greg KH wrote: > > On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > >> Hi, > >> > >> On 11/30/20 1:58 PM, Tom Yan wrote: > >>> It's merely a moving of comment moving for/and a no-behavioral-change > >>> adaptation for the reversion.> > >> > >> IMHO the revert of the troublesome commit and the other/new changes really > >> should be 2 separate commits. But I will let Alan and Greg have the final > >> verdict on this. > > > > I would prefer to just revert the commits and not do anything > > different/special here so late in the release cycle. > > > > So, if Alan agrees, I'll be glad to do them on my end, I just need the > > commit ids for them. > > The troublesome commit are (in reverse, so revert, order): > > 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > 558033c2828f ("uas: fix sdev->host->dma_dev") > 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > > Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > last 2 patches do, with the dmadev argument of that call pointing to the device > for the XHCI controller is causing changes to the DMA settings of the XHCI controller > itself which is causing regressions in 5.10, see this email thread: > > https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t It's hard to go wrong with reverting, so it's okay with me. Still, Hans, have you checked out the difference between the scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter of using dev vs. sysdev. In particular, have you checked to see what those two devices are on your system? It seems likely that if one of those calls messes up some DMA settings, the other one does too -- just maybe not settings that matter much. Alan Stern
On Mon, Nov 30, 2020 at 12:20:04PM -0500, Alan Stern wrote: > > https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > It's hard to go wrong with reverting, so it's okay with me. > > Still, Hans, have you checked out the difference between the > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter > of using dev vs. sysdev. In particular, have you checked to see what > those two devices are on your system? > > It seems likely that if one of those calls messes up some DMA settings, > the other one does too -- just maybe not settings that matter much. The effects from scsi_add_host_with_dma should be: (1) picking which device is used for the SCSI dma map helpers (2) use dma_max_mapping_size() to limite the I/O size The helpers affected by (1) are not used by UAS (or usb-storage for that matter), while we do have a real bug in the intel-iommu with bounce buffering implementation used in the bug report. So my clear bet is on (2) not limiting the size, but the patch that would have fixed this did not make a different for Hans, which leaves me a little confused.
Hi, On 11/30/20 6:20 PM, Alan Stern wrote: > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: >> Hi, >> >> On 11/30/20 2:30 PM, Greg KH wrote: >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 11/30/20 1:58 PM, Tom Yan wrote: >>>>> It's merely a moving of comment moving for/and a no-behavioral-change >>>>> adaptation for the reversion.> >>>> >>>> IMHO the revert of the troublesome commit and the other/new changes really >>>> should be 2 separate commits. But I will let Alan and Greg have the final >>>> verdict on this. >>> >>> I would prefer to just revert the commits and not do anything >>> different/special here so late in the release cycle. >>> >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the >>> commit ids for them. >> >> The troublesome commit are (in reverse, so revert, order): >> >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") >> 558033c2828f ("uas: fix sdev->host->dma_dev") >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") >> >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the >> last 2 patches do, with the dmadev argument of that call pointing to the device >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller >> itself which is causing regressions in 5.10, see this email thread: >> >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > It's hard to go wrong with reverting, so it's okay with me. > > Still, Hans, have you checked out the difference between the > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter > of using dev vs. sysdev. In particular, have you checked to see what > those two devices are on your system? Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume that the latter is actually the XHCI controller. my vote goes to reverting to avoid the regression for 5.10, esp. since this is a clean revert of 3 patches with nothing depending / building on top of the reverted commits. Then for 5.11 we can retry to introduce similar changes. I would be happy to try a new patch-set for 5.11. > It seems likely that if one of those calls messes up some DMA settings, > the other one does too -- just maybe not settings that matter much. I'm not very familiar with all the DMA mapping / mask code, but AFAIK making changes to the DMA settings of a child will not influence the parent. Where as when passing bus->sysdev, then changes are made to a device which is shared with other devices on the bus, which is why we see a regression in an USB NIC driver being triggered by the UAS driver binding to a device (on the same bus). At least that is my interpretation of this. I bisected the regression and that pointed at the UAS DMA change and reverting it fixes things, confirming that I did not make any mistakes during the bisect. Regards, Hans
This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816 UAS: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918 BOT (AFAICT): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466 It would explain why the issue is only triggered with UAS drives. The questions (from me) are: 1. From the scsi layer POV (as per what __scsi_init_queue() does), what/which should we use as dma_dev? 2. Do we really need to set dma_boundary in the UAS host template (to PAGE_SIZE - 1)? 3. Kind of the same question as #1: when we clamp hw_max_sectors to dma max mapping size, should the size actually be "the smaller one among dev and sysdev"? Or is one of the two sizes *always* the smaller one? On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 11/30/20 6:20 PM, Alan Stern wrote: > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > >> Hi, > >> > >> On 11/30/20 2:30 PM, Greg KH wrote: > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > >>>> Hi, > >>>> > >>>> On 11/30/20 1:58 PM, Tom Yan wrote: > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change > >>>>> adaptation for the reversion.> > >>>> > >>>> IMHO the revert of the troublesome commit and the other/new changes really > >>>> should be 2 separate commits. But I will let Alan and Greg have the final > >>>> verdict on this. > >>> > >>> I would prefer to just revert the commits and not do anything > >>> different/special here so late in the release cycle. > >>> > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the > >>> commit ids for them. > >> > >> The troublesome commit are (in reverse, so revert, order): > >> > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > >> 558033c2828f ("uas: fix sdev->host->dma_dev") > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > >> > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > >> last 2 patches do, with the dmadev argument of that call pointing to the device > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller > >> itself which is causing regressions in 5.10, see this email thread: > >> > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > > > It's hard to go wrong with reverting, so it's okay with me. > > > > Still, Hans, have you checked out the difference between the > > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter > > of using dev vs. sysdev. In particular, have you checked to see what > > those two devices are on your system? > > Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume > that the latter is actually the XHCI controller. > > my vote goes to reverting to avoid the regression for 5.10, esp. since > this is a clean revert of 3 patches with nothing depending / building > on top of the reverted commits. > > Then for 5.11 we can retry to introduce similar changes. I would be happy > to try a new patch-set for 5.11. > > > It seems likely that if one of those calls messes up some DMA settings, > > the other one does too -- just maybe not settings that matter much. > > I'm not very familiar with all the DMA mapping / mask code, but AFAIK making > changes to the DMA settings of a child will not influence the parent. > > Where as when passing bus->sysdev, then changes are made to a device > which is shared with other devices on the bus, which is why we see > a regression in an USB NIC driver being triggered by the UAS driver > binding to a device (on the same bus). > > At least that is my interpretation of this. I bisected the regression > and that pointed at the UAS DMA change and reverting it fixes things, > confirming that I did not make any mistakes during the bisect. > > Regards, > > Hans >
For the record, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/scsi/scsi_host.h?h=v5.10-rc6#n753 On Tue, 1 Dec 2020 at 02:57, Tom Yan <tom.ty89@gmail.com> wrote: > > This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816 > > UAS: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918 > BOT (AFAICT): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466 > > It would explain why the issue is only triggered with UAS drives. > > The questions (from me) are: > 1. From the scsi layer POV (as per what __scsi_init_queue() does), > what/which should we use as dma_dev? > 2. Do we really need to set dma_boundary in the UAS host template (to > PAGE_SIZE - 1)? > 3. Kind of the same question as #1: when we clamp hw_max_sectors to > dma max mapping size, should the size actually be "the smaller one > among dev and sysdev"? Or is one of the two sizes *always* the smaller > one? > > > On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi, > > > > On 11/30/20 6:20 PM, Alan Stern wrote: > > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > > >> Hi, > > >> > > >> On 11/30/20 2:30 PM, Greg KH wrote: > > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > > >>>> Hi, > > >>>> > > >>>> On 11/30/20 1:58 PM, Tom Yan wrote: > > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change > > >>>>> adaptation for the reversion.> > > >>>> > > >>>> IMHO the revert of the troublesome commit and the other/new changes really > > >>>> should be 2 separate commits. But I will let Alan and Greg have the final > > >>>> verdict on this. > > >>> > > >>> I would prefer to just revert the commits and not do anything > > >>> different/special here so late in the release cycle. > > >>> > > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the > > >>> commit ids for them. > > >> > > >> The troublesome commit are (in reverse, so revert, order): > > >> > > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > > >> 558033c2828f ("uas: fix sdev->host->dma_dev") > > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > > >> > > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > > >> last 2 patches do, with the dmadev argument of that call pointing to the device > > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller > > >> itself which is causing regressions in 5.10, see this email thread: > > >> > > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > > > > > It's hard to go wrong with reverting, so it's okay with me. > > > > > > Still, Hans, have you checked out the difference between the > > > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter > > > of using dev vs. sysdev. In particular, have you checked to see what > > > those two devices are on your system? > > > > Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume > > that the latter is actually the XHCI controller. > > > > my vote goes to reverting to avoid the regression for 5.10, esp. since > > this is a clean revert of 3 patches with nothing depending / building > > on top of the reverted commits. > > > > Then for 5.11 we can retry to introduce similar changes. I would be happy > > to try a new patch-set for 5.11. > > > > > It seems likely that if one of those calls messes up some DMA settings, > > > the other one does too -- just maybe not settings that matter much. > > > > I'm not very familiar with all the DMA mapping / mask code, but AFAIK making > > changes to the DMA settings of a child will not influence the parent. > > > > Where as when passing bus->sysdev, then changes are made to a device > > which is shared with other devices on the bus, which is why we see > > a regression in an USB NIC driver being triggered by the UAS driver > > binding to a device (on the same bus). > > > > At least that is my interpretation of this. I bisected the regression > > and that pointed at the UAS DMA change and reverting it fixes things, > > confirming that I did not make any mistakes during the bisect. > > > > Regards, > > > > Hans > >
[Added linux-scsi to CC: list. When discussing code in a particular subsystem, it's a good idea to include that subsystem's mailing list in the CC:.] On Tue, Dec 01, 2020 at 03:01:56AM +0800, Tom Yan wrote: > For the record, > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/scsi/scsi_host.h?h=v5.10-rc6#n753 > > On Tue, 1 Dec 2020 at 02:57, Tom Yan <tom.ty89@gmail.com> wrote: > > > > This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816 > > > > UAS: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918 > > BOT (AFAICT): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466 > > > > It would explain why the issue is only triggered with UAS drives. In brief, a recent change -- calling scsi_add_host_with_dma rather than scsi_add_host -- in the USB uas driver has caused a regression in performance. (Note that the shost->dma_dev value is set differently as a result of this change.) Hans has determined that the problem seems to be related to permanent changes in the dma_dev's settings caused by scsi_add_host_with_dma. Tom pointed out that __scsi_init_queue contains a couple of questionable assignments: dma_set_seg_boundary(dev, shost->dma_boundary); and dma_set_max_seg_size(dev, queue_max_segment_size(q)); where dev = shost->dma_dev -- in this case, a USB host controller. So an important question is why decisions related to a particular SCSI host should affect the DMA settings of a device somewhere else in the heirarchy? Sure, the properties of the USB controller should constrain the settings available to the SCSI host, but there doesn't seem to be any good reason for restrictions to go in the other direction. Doesn't the way we handle DMA permit a child device to impose additional restrictions (such as a smaller max segment size) beyond those of the parent device which actually performs the DMA transfer? > > The questions (from me) are: > > 1. From the scsi layer POV (as per what __scsi_init_queue() does), > > what/which should we use as dma_dev? We should be using the USB host controller, because it is the device which actually performs the DMA transfers. > > 2. Do we really need to set dma_boundary in the UAS host template (to > > PAGE_SIZE - 1)? I don't know. But in theory it should be possible to have settings (like this one) which affect only the transfers carried out by the SCSI host, not the transfers carred out by other drivers which might use the same USB controller. > > 3. Kind of the same question as #1: when we clamp hw_max_sectors to > > dma max mapping size, should the size actually be "the smaller one > > among dev and sysdev"? Or is one of the two sizes *always* the smaller > > one? I assume you're referring to code in the uas driver. There the value of dev is meaningless as far as DMA is concerned. Only sysdev matters. Alan Stern > > On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > Hi, > > > > > > On 11/30/20 6:20 PM, Alan Stern wrote: > > > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > > > >> Hi, > > > >> > > > >> On 11/30/20 2:30 PM, Greg KH wrote: > > > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > > > >>>> Hi, > > > >>>> > > > >>>> On 11/30/20 1:58 PM, Tom Yan wrote: > > > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change > > > >>>>> adaptation for the reversion.> > > > >>>> > > > >>>> IMHO the revert of the troublesome commit and the other/new changes really > > > >>>> should be 2 separate commits. But I will let Alan and Greg have the final > > > >>>> verdict on this. > > > >>> > > > >>> I would prefer to just revert the commits and not do anything > > > >>> different/special here so late in the release cycle. > > > >>> > > > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the > > > >>> commit ids for them. > > > >> > > > >> The troublesome commit are (in reverse, so revert, order): > > > >> > > > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > > > >> 558033c2828f ("uas: fix sdev->host->dma_dev") > > > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > > > >> > > > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > > > >> last 2 patches do, with the dmadev argument of that call pointing to the device > > > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller > > > >> itself which is causing regressions in 5.10, see this email thread: > > > >> > > > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > > > > > > > It's hard to go wrong with reverting, so it's okay with me. > > > > > > > > Still, Hans, have you checked out the difference between the > > > > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter > > > > of using dev vs. sysdev. In particular, have you checked to see what > > > > those two devices are on your system? > > > > > > Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume > > > that the latter is actually the XHCI controller. > > > > > > my vote goes to reverting to avoid the regression for 5.10, esp. since > > > this is a clean revert of 3 patches with nothing depending / building > > > on top of the reverted commits. > > > > > > Then for 5.11 we can retry to introduce similar changes. I would be happy > > > to try a new patch-set for 5.11. > > > > > > > It seems likely that if one of those calls messes up some DMA settings, > > > > the other one does too -- just maybe not settings that matter much. > > > > > > I'm not very familiar with all the DMA mapping / mask code, but AFAIK making > > > changes to the DMA settings of a child will not influence the parent. > > > > > > Where as when passing bus->sysdev, then changes are made to a device > > > which is shared with other devices on the bus, which is why we see > > > a regression in an USB NIC driver being triggered by the UAS driver > > > binding to a device (on the same bus). > > > > > > At least that is my interpretation of this. I bisected the regression > > > and that pointed at the UAS DMA change and reverting it fixes things, > > > confirming that I did not make any mistakes during the bisect. > > > > > > Regards, > > > > > > Hans > > >
On Mon, Nov 30, 2020 at 02:55:45PM +0100, Hans de Goede wrote: > Hi, > > On 11/30/20 2:53 PM, Greg KH wrote: > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > >> Hi, > >> > >> On 11/30/20 2:30 PM, Greg KH wrote: > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > >>>> Hi, > >>>> > >>>> On 11/30/20 1:58 PM, Tom Yan wrote: > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change > >>>>> adaptation for the reversion.> > >>>> > >>>> IMHO the revert of the troublesome commit and the other/new changes really > >>>> should be 2 separate commits. But I will let Alan and Greg have the final > >>>> verdict on this. > >>> > >>> I would prefer to just revert the commits and not do anything > >>> different/special here so late in the release cycle. > >>> > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the > >>> commit ids for them. > >> > >> The troublesome commit are (in reverse, so revert, order): > >> > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > >> 558033c2828f ("uas: fix sdev->host->dma_dev") > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > >> > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > >> last 2 patches do, with the dmadev argument of that call pointing to the device > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller > >> itself which is causing regressions in 5.10, see this email thread: > >> > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > > > Thanks, I'll wait for Alan to respond, but I think just reverting these > > is the best solution at this point in time. You have tested those > > reverts, solve this, right? If so, can I get a "Tested-by:"? > > Yes that was my first solution to this problem and I can confirm that that fixes > the regression: > > Tested-by: Hans de Goede <hdegoede@redhat.com> All now reverted. thanks, greg k-h
This thread seems to have fallen through the cracks. Maybe now would be a good time to address the problem (since we originally planned to fix it in 5.11!). The questions listed below are pretty self-contained, although the rest of the discussion isn't. But I never received any answers. Alan Stern On Mon, Nov 30, 2020 at 03:36:18PM -0500, Alan Stern wrote: > [Added linux-scsi to CC: list. When discussing code in a particular > subsystem, it's a good idea to include that subsystem's mailing list in > the CC:.] > > On Tue, Dec 01, 2020 at 03:01:56AM +0800, Tom Yan wrote: > > For the record, > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/scsi/scsi_host.h?h=v5.10-rc6#n753 > > > > On Tue, 1 Dec 2020 at 02:57, Tom Yan <tom.ty89@gmail.com> wrote: > > > > > > This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816 > > > > > > UAS: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918 > > > BOT (AFAICT): > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466 > > > > > > It would explain why the issue is only triggered with UAS drives. > > In brief, a recent change -- calling scsi_add_host_with_dma rather than > scsi_add_host -- in the USB uas driver has caused a regression in > performance. (Note that the shost->dma_dev value is set differently as > a result of this change.) Hans has determined that the problem seems > to be related to permanent changes in the dma_dev's settings caused by > scsi_add_host_with_dma. > > Tom pointed out that __scsi_init_queue contains a couple of questionable > assignments: > > dma_set_seg_boundary(dev, shost->dma_boundary); > > and > > dma_set_max_seg_size(dev, queue_max_segment_size(q)); > > where dev = shost->dma_dev -- in this case, a USB host controller. > > So an important question is why decisions related to a particular SCSI > host should affect the DMA settings of a device somewhere else in the > heirarchy? Sure, the properties of the USB controller should constrain > the settings available to the SCSI host, but there doesn't seem to be > any good reason for restrictions to go in the other direction. > > Doesn't the way we handle DMA permit a child device to impose additional > restrictions (such as a smaller max segment size) beyond those of the > parent device which actually performs the DMA transfer? > > > > The questions (from me) are: > > > 1. From the scsi layer POV (as per what __scsi_init_queue() does), > > > what/which should we use as dma_dev? > > We should be using the USB host controller, because it is the device > which actually performs the DMA transfers. > > > > 2. Do we really need to set dma_boundary in the UAS host template (to > > > PAGE_SIZE - 1)? > > I don't know. But in theory it should be possible to have settings > (like this one) which affect only the transfers carried out by the SCSI > host, not the transfers carred out by other drivers which might use the > same USB controller. > > > > 3. Kind of the same question as #1: when we clamp hw_max_sectors to > > > dma max mapping size, should the size actually be "the smaller one > > > among dev and sysdev"? Or is one of the two sizes *always* the smaller > > > one? > > I assume you're referring to code in the uas driver. There the value of > dev is meaningless as far as DMA is concerned. Only sysdev matters. > > Alan Stern > > > > On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > > > Hi, > > > > > > > > On 11/30/20 6:20 PM, Alan Stern wrote: > > > > > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote: > > > > >> Hi, > > > > >> > > > > >> On 11/30/20 2:30 PM, Greg KH wrote: > > > > >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote: > > > > >>>> Hi, > > > > >>>> > > > > >>>> On 11/30/20 1:58 PM, Tom Yan wrote: > > > > >>>>> It's merely a moving of comment moving for/and a no-behavioral-change > > > > >>>>> adaptation for the reversion.> > > > > >>>> > > > > >>>> IMHO the revert of the troublesome commit and the other/new changes really > > > > >>>> should be 2 separate commits. But I will let Alan and Greg have the final > > > > >>>> verdict on this. > > > > >>> > > > > >>> I would prefer to just revert the commits and not do anything > > > > >>> different/special here so late in the release cycle. > > > > >>> > > > > >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the > > > > >>> commit ids for them. > > > > >> > > > > >> The troublesome commit are (in reverse, so revert, order): > > > > >> > > > > >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives") > > > > >> 558033c2828f ("uas: fix sdev->host->dma_dev") > > > > >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev") > > > > >> > > > > >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the > > > > >> last 2 patches do, with the dmadev argument of that call pointing to the device > > > > >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller > > > > >> itself which is causing regressions in 5.10, see this email thread: > > > > >> > > > > >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t > > > > > > > > > > It's hard to go wrong with reverting, so it's okay with me. > > > > > > > > > > Still, Hans, have you checked out the difference between the > > > > > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter > > > > > of using dev vs. sysdev. In particular, have you checked to see what > > > > > those two devices are on your system? > > > > > > > > Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume > > > > that the latter is actually the XHCI controller. > > > > > > > > my vote goes to reverting to avoid the regression for 5.10, esp. since > > > > this is a clean revert of 3 patches with nothing depending / building > > > > on top of the reverted commits. > > > > > > > > Then for 5.11 we can retry to introduce similar changes. I would be happy > > > > to try a new patch-set for 5.11. > > > > > > > > > It seems likely that if one of those calls messes up some DMA settings, > > > > > the other one does too -- just maybe not settings that matter much. > > > > > > > > I'm not very familiar with all the DMA mapping / mask code, but AFAIK making > > > > changes to the DMA settings of a child will not influence the parent. > > > > > > > > Where as when passing bus->sysdev, then changes are made to a device > > > > which is shared with other devices on the bus, which is why we see > > > > a regression in an USB NIC driver being triggered by the UAS driver > > > > binding to a device (on the same bus). > > > > > > > > At least that is my interpretation of this. I bisected the regression > > > > and that pointed at the UAS DMA change and reverting it fixes things, > > > > confirming that I did not make any mistakes during the bisect. > > > > > > > > Regards, > > > > > > > > Hans > > > >
On Thu, Feb 25, 2021 at 11:35:57AM -0500, Alan Stern wrote: > This thread seems to have fallen through the cracks. Maybe now would be > a good time to address the problem (since we originally planned to fix > it in 5.11!). > > The questions listed below are pretty self-contained, although the rest > of the discussion isn't. But I never received any answers. usb-storage must use scsi_add_host_with_dma to use the right device for DMA mapping and parameters. The calls to set the DMA options on the device are needed so that IOMMU merging doesn't change the imposed requirements. If these requirements slow you down you need to relax them, as apparently the hardware is able to handle bigger limits.
On Fri, Feb 26, 2021 at 06:53:52AM +0100, Christoph Hellwig wrote: > On Thu, Feb 25, 2021 at 11:35:57AM -0500, Alan Stern wrote: > > This thread seems to have fallen through the cracks. Maybe now would be > > a good time to address the problem (since we originally planned to fix > > it in 5.11!). > > > > The questions listed below are pretty self-contained, although the rest > > of the discussion isn't. But I never received any answers. > > usb-storage must use scsi_add_host_with_dma to use the right device > for DMA mapping and parameters. The calls to set the DMA options > on the device are needed so that IOMMU merging doesn't change the > imposed requirements. If these requirements slow you down you need > to relax them, as apparently the hardware is able to handle bigger > limits. Hans, don't you have the right equipment to test this approach? Alan Stern
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index 560efd1479ba..6539bae1c188 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 = sdev->host->dma_dev; + struct device *dev = us->pusb_dev->bus->sysdev; /* * Many devices have trouble transferring more than 32KB at a time, @@ -120,6 +120,25 @@ static int slave_configure(struct scsi_device *sdev) * better throughput on most devices. */ blk_queue_max_hw_sectors(sdev->request_queue, 2048); + } else { + /* + * 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. + */ + blk_queue_max_hw_sectors(sdev->request_queue, 240); } /* @@ -627,25 +646,6 @@ static const struct scsi_host_template usb_stor_host_template = { .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/usb.c b/drivers/usb/storage/usb.c index c2ef367cf257..f177da4ff1bc 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -1050,8 +1050,7 @@ int usb_stor_probe2(struct us_data *us) usb_autopm_get_interface_no_resume(us->pusb_intf); snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", dev_name(dev)); - result = scsi_add_host_with_dma(us_to_host(us), dev, - us->pusb_dev->bus->sysdev); + result = scsi_add_host(us_to_host(us), dev); if (result) { dev_warn(dev, "Unable to add the scsi host\n");
While the change only seemed to have caused issue on uas drives, we probably want to avoid it in the usb-storage driver as well, until we are sure it is the right thing to do. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/scsiglue.c | 40 +++++++++++++++++----------------- drivers/usb/storage/usb.c | 3 +-- 2 files changed, 21 insertions(+), 22 deletions(-)