* [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping @ 2020-09-01 5:54 Tom Yan 2020-09-01 14:55 ` Alan Stern 0 siblings, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-01 5:54 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, Tom Yan When the scsi request queue is initialized/allocated, the scsi driver clamps hw_max_sectors against the dma max mapping size of sdev->host->dma_dev. The clamping is apparently inappriorate to USB drives. Either way we are calling blk_queue_max_hw_sectors() in the usb drivers for some (but not all) cases, which causes the clamping to be overriden (inconsistently) anyway. Therefore the usb driver should always set hw_max_sectors and do the clamping against the right device itself. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------ drivers/usb/storage/uas.c | 23 ++++++++++++++++----- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index e5a971b83e3f..804cbc0ba4da 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev) * better throughput on most devices. */ blk_queue_max_hw_sectors(sdev->request_queue, 2048); + } else { + /* + * 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); } /* @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = { /* lots of sg segments can be handled */ .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/uas.c b/drivers/usb/storage/uas.c index 08f9296431e9..cffa435afd84 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) */ blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); - if (devinfo->flags & US_FL_MAX_SECTORS_64) - blk_queue_max_hw_sectors(sdev->request_queue, 64); - else if (devinfo->flags & US_FL_MAX_SECTORS_240) - blk_queue_max_hw_sectors(sdev->request_queue, 240); - return 0; } @@ -839,6 +834,24 @@ static int uas_slave_configure(struct scsi_device *sdev) { struct uas_dev_info *devinfo = sdev->hostdata; + struct us_data *us = host_to_us(sdev->host); + struct device *dev = us->pusb_dev->bus->sysdev; + + if (devinfo->flags & US_FL_MAX_SECTORS_64) + blk_queue_max_hw_sectors(sdev->request_queue, 64); + else if (devinfo->flags & US_FL_MAX_SECTORS_240) + blk_queue_max_hw_sectors(sdev->request_queue, 240); + else + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS); + + /* + * The max_hw_sectors should be up to maximum size of a mapping for + * the device. Otherwise, a DMA API might fail on swiotlb environment. + */ + blk_queue_max_hw_sectors(sdev->request_queue, + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); + if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping 2020-09-01 5:54 [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan @ 2020-09-01 14:55 ` Alan Stern 2020-09-01 23:24 ` Tom Yan 2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 0 siblings, 2 replies; 24+ messages in thread From: Alan Stern @ 2020-09-01 14:55 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, Yoshihiro Shimoda Patch submissions should have text lines limited to fewer than 80 columns. On Tue, Sep 01, 2020 at 01:54:17PM +0800, Tom Yan wrote: > When the scsi request queue is initialized/allocated, the scsi driver > clamps hw_max_sectors against the dma max mapping size of > sdev->host->dma_dev. The clamping is apparently inappriorate to USB > drives. Wouldn't it be more accurate to say that the clamping _is_ appropriate, but it should be performed using the sysdev device rather than the nominal parent? Thus the error lies in allowing shost->dma_dev to be set incorrectly. > Either way we are calling blk_queue_max_hw_sectors() in the usb > drivers for some (but not all) cases, which causes the clamping to be > overriden (inconsistently) anyway. > > Therefore the usb driver should always set hw_max_sectors and do the > clamping against the right device itself. How about fixing the dma_dev assignment instead? Alan Stern > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > --- > drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------ > drivers/usb/storage/uas.c | 23 ++++++++++++++++----- > 2 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index e5a971b83e3f..804cbc0ba4da 100644 > --- a/drivers/usb/storage/scsiglue.c > +++ b/drivers/usb/storage/scsiglue.c > @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev) > * better throughput on most devices. > */ > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > + } else { > + /* > + * 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); > } > > /* > @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = { > /* lots of sg segments can be handled */ > .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/uas.c b/drivers/usb/storage/uas.c > index 08f9296431e9..cffa435afd84 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) > */ > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > - if (devinfo->flags & US_FL_MAX_SECTORS_64) > - blk_queue_max_hw_sectors(sdev->request_queue, 64); > - else if (devinfo->flags & US_FL_MAX_SECTORS_240) > - blk_queue_max_hw_sectors(sdev->request_queue, 240); > - > return 0; > } > > @@ -839,6 +834,24 @@ static int uas_slave_configure(struct scsi_device *sdev) > { > struct uas_dev_info *devinfo = sdev->hostdata; > > + struct us_data *us = host_to_us(sdev->host); > + struct device *dev = us->pusb_dev->bus->sysdev; > + > + if (devinfo->flags & US_FL_MAX_SECTORS_64) > + blk_queue_max_hw_sectors(sdev->request_queue, 64); > + else if (devinfo->flags & US_FL_MAX_SECTORS_240) > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > + else > + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS); > + > + /* > + * The max_hw_sectors should be up to maximum size of a mapping for > + * the device. Otherwise, a DMA API might fail on swiotlb environment. > + */ > + blk_queue_max_hw_sectors(sdev->request_queue, > + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); > + > if (devinfo->flags & US_FL_NO_REPORT_OPCODES) > sdev->no_report_opcodes = 1; > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping 2020-09-01 14:55 ` Alan Stern @ 2020-09-01 23:24 ` Tom Yan 2020-09-01 23:44 ` Tom Yan 2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 1 sibling, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-01 23:24 UTC (permalink / raw) To: Alan Stern; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda On Tue, 1 Sep 2020 at 22:55, Alan Stern <stern@rowland.harvard.edu> wrote: > > Patch submissions should have text lines limited to fewer than 80 > columns. > > On Tue, Sep 01, 2020 at 01:54:17PM +0800, Tom Yan wrote: > > When the scsi request queue is initialized/allocated, the scsi driver > > clamps hw_max_sectors against the dma max mapping size of > > sdev->host->dma_dev. The clamping is apparently inappriorate to USB > > drives. > > Wouldn't it be more accurate to say that the clamping _is_ appropriate, > but it should be performed using the sysdev device rather than the > nominal parent? Thus the error lies in allowing shost->dma_dev to be > set incorrectly. > > > Either way we are calling blk_queue_max_hw_sectors() in the usb > > drivers for some (but not all) cases, which causes the clamping to be > > overriden (inconsistently) anyway. > > > > Therefore the usb driver should always set hw_max_sectors and do the > > clamping against the right device itself. > > How about fixing the dma_dev assignment instead? That won't really help unless we also do the clamping on the USB side *conditionally* (i.e. only after when we call `blk_queue_max_hw_sectors()` again there). I'm not sure there's a good reason for doing so. Although this might seem a bit redundant/clumsy at first glance, it's also much easier to follow (e.g. we don't have to dive into the SCSI side to know where the hw_max_sectors came from). Besides, the whole DMA thing is quite beyond my knowledge. I'm going to fix the commit message, and use 2048 blocks for SS UAS drives. > > Alan Stern > > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > --- > > drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------ > > drivers/usb/storage/uas.c | 23 ++++++++++++++++----- > > 2 files changed, 35 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > > index e5a971b83e3f..804cbc0ba4da 100644 > > --- a/drivers/usb/storage/scsiglue.c > > +++ b/drivers/usb/storage/scsiglue.c > > @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev) > > * better throughput on most devices. > > */ > > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > > + } else { > > + /* > > + * 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); > > } > > > > /* > > @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = { > > /* lots of sg segments can be handled */ > > .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/uas.c b/drivers/usb/storage/uas.c > > index 08f9296431e9..cffa435afd84 100644 > > --- a/drivers/usb/storage/uas.c > > +++ b/drivers/usb/storage/uas.c > > @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) > > */ > > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > > > - if (devinfo->flags & US_FL_MAX_SECTORS_64) > > - blk_queue_max_hw_sectors(sdev->request_queue, 64); > > - else if (devinfo->flags & US_FL_MAX_SECTORS_240) > > - blk_queue_max_hw_sectors(sdev->request_queue, 240); > > - > > return 0; > > } > > > > @@ -839,6 +834,24 @@ static int uas_slave_configure(struct scsi_device *sdev) > > { > > struct uas_dev_info *devinfo = sdev->hostdata; > > > > + struct us_data *us = host_to_us(sdev->host); > > + struct device *dev = us->pusb_dev->bus->sysdev; > > + > > + if (devinfo->flags & US_FL_MAX_SECTORS_64) > > + blk_queue_max_hw_sectors(sdev->request_queue, 64); > > + else if (devinfo->flags & US_FL_MAX_SECTORS_240) > > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > > + else > > + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS); > > + > > + /* > > + * The max_hw_sectors should be up to maximum size of a mapping for > > + * the device. Otherwise, a DMA API might fail on swiotlb environment. > > + */ > > + blk_queue_max_hw_sectors(sdev->request_queue, > > + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > > + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); > > + > > if (devinfo->flags & US_FL_NO_REPORT_OPCODES) > > sdev->no_report_opcodes = 1; > > > > -- > > 2.28.0 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping 2020-09-01 23:24 ` Tom Yan @ 2020-09-01 23:44 ` Tom Yan 2020-09-02 15:24 ` Alan Stern 0 siblings, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-01 23:44 UTC (permalink / raw) To: Alan Stern; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda Oh I think I see your point now. If/When dma_dev is fixed / the same as `bus->sysdev`, it wouldn't hurt / matter if the clamping is done twice at different stages. max_sectors will be clamped to the same value no matter if it's set on the SCSI side or the USB side. But again, fixing the dma_dev is beyond my knowledge. Probably need someone else to look into it. On Wed, 2 Sep 2020 at 07:24, Tom Yan <tom.ty89@gmail.com> wrote: > > On Tue, 1 Sep 2020 at 22:55, Alan Stern <stern@rowland.harvard.edu> wrote: > > > > Patch submissions should have text lines limited to fewer than 80 > > columns. > > > > On Tue, Sep 01, 2020 at 01:54:17PM +0800, Tom Yan wrote: > > > When the scsi request queue is initialized/allocated, the scsi driver > > > clamps hw_max_sectors against the dma max mapping size of > > > sdev->host->dma_dev. The clamping is apparently inappriorate to USB > > > drives. > > > > Wouldn't it be more accurate to say that the clamping _is_ appropriate, > > but it should be performed using the sysdev device rather than the > > nominal parent? Thus the error lies in allowing shost->dma_dev to be > > set incorrectly. > > > > > Either way we are calling blk_queue_max_hw_sectors() in the usb > > > drivers for some (but not all) cases, which causes the clamping to be > > > overriden (inconsistently) anyway. > > > > > > Therefore the usb driver should always set hw_max_sectors and do the > > > clamping against the right device itself. > > > > How about fixing the dma_dev assignment instead? > > That won't really help unless we also do the clamping on the USB side > *conditionally* (i.e. only after when we call > `blk_queue_max_hw_sectors()` again there). I'm not sure there's a good > reason for doing so. > > Although this might seem a bit redundant/clumsy at first glance, it's > also much easier to follow (e.g. we don't have to dive into the SCSI > side to know where the hw_max_sectors came from). > > Besides, the whole DMA thing is quite beyond my knowledge. > > I'm going to fix the commit message, and use 2048 blocks for SS UAS drives. > > > > > Alan Stern > > > > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > > --- > > > drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------ > > > drivers/usb/storage/uas.c | 23 ++++++++++++++++----- > > > 2 files changed, 35 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > > > index e5a971b83e3f..804cbc0ba4da 100644 > > > --- a/drivers/usb/storage/scsiglue.c > > > +++ b/drivers/usb/storage/scsiglue.c > > > @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev) > > > * better throughput on most devices. > > > */ > > > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > > > + } else { > > > + /* > > > + * 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); > > > } > > > > > > /* > > > @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = { > > > /* lots of sg segments can be handled */ > > > .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/uas.c b/drivers/usb/storage/uas.c > > > index 08f9296431e9..cffa435afd84 100644 > > > --- a/drivers/usb/storage/uas.c > > > +++ b/drivers/usb/storage/uas.c > > > @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) > > > */ > > > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > > > > > - if (devinfo->flags & US_FL_MAX_SECTORS_64) > > > - blk_queue_max_hw_sectors(sdev->request_queue, 64); > > > - else if (devinfo->flags & US_FL_MAX_SECTORS_240) > > > - blk_queue_max_hw_sectors(sdev->request_queue, 240); > > > - > > > return 0; > > > } > > > > > > @@ -839,6 +834,24 @@ static int uas_slave_configure(struct scsi_device *sdev) > > > { > > > struct uas_dev_info *devinfo = sdev->hostdata; > > > > > > + struct us_data *us = host_to_us(sdev->host); > > > + struct device *dev = us->pusb_dev->bus->sysdev; > > > + > > > + if (devinfo->flags & US_FL_MAX_SECTORS_64) > > > + blk_queue_max_hw_sectors(sdev->request_queue, 64); > > > + else if (devinfo->flags & US_FL_MAX_SECTORS_240) > > > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > > > + else > > > + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS); > > > + > > > + /* > > > + * The max_hw_sectors should be up to maximum size of a mapping for > > > + * the device. Otherwise, a DMA API might fail on swiotlb environment. > > > + */ > > > + blk_queue_max_hw_sectors(sdev->request_queue, > > > + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > > > + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); > > > + > > > if (devinfo->flags & US_FL_NO_REPORT_OPCODES) > > > sdev->no_report_opcodes = 1; > > > > > > -- > > > 2.28.0 > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping 2020-09-01 23:44 ` Tom Yan @ 2020-09-02 15:24 ` Alan Stern 0 siblings, 0 replies; 24+ messages in thread From: Alan Stern @ 2020-09-02 15:24 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda On Wed, Sep 02, 2020 at 07:44:42AM +0800, Tom Yan wrote: > Oh I think I see your point now. If/When dma_dev is fixed / the same > as `bus->sysdev`, it wouldn't hurt / matter if the clamping is done > twice at different stages. max_sectors will be clamped to the same > value no matter if it's set on the SCSI side or the USB side. > > But again, fixing the dma_dev is beyond my knowledge. Probably need > someone else to look into it. It's straightforward. All you have to do is convert the calls to scsi_add_host() in usb.c and uas.c. Make them call scsi_add_host_with_dma() instead. The extra third argument should be a pointer to the sysdev device. Once you do this, presumably your second patch will become unnecessary. Instead, maybe you can remove the clamping code in usb-storage. Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-01 14:55 ` Alan Stern 2020-09-01 23:24 ` Tom Yan @ 2020-09-02 0:09 ` Tom Yan 2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan 2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern 1 sibling, 2 replies; 24+ messages in thread From: Tom Yan @ 2020-09-02 0:09 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan There's no reason for uas to use a smaller value of max_sectors than usb-storage. Also copying the dma max mapping size clamping from usb-storage. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/uas.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 08f9296431e9..813c49914b9a 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) */ blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); - if (devinfo->flags & US_FL_MAX_SECTORS_64) - blk_queue_max_hw_sectors(sdev->request_queue, 64); - else if (devinfo->flags & US_FL_MAX_SECTORS_240) - blk_queue_max_hw_sectors(sdev->request_queue, 240); - return 0; } @@ -839,6 +834,20 @@ static int uas_slave_configure(struct scsi_device *sdev) { struct uas_dev_info *devinfo = sdev->hostdata; + struct us_data *us = host_to_us(sdev->host); + struct device *dev = us->pusb_dev->bus->sysdev; + + if (devinfo->flags & US_FL_MAX_SECTORS_64) + blk_queue_max_hw_sectors(sdev->request_queue, 64); + else if (devinfo->flags & US_FL_MAX_SECTORS_240) + blk_queue_max_hw_sectors(sdev->request_queue, 240); + else if (us->pusb_dev->speed >= USB_SPEED_SUPER) + blk_queue_max_hw_sectors(sdev->request_queue, 2048); + + blk_queue_max_hw_sectors(sdev->request_queue, + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); + if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping 2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan @ 2020-09-02 0:09 ` Tom Yan 2020-09-02 0:20 ` Tom Yan 2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern 1 sibling, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-02 0:09 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan When the scsi request queue is initialized/allocated, the scsi driver clamps hw_max_sectors against the dma max mapping size of sdev->host->dma_dev. The device is currently inappriorate to use for USB drives. Therefore, always (re)set hw_max_sectors in the usb drivers to invalidate the clamping. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------ drivers/usb/storage/uas.c | 2 ++ 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index e5a971b83e3f..1f60d777a7e8 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev) * better throughput on most devices. */ blk_queue_max_hw_sectors(sdev->request_queue, 2048); + } else { + /* + * 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); } /* @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = { /* lots of sg segments can be handled */ .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/uas.c b/drivers/usb/storage/uas.c index 813c49914b9a..592e1358822e 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -843,6 +843,8 @@ static int uas_slave_configure(struct scsi_device *sdev) blk_queue_max_hw_sectors(sdev->request_queue, 240); else if (us->pusb_dev->speed >= USB_SPEED_SUPER) blk_queue_max_hw_sectors(sdev->request_queue, 2048); + else + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS); blk_queue_max_hw_sectors(sdev->request_queue, min_t(size_t, queue_max_hw_sectors(sdev->request_queue), -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping 2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan @ 2020-09-02 0:20 ` Tom Yan 0 siblings, 0 replies; 24+ messages in thread From: Tom Yan @ 2020-09-02 0:20 UTC (permalink / raw) To: linux-usb, gregkh, Alan Stern, Arnd Bergmann; +Cc: cyrozap, Yoshihiro Shimoda I split it into two patches: 1. bump hw_max_sectors to 2048 for SS UAS drives and do the clamping for all UAS drives 2. make sure there's a "fallback" hw_max_sectors "change" so that the clamping on the SCSI side is invalidated. If anyone can help fix the dma_dev, the second one can be omitted (better make the clamping on the USB side to use shost->dev->dma_dev though). On Wed, 2 Sep 2020 at 08:09, Tom Yan <tom.ty89@gmail.com> wrote: > > When the scsi request queue is initialized/allocated, the scsi driver clamps > hw_max_sectors against the dma max mapping size of sdev->host->dma_dev. The > device is currently inappriorate to use for USB drives. > > Therefore, always (re)set hw_max_sectors in the usb drivers to invalidate the > clamping. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > --- > drivers/usb/storage/scsiglue.c | 37 ++++++++++++++++------------------ > drivers/usb/storage/uas.c | 2 ++ > 2 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index e5a971b83e3f..1f60d777a7e8 100644 > --- a/drivers/usb/storage/scsiglue.c > +++ b/drivers/usb/storage/scsiglue.c > @@ -120,6 +120,23 @@ static int slave_configure(struct scsi_device *sdev) > * better throughput on most devices. > */ > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > + } else { > + /* > + * 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); > } > > /* > @@ -626,26 +643,6 @@ static const struct scsi_host_template usb_stor_host_template = { > /* lots of sg segments can be handled */ > .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/uas.c b/drivers/usb/storage/uas.c > index 813c49914b9a..592e1358822e 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -843,6 +843,8 @@ static int uas_slave_configure(struct scsi_device *sdev) > blk_queue_max_hw_sectors(sdev->request_queue, 240); > else if (us->pusb_dev->speed >= USB_SPEED_SUPER) > blk_queue_max_hw_sectors(sdev->request_queue, 2048); > + else > + blk_queue_max_hw_sectors(sdev->request_queue, SCSI_DEFAULT_MAX_SECTORS); > > blk_queue_max_hw_sectors(sdev->request_queue, > min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan @ 2020-09-02 15:30 ` Alan Stern 2020-09-03 6:57 ` Tom Yan 1 sibling, 1 reply; 24+ messages in thread From: Alan Stern @ 2020-09-02 15:30 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, yoshihiro.shimoda.uh On Wed, Sep 02, 2020 at 08:09:36AM +0800, Tom Yan wrote: > There's no reason for uas to use a smaller value of max_sectors than > usb-storage. > > Also copying the dma max mapping size clamping from usb-storage. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > --- > drivers/usb/storage/uas.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index 08f9296431e9..813c49914b9a 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) > */ > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > - if (devinfo->flags & US_FL_MAX_SECTORS_64) > - blk_queue_max_hw_sectors(sdev->request_queue, 64); > - else if (devinfo->flags & US_FL_MAX_SECTORS_240) > - blk_queue_max_hw_sectors(sdev->request_queue, 240); > - > return 0; > } > > @@ -839,6 +834,20 @@ static int uas_slave_configure(struct scsi_device *sdev) > { > struct uas_dev_info *devinfo = sdev->hostdata; > > + struct us_data *us = host_to_us(sdev->host); > + struct device *dev = us->pusb_dev->bus->sysdev; Yeah, this won't work. The uas driver doesn't use struct us_data at all. Instead you should have: struct device *dev = devinfo->udev->bus->sysdev; except that now you probably don't need it. > + > + if (devinfo->flags & US_FL_MAX_SECTORS_64) > + blk_queue_max_hw_sectors(sdev->request_queue, 64); > + else if (devinfo->flags & US_FL_MAX_SECTORS_240) > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > + else if (us->pusb_dev->speed >= USB_SPEED_SUPER) Same thing here: devinfo->udev->speed. > + blk_queue_max_hw_sectors(sdev->request_queue, 2048); Also, you might want to check before doing this. If it would decrease the max_hw_sectors value, there's no point doing it. > + > + blk_queue_max_hw_sectors(sdev->request_queue, > + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); And presumably this will be unnecessary. Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern @ 2020-09-03 6:57 ` Tom Yan 2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 0 siblings, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-03 6:57 UTC (permalink / raw) To: Alan Stern; +Cc: linux-usb, gregkh, Arnd Bergmann, cyrozap, Yoshihiro Shimoda On Wed, 2 Sep 2020 at 23:30, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Sep 02, 2020 at 08:09:36AM +0800, Tom Yan wrote: > > There's no reason for uas to use a smaller value of max_sectors than > > usb-storage. > > > > Also copying the dma max mapping size clamping from usb-storage. > > > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > --- > > drivers/usb/storage/uas.c | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > > index 08f9296431e9..813c49914b9a 100644 > > --- a/drivers/usb/storage/uas.c > > +++ b/drivers/usb/storage/uas.c > > @@ -827,11 +827,6 @@ static int uas_slave_alloc(struct scsi_device *sdev) > > */ > > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > > > - if (devinfo->flags & US_FL_MAX_SECTORS_64) > > - blk_queue_max_hw_sectors(sdev->request_queue, 64); > > - else if (devinfo->flags & US_FL_MAX_SECTORS_240) > > - blk_queue_max_hw_sectors(sdev->request_queue, 240); > > - > > return 0; > > } > > > > @@ -839,6 +834,20 @@ static int uas_slave_configure(struct scsi_device *sdev) > > { > > struct uas_dev_info *devinfo = sdev->hostdata; > > > > + struct us_data *us = host_to_us(sdev->host); > > + struct device *dev = us->pusb_dev->bus->sysdev; > > Yeah, this won't work. The uas driver doesn't use struct us_data at > all. Instead you should have: > > struct device *dev = devinfo->udev->bus->sysdev; > > except that now you probably don't need it. > > > + > > + if (devinfo->flags & US_FL_MAX_SECTORS_64) > > + blk_queue_max_hw_sectors(sdev->request_queue, 64); > > + else if (devinfo->flags & US_FL_MAX_SECTORS_240) > > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > > + else if (us->pusb_dev->speed >= USB_SPEED_SUPER) > > Same thing here: devinfo->udev->speed. > > > + blk_queue_max_hw_sectors(sdev->request_queue, 2048); > > Also, you might want to check before doing this. If it would decrease > the max_hw_sectors value, there's no point doing it. It wouldn't. Current SCSI_DEFAULT_MAX_SECTORS (which is used for "normal" uas drives) is 1024. I'm not entirely sure if there's a point in doing more than that, although BLK_DEF_MAX_SECTORS is 2560 (which originates from an odd context: https://github.com/torvalds/linux/commit/d2be537c3ba3568acd79cd178327b842e60d035e). It just seems odd to me that we do less for SS UAS drives anyway. (Maybe it was never necessary to go up to 2048 for SS BOT drives, I don't know.) > > > + > > + blk_queue_max_hw_sectors(sdev->request_queue, > > + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > > + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); > > And presumably this will be unnecessary. It will still be. Whenever we change hw_max_sectors, it needs to be clamped again afterwards, because the original clamping would be invalidated. > > Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 6:57 ` Tom Yan @ 2020-09-03 7:56 ` Tom Yan 2020-09-03 7:56 ` [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH 0 siblings, 2 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 7:56 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c. When the scsi request queue is initialized/allocated, hw_max_sectors is clamped to the dma max mapping size. Therefore, the correct device that should be used for the clamping needs to be set. The same clamping is still needed in the USB drivers as hw_max_sectors could be changed there. The original clamping would be invalidated in such cases. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/scsiglue.c | 2 +- drivers/usb/storage/uas.c | 17 +++++++++++------ drivers/usb/storage/usb.c | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index e5a971b83e3f..560efd1479ba 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 = us->pusb_dev->bus->sysdev; + struct device *dev = sdev->host->dma_dev; /* * Many devices have trouble transferring more than 32KB at a time, diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 08f9296431e9..f4beeb8a8adb 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -827,17 +827,22 @@ static int uas_slave_alloc(struct scsi_device *sdev) */ blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); - if (devinfo->flags & US_FL_MAX_SECTORS_64) - blk_queue_max_hw_sectors(sdev->request_queue, 64); - else if (devinfo->flags & US_FL_MAX_SECTORS_240) - blk_queue_max_hw_sectors(sdev->request_queue, 240); - return 0; } static int uas_slave_configure(struct scsi_device *sdev) { struct uas_dev_info *devinfo = sdev->hostdata; + struct device *dev = sdev->host->dma_dev; + + if (devinfo->flags & US_FL_MAX_SECTORS_64) + blk_queue_max_hw_sectors(sdev->request_queue, 64); + else if (devinfo->flags & US_FL_MAX_SECTORS_240) + blk_queue_max_hw_sectors(sdev->request_queue, 240); + + blk_queue_max_hw_sectors(sdev->request_queue, + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; @@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id) shost->can_queue = devinfo->qdepth - 2; usb_set_intfdata(intf, shost); - result = scsi_add_host(shost, &intf->dev); + result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev); if (result) goto free_streams; diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 94a64729dc27..c2ef367cf257 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us) goto BadDevice; usb_autopm_get_interface_no_resume(us->pusb_intf); snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", - dev_name(&us->pusb_intf->dev)); - result = scsi_add_host(us_to_host(us), dev); + dev_name(dev)); + result = scsi_add_host_with_dma(us_to_host(us), dev, + us->pusb_dev->bus->sysdev); if (result) { dev_warn(dev, "Unable to add the scsi host\n"); -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan @ 2020-09-03 7:56 ` Tom Yan 2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH 1 sibling, 0 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 7:56 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan There's no reason for uas to use a smaller value of max_sectors than usb-storage. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/uas.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index f4beeb8a8adb..c1123da43407 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev) blk_queue_max_hw_sectors(sdev->request_queue, 64); else if (devinfo->flags & US_FL_MAX_SECTORS_240) blk_queue_max_hw_sectors(sdev->request_queue, 240); + else if (devinfo->udev->speed >= USB_SPEED_SUPER) + blk_queue_max_hw_sectors(sdev->request_queue, 2048); blk_queue_max_hw_sectors(sdev->request_queue, min_t(size_t, queue_max_hw_sectors(sdev->request_queue), -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 2020-09-03 7:56 ` [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan @ 2020-09-03 8:20 ` Greg KH 2020-09-03 8:28 ` Tom Yan 1 sibling, 1 reply; 24+ messages in thread From: Greg KH @ 2020-09-03 8:20 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, stern, arnd, cyrozap, yoshihiro.shimoda.uh On Thu, Sep 03, 2020 at 03:56:38PM +0800, Tom Yan wrote: > Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c. > > When the scsi request queue is initialized/allocated, hw_max_sectors is clamped > to the dma max mapping size. Therefore, the correct device that should be used > for the clamping needs to be set. > > The same clamping is still needed in the USB drivers as hw_max_sectors could be > changed there. The original clamping would be invalidated in such cases. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > --- > drivers/usb/storage/scsiglue.c | 2 +- > drivers/usb/storage/uas.c | 17 +++++++++++------ > drivers/usb/storage/usb.c | 5 +++-- > 3 files changed, 15 insertions(+), 9 deletions(-) What changed from the previous version? Always include that below the --- line as is documented to do so. Please fix up and resend. thanks, greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH @ 2020-09-03 8:28 ` Tom Yan 2020-09-03 8:34 ` Greg KH 0 siblings, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-03 8:28 UTC (permalink / raw) To: Greg KH; +Cc: linux-usb, Alan Stern, Arnd Bergmann, cyrozap, Yoshihiro Shimoda Hmm, actually the older versions were pretty much an entirely different approach/patch (not involving fixing the dma_dev at all). Should I just resend without the "v3"? On Thu, 3 Sep 2020 at 16:19, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Sep 03, 2020 at 03:56:38PM +0800, Tom Yan wrote: > > Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c. > > > > When the scsi request queue is initialized/allocated, hw_max_sectors is clamped > > to the dma max mapping size. Therefore, the correct device that should be used > > for the clamping needs to be set. > > > > The same clamping is still needed in the USB drivers as hw_max_sectors could be > > changed there. The original clamping would be invalidated in such cases. > > > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > --- > > drivers/usb/storage/scsiglue.c | 2 +- > > drivers/usb/storage/uas.c | 17 +++++++++++------ > > drivers/usb/storage/usb.c | 5 +++-- > > 3 files changed, 15 insertions(+), 9 deletions(-) > > What changed from the previous version? Always include that below the > --- line as is documented to do so. > > Please fix up and resend. > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 8:28 ` Tom Yan @ 2020-09-03 8:34 ` Greg KH 2020-09-03 8:46 ` [PATCH v4 " Tom Yan 2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 0 siblings, 2 replies; 24+ messages in thread From: Greg KH @ 2020-09-03 8:34 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, Alan Stern, Arnd Bergmann, cyrozap, Yoshihiro Shimoda On Thu, Sep 03, 2020 at 04:28:53PM +0800, Tom Yan wrote: > Hmm, actually the older versions were pretty much an entirely > different approach/patch (not involving fixing the dma_dev at all). > Should I just resend without the "v3"? As you already sent a v3 patch series, the next one would be v4, right? :) thanks, greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 8:34 ` Greg KH @ 2020-09-03 8:46 ` Tom Yan 2020-09-03 8:46 ` [PATCH v4 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 1 sibling, 1 reply; 24+ messages in thread From: Tom Yan @ 2020-09-03 8:46 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c. When the scsi request queue is initialized/allocated, hw_max_sectors is clamped to the dma max mapping size. Therefore, the correct device that should be used for the clamping needs to be set. The same clamping is still needed in the USB drivers as hw_max_sectors could be changed there. The original clamping would be invalidated in such cases. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS drives; split the "fallback" hw_max_sectors setting into another patch v3: use a different approach: fix the dma_dev instead v4: add the changelog of the patch series drivers/usb/storage/scsiglue.c | 2 +- drivers/usb/storage/uas.c | 17 +++++++++++------ drivers/usb/storage/usb.c | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index e5a971b83e3f..560efd1479ba 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 = us->pusb_dev->bus->sysdev; + struct device *dev = sdev->host->dma_dev; /* * Many devices have trouble transferring more than 32KB at a time, diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 08f9296431e9..f4beeb8a8adb 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -827,17 +827,22 @@ static int uas_slave_alloc(struct scsi_device *sdev) */ blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); - if (devinfo->flags & US_FL_MAX_SECTORS_64) - blk_queue_max_hw_sectors(sdev->request_queue, 64); - else if (devinfo->flags & US_FL_MAX_SECTORS_240) - blk_queue_max_hw_sectors(sdev->request_queue, 240); - return 0; } static int uas_slave_configure(struct scsi_device *sdev) { struct uas_dev_info *devinfo = sdev->hostdata; + struct device *dev = sdev->host->dma_dev; + + if (devinfo->flags & US_FL_MAX_SECTORS_64) + blk_queue_max_hw_sectors(sdev->request_queue, 64); + else if (devinfo->flags & US_FL_MAX_SECTORS_240) + blk_queue_max_hw_sectors(sdev->request_queue, 240); + + blk_queue_max_hw_sectors(sdev->request_queue, + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; @@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id) shost->can_queue = devinfo->qdepth - 2; usb_set_intfdata(intf, shost); - result = scsi_add_host(shost, &intf->dev); + result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev); if (result) goto free_streams; diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 94a64729dc27..c2ef367cf257 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us) goto BadDevice; usb_autopm_get_interface_no_resume(us->pusb_intf); snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", - dev_name(&us->pusb_intf->dev)); - result = scsi_add_host(us_to_host(us), dev); + dev_name(dev)); + result = scsi_add_host_with_dma(us_to_host(us), dev, + us->pusb_dev->bus->sysdev); if (result) { dev_warn(dev, "Unable to add the scsi host\n"); -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-03 8:46 ` [PATCH v4 " Tom Yan @ 2020-09-03 8:46 ` Tom Yan 0 siblings, 0 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 8:46 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan There's no reason for uas to use a smaller value of max_sectors than usb-storage. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/uas.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index f4beeb8a8adb..c1123da43407 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev) blk_queue_max_hw_sectors(sdev->request_queue, 64); else if (devinfo->flags & US_FL_MAX_SECTORS_240) blk_queue_max_hw_sectors(sdev->request_queue, 240); + else if (devinfo->udev->speed >= USB_SPEED_SUPER) + blk_queue_max_hw_sectors(sdev->request_queue, 2048); blk_queue_max_hw_sectors(sdev->request_queue, min_t(size_t, queue_max_hw_sectors(sdev->request_queue), -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 8:34 ` Greg KH 2020-09-03 8:46 ` [PATCH v4 " Tom Yan @ 2020-09-03 8:50 ` Tom Yan 2020-09-03 8:50 ` [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern 1 sibling, 2 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 8:50 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c. When the scsi request queue is initialized/allocated, hw_max_sectors is clamped to the dma max mapping size. Therefore, the correct device that should be used for the clamping needs to be set. The same clamping is still needed in the USB drivers as hw_max_sectors could be changed there. The original clamping would be invalidated in such cases. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS drives; split the "fallback" hw_max_sectors setting into another patch v3: use a different approach: fix the dma_dev instead v4: add the changelog of the patch series v5: fix changelog line length drivers/usb/storage/scsiglue.c | 2 +- drivers/usb/storage/uas.c | 17 +++++++++++------ drivers/usb/storage/usb.c | 5 +++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index e5a971b83e3f..560efd1479ba 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 = us->pusb_dev->bus->sysdev; + struct device *dev = sdev->host->dma_dev; /* * Many devices have trouble transferring more than 32KB at a time, diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 08f9296431e9..f4beeb8a8adb 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -827,17 +827,22 @@ static int uas_slave_alloc(struct scsi_device *sdev) */ blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); - if (devinfo->flags & US_FL_MAX_SECTORS_64) - blk_queue_max_hw_sectors(sdev->request_queue, 64); - else if (devinfo->flags & US_FL_MAX_SECTORS_240) - blk_queue_max_hw_sectors(sdev->request_queue, 240); - return 0; } static int uas_slave_configure(struct scsi_device *sdev) { struct uas_dev_info *devinfo = sdev->hostdata; + struct device *dev = sdev->host->dma_dev; + + if (devinfo->flags & US_FL_MAX_SECTORS_64) + blk_queue_max_hw_sectors(sdev->request_queue, 64); + else if (devinfo->flags & US_FL_MAX_SECTORS_240) + blk_queue_max_hw_sectors(sdev->request_queue, 240); + + blk_queue_max_hw_sectors(sdev->request_queue, + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; @@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id) shost->can_queue = devinfo->qdepth - 2; usb_set_intfdata(intf, shost); - result = scsi_add_host(shost, &intf->dev); + result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev); if (result) goto free_streams; diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 94a64729dc27..c2ef367cf257 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us) goto BadDevice; usb_autopm_get_interface_no_resume(us->pusb_intf); snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", - dev_name(&us->pusb_intf->dev)); - result = scsi_add_host(us_to_host(us), dev); + dev_name(dev)); + result = scsi_add_host_with_dma(us_to_host(us), dev, + us->pusb_dev->bus->sysdev); if (result) { dev_warn(dev, "Unable to add the scsi host\n"); -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan @ 2020-09-03 8:50 ` Tom Yan 2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern 1 sibling, 0 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 8:50 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan There's no reason for uas to use a smaller value of max_sectors than usb-storage. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/uas.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index f4beeb8a8adb..c1123da43407 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev) blk_queue_max_hw_sectors(sdev->request_queue, 64); else if (devinfo->flags & US_FL_MAX_SECTORS_240) blk_queue_max_hw_sectors(sdev->request_queue, 240); + else if (devinfo->udev->speed >= USB_SPEED_SUPER) + blk_queue_max_hw_sectors(sdev->request_queue, 2048); blk_queue_max_hw_sectors(sdev->request_queue, min_t(size_t, queue_max_hw_sectors(sdev->request_queue), -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev 2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 2020-09-03 8:50 ` [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan @ 2020-09-03 15:54 ` Alan Stern 2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan 1 sibling, 1 reply; 24+ messages in thread From: Alan Stern @ 2020-09-03 15:54 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, yoshihiro.shimoda.uh On Thu, Sep 03, 2020 at 04:50:34PM +0800, Tom Yan wrote: > Use scsi_add_host_with_dma() instead of scsi_add_host() in usb.c and uas.c. > > When the scsi request queue is initialized/allocated, hw_max_sectors is clamped > to the dma max mapping size. Therefore, the correct device that should be used > for the clamping needs to be set. > > The same clamping is still needed in the USB drivers as hw_max_sectors could be > changed there. The original clamping would be invalidated in such cases. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > --- > v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS > drives; split the "fallback" hw_max_sectors setting into another patch > v3: use a different approach: fix the dma_dev instead > v4: add the changelog of the patch series > v5: fix changelog line length > drivers/usb/storage/scsiglue.c | 2 +- > drivers/usb/storage/uas.c | 17 +++++++++++------ > drivers/usb/storage/usb.c | 5 +++-- > 3 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index e5a971b83e3f..560efd1479ba 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 = us->pusb_dev->bus->sysdev; > + struct device *dev = sdev->host->dma_dev; > > /* > * Many devices have trouble transferring more than 32KB at a time, > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index 08f9296431e9..f4beeb8a8adb 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -827,17 +827,22 @@ static int uas_slave_alloc(struct scsi_device *sdev) > */ > blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); > > - if (devinfo->flags & US_FL_MAX_SECTORS_64) > - blk_queue_max_hw_sectors(sdev->request_queue, 64); > - else if (devinfo->flags & US_FL_MAX_SECTORS_240) > - blk_queue_max_hw_sectors(sdev->request_queue, 240); > - > return 0; > } > > static int uas_slave_configure(struct scsi_device *sdev) > { > struct uas_dev_info *devinfo = sdev->hostdata; > + struct device *dev = sdev->host->dma_dev; > + > + if (devinfo->flags & US_FL_MAX_SECTORS_64) > + blk_queue_max_hw_sectors(sdev->request_queue, 64); > + else if (devinfo->flags & US_FL_MAX_SECTORS_240) > + blk_queue_max_hw_sectors(sdev->request_queue, 240); > + > + blk_queue_max_hw_sectors(sdev->request_queue, > + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), > + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); > > if (devinfo->flags & US_FL_NO_REPORT_OPCODES) > sdev->no_report_opcodes = 1; > @@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id) > shost->can_queue = devinfo->qdepth - 2; > > usb_set_intfdata(intf, shost); > - result = scsi_add_host(shost, &intf->dev); > + result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev); > if (result) > goto free_streams; > > diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c > index 94a64729dc27..c2ef367cf257 100644 > --- a/drivers/usb/storage/usb.c > +++ b/drivers/usb/storage/usb.c > @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us) > goto BadDevice; > usb_autopm_get_interface_no_resume(us->pusb_intf); > snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", > - dev_name(&us->pusb_intf->dev)); > - result = scsi_add_host(us_to_host(us), dev); > + dev_name(dev)); > + result = scsi_add_host_with_dma(us_to_host(us), dev, > + us->pusb_dev->bus->sysdev); > if (result) { > dev_warn(dev, > "Unable to add the scsi host\n"); > -- > 2.28.0 These changes look fine. However, usb-storage and uas are two separate drivers. Could you split the patch into two, one containing the changes to usb.c and scsiglue.c (those are part of usb-storage) and the other containing the changes to uas.c? Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev 2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern @ 2020-09-03 18:17 ` Tom Yan 2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 18:17 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan Use scsi_add_host_with_dma() instead of scsi_add_host(). When the scsi request queue is initialized/allocated, hw_max_sectors is clamped to the dma max mapping size. Therefore, the correct device that should be used for the clamping needs to be set. The same clamping is still needed in usb-storage as hw_max_sectors could be changed there. The original clamping would be invalidated in such cases. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS drives; split the "fallback" hw_max_sectors setting into another patch v3: use a different approach: fix the dma_dev instead v4: add the changelog of the patch series v5: fix changelog line length v6: split dma_dev fix for usb-storage and uas drivers/usb/storage/scsiglue.c | 2 +- drivers/usb/storage/usb.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index e5a971b83e3f..560efd1479ba 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 = us->pusb_dev->bus->sysdev; + struct device *dev = sdev->host->dma_dev; /* * Many devices have trouble transferring more than 32KB at a time, diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index 94a64729dc27..c2ef367cf257 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us) goto BadDevice; usb_autopm_get_interface_no_resume(us->pusb_intf); snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", - dev_name(&us->pusb_intf->dev)); - result = scsi_add_host(us_to_host(us), dev); + dev_name(dev)); + result = scsi_add_host_with_dma(us_to_host(us), dev, + us->pusb_dev->bus->sysdev); if (result) { dev_warn(dev, "Unable to add the scsi host\n"); -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 2/3] uas: fix sdev->host->dma_dev 2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan @ 2020-09-03 18:17 ` Tom Yan 2020-09-03 18:17 ` [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 18:31 ` [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev Alan Stern 2 siblings, 0 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 18:17 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan Use scsi_add_host_with_dma() instead of scsi_add_host(). When the scsi request queue is initialized/allocated, hw_max_sectors is clamped to the dma max mapping size. Therefore, the correct device that should be used for the clamping needs to be set. The same clamping is still needed in uas as hw_max_sectors could be changed there. The original clamping would be invalidated in such cases. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/uas.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 08f9296431e9..f4beeb8a8adb 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -827,17 +827,22 @@ static int uas_slave_alloc(struct scsi_device *sdev) */ blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1)); - if (devinfo->flags & US_FL_MAX_SECTORS_64) - blk_queue_max_hw_sectors(sdev->request_queue, 64); - else if (devinfo->flags & US_FL_MAX_SECTORS_240) - blk_queue_max_hw_sectors(sdev->request_queue, 240); - return 0; } static int uas_slave_configure(struct scsi_device *sdev) { struct uas_dev_info *devinfo = sdev->hostdata; + struct device *dev = sdev->host->dma_dev; + + if (devinfo->flags & US_FL_MAX_SECTORS_64) + blk_queue_max_hw_sectors(sdev->request_queue, 64); + else if (devinfo->flags & US_FL_MAX_SECTORS_240) + blk_queue_max_hw_sectors(sdev->request_queue, 240); + + blk_queue_max_hw_sectors(sdev->request_queue, + min_t(size_t, queue_max_hw_sectors(sdev->request_queue), + dma_max_mapping_size(dev) >> SECTOR_SHIFT)); if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; @@ -1023,7 +1028,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id) shost->can_queue = devinfo->qdepth - 2; usb_set_intfdata(intf, shost); - result = scsi_add_host(shost, &intf->dev); + result = scsi_add_host_with_dma(shost, &intf->dev, udev->bus->sysdev); if (result) goto free_streams; -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives 2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan 2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan @ 2020-09-03 18:17 ` Tom Yan 2020-09-03 18:31 ` [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev Alan Stern 2 siblings, 0 replies; 24+ messages in thread From: Tom Yan @ 2020-09-03 18:17 UTC (permalink / raw) To: linux-usb, gregkh, stern, arnd; +Cc: cyrozap, yoshihiro.shimoda.uh, Tom Yan There's no reason for uas to use a smaller value of max_sectors than usb-storage. Signed-off-by: Tom Yan <tom.ty89@gmail.com> --- drivers/usb/storage/uas.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index f4beeb8a8adb..c1123da43407 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -839,6 +839,8 @@ static int uas_slave_configure(struct scsi_device *sdev) blk_queue_max_hw_sectors(sdev->request_queue, 64); else if (devinfo->flags & US_FL_MAX_SECTORS_240) blk_queue_max_hw_sectors(sdev->request_queue, 240); + else if (devinfo->udev->speed >= USB_SPEED_SUPER) + blk_queue_max_hw_sectors(sdev->request_queue, 2048); blk_queue_max_hw_sectors(sdev->request_queue, min_t(size_t, queue_max_hw_sectors(sdev->request_queue), -- 2.28.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev 2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan 2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan 2020-09-03 18:17 ` [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan @ 2020-09-03 18:31 ` Alan Stern 2 siblings, 0 replies; 24+ messages in thread From: Alan Stern @ 2020-09-03 18:31 UTC (permalink / raw) To: Tom Yan; +Cc: linux-usb, gregkh, arnd, cyrozap, yoshihiro.shimoda.uh On Fri, Sep 04, 2020 at 02:17:23AM +0800, Tom Yan wrote: > Use scsi_add_host_with_dma() instead of scsi_add_host(). > > When the scsi request queue is initialized/allocated, hw_max_sectors is clamped > to the dma max mapping size. Therefore, the correct device that should be used > for the clamping needs to be set. > > The same clamping is still needed in usb-storage as hw_max_sectors could be > changed there. The original clamping would be invalidated in such cases. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > --- > v2: fix commit message line length; bump hw_max_sectors to 2048 for SS UAS > drives; split the "fallback" hw_max_sectors setting into another patch > v3: use a different approach: fix the dma_dev instead > v4: add the changelog of the patch series > v5: fix changelog line length > v6: split dma_dev fix for usb-storage and uas > drivers/usb/storage/scsiglue.c | 2 +- > drivers/usb/storage/usb.c | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > index e5a971b83e3f..560efd1479ba 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 = us->pusb_dev->bus->sysdev; > + struct device *dev = sdev->host->dma_dev; > > /* > * Many devices have trouble transferring more than 32KB at a time, > diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c > index 94a64729dc27..c2ef367cf257 100644 > --- a/drivers/usb/storage/usb.c > +++ b/drivers/usb/storage/usb.c > @@ -1049,8 +1049,9 @@ int usb_stor_probe2(struct us_data *us) > goto BadDevice; > usb_autopm_get_interface_no_resume(us->pusb_intf); > snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", > - dev_name(&us->pusb_intf->dev)); > - result = scsi_add_host(us_to_host(us), dev); > + dev_name(dev)); > + result = scsi_add_host_with_dma(us_to_host(us), dev, > + us->pusb_dev->bus->sysdev); > if (result) { > dev_warn(dev, > "Unable to add the scsi host\n"); > -- > 2.28.0 For all three patches: Reviewed-by: Alan Stern <stern@rowland.harvard.edu> ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-09-03 18:31 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-01 5:54 [PATCH] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan 2020-09-01 14:55 ` Alan Stern 2020-09-01 23:24 ` Tom Yan 2020-09-01 23:44 ` Tom Yan 2020-09-02 15:24 ` Alan Stern 2020-09-02 0:09 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-02 0:09 ` [PATCH v2 2/2] usb-storage: always set hw_max_sectors in slave_configure to avoid inappropriate clamping Tom Yan 2020-09-02 0:20 ` Tom Yan 2020-09-02 15:30 ` [PATCH v2 1/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Alan Stern 2020-09-03 6:57 ` Tom Yan 2020-09-03 7:56 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 2020-09-03 7:56 ` [PATCH v3 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 8:20 ` [PATCH v3 1/2] usb-storage: fix sdev->host->dma_dev Greg KH 2020-09-03 8:28 ` Tom Yan 2020-09-03 8:34 ` Greg KH 2020-09-03 8:46 ` [PATCH v4 " Tom Yan 2020-09-03 8:46 ` [PATCH v4 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 8:50 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Tom Yan 2020-09-03 8:50 ` [PATCH v5 2/2] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 15:54 ` [PATCH v5 1/2] usb-storage: fix sdev->host->dma_dev Alan Stern 2020-09-03 18:17 ` [PATCH v6 1/3] " Tom Yan 2020-09-03 18:17 ` [PATCH v6 2/3] uas: " Tom Yan 2020-09-03 18:17 ` [PATCH v6 3/3] uas: bump hw_max_sectors to 2048 blocks for SS or faster drives Tom Yan 2020-09-03 18:31 ` [PATCH v6 1/3] usb-storage: fix sdev->host->dma_dev Alan Stern
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).