diff mbox series

[09/23] scsi: use the atomic queue limits API in scsi_add_lun

Message ID 20240324235448.2039074-10-hch@lst.de (mailing list archive)
State Superseded
Headers show
Series [01/23] block: don't reject too large max_user_setors in blk_validate_limits | expand

Commit Message

Christoph Hellwig March 24, 2024, 11:54 p.m. UTC
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(-)

Comments

Damien Le Moal March 25, 2024, 7:34 a.m. UTC | #1
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>
Bart Van Assche March 25, 2024, 10:13 p.m. UTC | #2
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>
John Garry March 27, 2024, 3:39 p.m. UTC | #3
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
Christoph Hellwig March 28, 2024, 5:02 a.m. UTC | #4
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 mbox series

Patch

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