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@xxxxxx>
Just a comment below. Apart from that:
Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
---
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
