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 <[email protected]>
---
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");
--
2.29.2
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>
>> 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/[email protected]/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/[email protected]/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/[email protected]/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 <[email protected]>
Regards,
Hans
On Mon, 30 Nov 2020 at 21:23, Hans de Goede <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>
> >> 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/[email protected]/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/[email protected]/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/[email protected]/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 <[email protected]> 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/[email protected]/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 <[email protected]> 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 <[email protected]> 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/[email protected]/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 <[email protected]> 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 <[email protected]> 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/[email protected]/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/[email protected]/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 <[email protected]>
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 <[email protected]> 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 <[email protected]> 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/[email protected]/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