Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

[Hans de Goede <hdegoede@redhat.com>]

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");
>>>
>>
>