| Message ID | 20240324235448.2039074-10-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/23] block: don't reject too large max_user_setors in blk_validate_limits | expand |
On 3/25/24 08:54, Christoph Hellwig wrote: > Switch scsi_add_lun to use the atomic queue limits API to update the > max_hw_sectors for devices with quirks. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks OK to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
On 3/24/24 16:54, Christoph Hellwig wrote: > Switch scsi_add_lun to use the atomic queue limits API to update the > max_hw_sectors for devices with quirks. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 24/03/2024 23:54, Christoph Hellwig wrote: > Switch scsi_add_lun to use the atomic queue limits API to update the > max_hw_sectors for devices with quirks. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Just a comment below. Apart from that: Reviewed-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/scsi_scan.c | 49 ++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 25 deletions(-) > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > index 205ab3b3ea89be..699356d7d17545 100644 > --- a/drivers/scsi/scsi_scan.c > +++ b/drivers/scsi/scsi_scan.c > @@ -874,6 +874,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, > static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, > blist_flags_t *bflags, int async) > { > + struct queue_limits lim; > int ret; > > /* > @@ -1004,19 +1005,6 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, > if (*bflags & BLIST_SELECT_NO_ATN) > sdev->select_no_atn = 1; > > - /* > - * Maximum 512 sector transfer length > - * broken RA4x00 Compaq Disk Array > - */ > - if (*bflags & BLIST_MAX_512) > - blk_queue_max_hw_sectors(sdev->request_queue, 512); > - /* > - * Max 1024 sector transfer length for targets that report incorrect > - * max/optimal lengths and relied on the old block layer safe default > - */ > - else if (*bflags & BLIST_MAX_1024) > - blk_queue_max_hw_sectors(sdev->request_queue, 1024); > - > /* > * Some devices may not want to have a start command automatically > * issued when a device is added. > @@ -1077,19 +1065,22 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, > > transport_configure_device(&sdev->sdev_gendev); > > + /* > + * No need to freeze the queue as it isn't reachable to anyone else yet. > + */ > + lim = queue_limits_start_update(sdev->request_queue); > + if (*bflags & BLIST_MAX_512) > + lim.max_hw_sectors = 512; > + else if (*bflags & BLIST_MAX_1024) > + lim.max_hw_sectors = 1024; > + ret = queue_limits_commit_update(sdev->request_queue, &lim); > + if (ret) > + goto fail; > + > if (sdev->host->hostt->slave_configure) { > ret = sdev->host->hostt->slave_configure(sdev); > - if (ret) { > - /* > - * if LLDD reports slave not present, don't clutter > - * console with alloc failure messages > - */ > - if (ret != -ENXIO) { > - sdev_printk(KERN_ERR, sdev, > - "failed to configure device\n"); Is there some reason to relocate this and have it included for other error paths, i.e. queue_limits_commit_update() call? It doesn't really tell us much to know the cause of the failure. At least previously it was in one location, so we knew the point of failure. > - } > - return SCSI_SCAN_NO_RESPONSE; > - } > + if (ret) > + goto fail; > > /* > * The queue_depth is often changed in ->slave_configure. > @@ -1115,8 +1106,16 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, > */ > if (!async && scsi_sysfs_add_sdev(sdev) != 0) > return SCSI_SCAN_NO_RESPONSE; > - > return SCSI_SCAN_LUN_PRESENT; > + > +fail: > + /* > + * If the LLDD reports LU not present, don't clutter the console with > + * alloc failure messages. > + */ > + if (ret != -ENXIO) > + sdev_printk(KERN_ERR, sdev, "failed to configure device\n"); > + return SCSI_SCAN_NO_RESPONSE; > } > > #ifdef CONFIG_SCSI_LOGGING
On Wed, Mar 27, 2024 at 03:39:25PM +0000, John Garry wrote: > Is there some reason to relocate this and have it included for other error > paths, i.e. queue_limits_commit_update() call? It doesn't really tell us > much to know the cause of the failure. At least previously it was in one > location, so we knew the point of failure. I assumed an error message might be useful, but maybe it should indeed be a different one.
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 205ab3b3ea89be..699356d7d17545 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -874,6 +874,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, blist_flags_t *bflags, int async) { + struct queue_limits lim; int ret; /* @@ -1004,19 +1005,6 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, if (*bflags & BLIST_SELECT_NO_ATN) sdev->select_no_atn = 1; - /* - * Maximum 512 sector transfer length - * broken RA4x00 Compaq Disk Array - */ - if (*bflags & BLIST_MAX_512) - blk_queue_max_hw_sectors(sdev->request_queue, 512); - /* - * Max 1024 sector transfer length for targets that report incorrect - * max/optimal lengths and relied on the old block layer safe default - */ - else if (*bflags & BLIST_MAX_1024) - blk_queue_max_hw_sectors(sdev->request_queue, 1024); - /* * Some devices may not want to have a start command automatically * issued when a device is added. @@ -1077,19 +1065,22 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, transport_configure_device(&sdev->sdev_gendev); + /* + * No need to freeze the queue as it isn't reachable to anyone else yet. + */ + lim = queue_limits_start_update(sdev->request_queue); + if (*bflags & BLIST_MAX_512) + lim.max_hw_sectors = 512; + else if (*bflags & BLIST_MAX_1024) + lim.max_hw_sectors = 1024; + ret = queue_limits_commit_update(sdev->request_queue, &lim); + if (ret) + goto fail; + if (sdev->host->hostt->slave_configure) { ret = sdev->host->hostt->slave_configure(sdev); - if (ret) { - /* - * if LLDD reports slave not present, don't clutter - * console with alloc failure messages - */ - if (ret != -ENXIO) { - sdev_printk(KERN_ERR, sdev, - "failed to configure device\n"); - } - return SCSI_SCAN_NO_RESPONSE; - } + if (ret) + goto fail; /* * The queue_depth is often changed in ->slave_configure. @@ -1115,8 +1106,16 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, */ if (!async && scsi_sysfs_add_sdev(sdev) != 0) return SCSI_SCAN_NO_RESPONSE; - return SCSI_SCAN_LUN_PRESENT; + +fail: + /* + * If the LLDD reports LU not present, don't clutter the console with + * alloc failure messages. + */ + if (ret != -ENXIO) + sdev_printk(KERN_ERR, sdev, "failed to configure device\n"); + return SCSI_SCAN_NO_RESPONSE; } #ifdef CONFIG_SCSI_LOGGING
Switch scsi_add_lun to use the atomic queue limits API to update the max_hw_sectors for devices with quirks. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/scsi/scsi_scan.c | 49 ++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 25 deletions(-)