| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 30 Nov 2020 14:23:48 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Tom Yan <tom.ty89@...il.com>,
Alan Stern <stern@...land.harvard.edu>
Cc: Christoph Hellwig <hch@....de>,
Greg KH <gregkh@...uxfoundation.org>,
linux-usb <linux-usb@...r.kernel.org>,
Mathias Nyman <mathias.nyman@...el.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-pci@...r.kernel.org, Lu Baolu <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to
scsi_add_host()
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@...hat.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@...il.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");
>>>
>>
>
Powered by blists - more mailing lists