[PATCH RFC] scsi: sd: Fix build warning in sd_revalidate_disk()

[PATCH RFC] scsi: sd: Fix build warning in sd_revalidate_disk()

[Abinash Singh]

A build warning was triggered due to excessive stack usage in
sd_revalidate_disk():

drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]

This is caused by a large local struct queue_limits (~400B) allocated
on the stack. Replacing it with a heap allocation using kmalloc()
significantly reduces frame usage. Kernel stack is limited (~8 KB),
and allocating large structs on the stack is discouraged.
As the function already performs heap allocations (e.g. for buffer),
this change fits well.

Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>

---
This was further confirmed by compiling sd.c with -fstack-usage flag

Before: drivers/scsi/sd.c:3694:12:sd_revalidate_disk.isra 1248 dynamic,bounded
After:  drivers/scsi/sd.c:3695:12:sd_revalidate_disk.isra  840 dynamic,bounded

Already we had a heap allocation in this function so I think  we can do this
without any issues.
I have followed the same pattern on allocation failure as done for
`buffer`.

This function appears stable; if it's not under active development,
we may consider cleaning up unused goto statements in a follow-up patch.

Thanks For your valuable Time

Best regards
Abinash
---
 drivers/scsi/sd.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4a68b2ab2804..a03844400e51 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -34,6 +34,7 @@
  */
 
 #include <linux/bio-integrity.h>
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/kernel.h>
@@ -3696,11 +3696,16 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	struct scsi_device *sdp = sdkp->device;
 	sector_t old_capacity = sdkp->capacity;
-	struct queue_limits lim;
 	unsigned char *buffer;
 	unsigned int dev_max;
 	int err;
 
+	struct queue_limits *lim __free(kfree) = kmalloc(sizeof(*lim), GFP_KERNEL);
+	if (!lim) {
+		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory allocation failure.\n");
+		goto out;
+	}
+
 	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
 				      "sd_revalidate_disk\n"));
 
@@ -3720,14 +3726,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
 
 	sd_spinup_disk(sdkp);
 
-	lim = queue_limits_start_update(sdkp->disk->queue);
+	*lim = queue_limits_start_update(sdkp->disk->queue);
 
 	/*
 	 * Without media there is no reason to ask; moreover, some devices
 	 * react badly if we do.
 	 */
 	if (sdkp->media_present) {
-		sd_read_capacity(sdkp, &lim, buffer);
+		sd_read_capacity(sdkp, lim, buffer);
 		/*
 		 * Some USB/UAS devices return generic values for mode pages
 		 * until the media has been accessed. Trigger a READ operation
@@ -3741,17 +3747,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		 * cause this to be updated correctly and any device which
 		 * doesn't support it should be treated as rotational.
 		 */
-		lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
+		lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
 
 		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
-			sd_read_block_limits(sdkp, &lim);
+			sd_read_block_limits(sdkp, lim);
 			sd_read_block_limits_ext(sdkp);
-			sd_read_block_characteristics(sdkp, &lim);
-			sd_zbc_read_zones(sdkp, &lim, buffer);
+			sd_read_block_characteristics(sdkp, lim);
+			sd_zbc_read_zones(sdkp, lim, buffer);
 		}
 
-		sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
+		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
 
 		sd_print_capacity(sdkp, old_capacity);
 
@@ -3761,45 +3767,45 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_app_tag_own(sdkp, buffer);
 		sd_read_write_same(sdkp, buffer);
 		sd_read_security(sdkp, buffer);
-		sd_config_protection(sdkp, &lim);
+		sd_config_protection(sdkp, lim);
 	}
 
 	/*
 	 * We now have all cache related info, determine how we deal
 	 * with flush requests.
 	 */
-	sd_set_flush_flag(sdkp, &lim);
+	sd_set_flush_flag(sdkp, lim);
 
 	/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
 	dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
 
 	/* Some devices report a maximum block count for READ/WRITE requests. */
 	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
-	lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
+	lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
 
 	if (sd_validate_min_xfer_size(sdkp))
-		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
+		lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
 	else
-		lim.io_min = 0;
+		lim->io_min = 0;
 
 	/*
 	 * Limit default to SCSI host optimal sector limit if set. There may be
 	 * an impact on performance for when the size of a request exceeds this
 	 * host limit.
 	 */
-	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
+	lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
 	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
-		lim.io_opt = min_not_zero(lim.io_opt,
+		lim->io_opt = min_not_zero(lim->io_opt,
 				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
 	}
 
 	sdkp->first_scan = 0;
 
 	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
-	sd_config_write_same(sdkp, &lim);
+	sd_config_write_same(sdkp, lim);
 	kfree(buffer);
 
-	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
+	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
 	if (err)
 		return err;
 
-- 
2.50.1

Re: [PATCH RFC] scsi: sd: Fix build warning in sd_revalidate_disk()

[Damien Le Moal]

On 8/8/25 3:30 AM, Abinash Singh wrote:
> A build warning was triggered due to excessive stack usage in
> sd_revalidate_disk():
> 
> drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
> drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> This is caused by a large local struct queue_limits (~400B) allocated
> on the stack. Replacing it with a heap allocation using kmalloc()
> significantly reduces frame usage. Kernel stack is limited (~8 KB),
> and allocating large structs on the stack is discouraged.
> As the function already performs heap allocations (e.g. for buffer),
> this change fits well.
> 
> Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4a68b2ab2804..a03844400e51 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -34,6 +34,7 @@
>   */
>  
>  #include <linux/bio-integrity.h>
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/fs.h>
>  #include <linux/kernel.h>
> @@ -3696,11 +3696,16 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	struct scsi_disk *sdkp = scsi_disk(disk);
>  	struct scsi_device *sdp = sdkp->device;
>  	sector_t old_capacity = sdkp->capacity;
> -	struct queue_limits lim;
>  	unsigned char *buffer;
>  	unsigned int dev_max;
>  	int err;
>  
> +	struct queue_limits *lim __free(kfree) = kmalloc(sizeof(*lim), GFP_KERNEL);

Please keep the declarations together. Not sure how that will work with that
unreadable __free(kfree) annotation though (the "unreadable" part of this
comment is only my opinion... I really dislike stuff that hides code...).

> +	if (!lim) {
> +		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory allocation failure.\n");

Long line. Please split after sdkp. Also, the same message is used for the
buffer allocation. So maybe differentiate with it ? E.g. something like "Disk
limit allocation failure" ?

> +		goto out;
> +	}
> +
>  	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
>  				      "sd_revalidate_disk\n"));
>  
> @@ -3720,14 +3726,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  
>  	sd_spinup_disk(sdkp);
>  
> -	lim = queue_limits_start_update(sdkp->disk->queue);
> +	*lim = queue_limits_start_update(sdkp->disk->queue);
>  
>  	/*
>  	 * Without media there is no reason to ask; moreover, some devices
>  	 * react badly if we do.
>  	 */
>  	if (sdkp->media_present) {
> -		sd_read_capacity(sdkp, &lim, buffer);
> +		sd_read_capacity(sdkp, lim, buffer);
>  		/*
>  		 * Some USB/UAS devices return generic values for mode pages
>  		 * until the media has been accessed. Trigger a READ operation
> @@ -3741,17 +3747,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		 * cause this to be updated correctly and any device which
>  		 * doesn't support it should be treated as rotational.
>  		 */
> -		lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
> +		lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
>  
>  		if (scsi_device_supports_vpd(sdp)) {
>  			sd_read_block_provisioning(sdkp);
> -			sd_read_block_limits(sdkp, &lim);
> +			sd_read_block_limits(sdkp, lim);
>  			sd_read_block_limits_ext(sdkp);
> -			sd_read_block_characteristics(sdkp, &lim);
> -			sd_zbc_read_zones(sdkp, &lim, buffer);
> +			sd_read_block_characteristics(sdkp, lim);
> +			sd_zbc_read_zones(sdkp, lim, buffer);
>  		}
>  
> -		sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
> +		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
>  
>  		sd_print_capacity(sdkp, old_capacity);
>  
> @@ -3761,45 +3767,45 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		sd_read_app_tag_own(sdkp, buffer);
>  		sd_read_write_same(sdkp, buffer);
>  		sd_read_security(sdkp, buffer);
> -		sd_config_protection(sdkp, &lim);
> +		sd_config_protection(sdkp, lim);
>  	}
>  
>  	/*
>  	 * We now have all cache related info, determine how we deal
>  	 * with flush requests.
>  	 */
> -	sd_set_flush_flag(sdkp, &lim);
> +	sd_set_flush_flag(sdkp, lim);
>  
>  	/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
>  	dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
>  
>  	/* Some devices report a maximum block count for READ/WRITE requests. */
>  	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
> -	lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
> +	lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
>  
>  	if (sd_validate_min_xfer_size(sdkp))
> -		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
> +		lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
>  	else
> -		lim.io_min = 0;
> +		lim->io_min = 0;
>  
>  	/*
>  	 * Limit default to SCSI host optimal sector limit if set. There may be
>  	 * an impact on performance for when the size of a request exceeds this
>  	 * host limit.
>  	 */
> -	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
> +	lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
>  	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
> -		lim.io_opt = min_not_zero(lim.io_opt,
> +		lim->io_opt = min_not_zero(lim->io_opt,
>  				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
>  	}
>  
>  	sdkp->first_scan = 0;
>  
>  	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
> -	sd_config_write_same(sdkp, &lim);
> +	sd_config_write_same(sdkp, lim);
>  	kfree(buffer);
>  
> -	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
> +	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
>  	if (err)
>  		return err;
>  


-- 
Damien Le Moal
Western Digital Research

[PATCH v2] scsi: sd: Fix build warning in sd_revalidate_disk()

[Abinash Singh]

A build warning was triggered due to excessive stack usage in
sd_revalidate_disk():

drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]

This is caused by a large local struct queue_limits (~400B) allocated
on the stack. Replacing it with a heap allocation using kmalloc()
significantly reduces frame usage. Kernel stack is limited (~8 KB),
and allocating large structs on the stack is discouraged.
As the function already performs heap allocations (e.g. for buffer),
this change fits well.

Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
---

Hi,

Thank you very much for your comments.
I have addressed all your suggestions from v1.

As you mentioned concerns regarding the readability of
the __free(kfree) attribute, I have used the classic
approach in v2. Additionally, I will also send v3
where the __free() attribute is used instead.

We can proceed with patch version you
find more suitable, and do let me know if you have
any further feedback.

changelog v1->v2:
	moved declarations together
	avoided "unreadable" cleanup attribute
	splited long line
	changed the log message to diiferentiate with buffer allocation

Thanks,

---
 drivers/scsi/sd.c | 49 +++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4a68b2ab2804..f5ab2a422df6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3696,7 +3696,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	struct scsi_device *sdp = sdkp->device;
 	sector_t old_capacity = sdkp->capacity;
-	struct queue_limits lim;
+	struct queue_limits *lim;
 	unsigned char *buffer;
 	unsigned int dev_max;
 	int err;
@@ -3711,23 +3711,30 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (!scsi_device_online(sdp))
 		goto out;
 
+	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
+	if (!lim) {
+		sd_printk(KERN_WARNING, sdkp,
+			"sd_revalidate_disk: Disk limit allocation failure.\n");
+		goto out;
+	}
+
 	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
 	if (!buffer) {
 		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
 			  "allocation failure.\n");
-		goto out;
+		goto free_lim;
 	}
 
 	sd_spinup_disk(sdkp);
 
-	lim = queue_limits_start_update(sdkp->disk->queue);
+	*lim = queue_limits_start_update(sdkp->disk->queue);
 
 	/*
 	 * Without media there is no reason to ask; moreover, some devices
 	 * react badly if we do.
 	 */
 	if (sdkp->media_present) {
-		sd_read_capacity(sdkp, &lim, buffer);
+		sd_read_capacity(sdkp, lim, buffer);
 		/*
 		 * Some USB/UAS devices return generic values for mode pages
 		 * until the media has been accessed. Trigger a READ operation
@@ -3741,17 +3748,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		 * cause this to be updated correctly and any device which
 		 * doesn't support it should be treated as rotational.
 		 */
-		lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
+		lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
 
 		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
-			sd_read_block_limits(sdkp, &lim);
+			sd_read_block_limits(sdkp, lim);
 			sd_read_block_limits_ext(sdkp);
-			sd_read_block_characteristics(sdkp, &lim);
-			sd_zbc_read_zones(sdkp, &lim, buffer);
+			sd_read_block_characteristics(sdkp, lim);
+			sd_zbc_read_zones(sdkp, lim, buffer);
 		}
 
-		sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
+		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
 
 		sd_print_capacity(sdkp, old_capacity);
 
@@ -3761,47 +3768,49 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_app_tag_own(sdkp, buffer);
 		sd_read_write_same(sdkp, buffer);
 		sd_read_security(sdkp, buffer);
-		sd_config_protection(sdkp, &lim);
+		sd_config_protection(sdkp, lim);
 	}
 
 	/*
 	 * We now have all cache related info, determine how we deal
 	 * with flush requests.
 	 */
-	sd_set_flush_flag(sdkp, &lim);
+	sd_set_flush_flag(sdkp, lim);
 
 	/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
 	dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
 
 	/* Some devices report a maximum block count for READ/WRITE requests. */
 	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
-	lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
+	lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
 
 	if (sd_validate_min_xfer_size(sdkp))
-		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
+		lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
 	else
-		lim.io_min = 0;
+		lim->io_min = 0;
 
 	/*
 	 * Limit default to SCSI host optimal sector limit if set. There may be
 	 * an impact on performance for when the size of a request exceeds this
 	 * host limit.
 	 */
-	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
+	lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
 	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
-		lim.io_opt = min_not_zero(lim.io_opt,
+		lim->io_opt = min_not_zero(lim->io_opt,
 				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
 	}
 
 	sdkp->first_scan = 0;
 
 	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
-	sd_config_write_same(sdkp, &lim);
+	sd_config_write_same(sdkp, lim);
 	kfree(buffer);
 
-	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
-	if (err)
+	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
+	if (err) {
+		kfree(lim);
 		return err;
+	}
 
 	/*
 	 * Query concurrent positioning ranges after
@@ -3819,6 +3828,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (sd_zbc_revalidate_zones(sdkp))
 		set_capacity_and_notify(disk, 0);
 
+ free_lim:
+	kfree(lim);
  out:
 	return 0;
 }
-- 
2.50.1

[PATCH v3] scsi: sd: Fix build warning in sd_revalidate_disk()

[Abinash Singh]

A build warning was triggered due to excessive stack usage in
sd_revalidate_disk():

drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]

This is caused by a large local struct queue_limits (~400B) allocated
on the stack. Replacing it with a heap allocation using kmalloc()
significantly reduces frame usage. Kernel stack is limited (~8 KB),
and allocating large structs on the stack is discouraged.
As the function already performs heap allocations (e.g. for buffer),
this change fits well.

Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
---

Hi ,

As I mentioned in v2,.. this is the corresponding
v3 of that. Only difference is in using __free() attribute
Please infor  me which one is better.

changelog v2->v3:
	used __free(kfree) attribute.
	no extra goto statements (i.e `free_lim`)

lim was initialized to NULL because there is a return path
before its allocation. Early exit will pass uninitlized pointer to
`kfree`.
We could move the allocation earlier, before this check:

			if (!scsi_device_online(sdp))
    			goto out;
However, doing so would result in unnecessary allocation
if the device is not online, leading to wasted resources.

Thanks,
---
 drivers/scsi/sd.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4a68b2ab2804..50abbab7e27a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -34,6 +34,7 @@
  */
 
 #include <linux/bio-integrity.h>
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/kernel.h>
@@ -3696,7 +3697,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	struct scsi_device *sdp = sdkp->device;
 	sector_t old_capacity = sdkp->capacity;
-	struct queue_limits lim;
+	struct queue_limits *lim __free(kfree) = NULL;
 	unsigned char *buffer;
 	unsigned int dev_max;
 	int err;
@@ -3711,6 +3712,13 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (!scsi_device_online(sdp))
 		goto out;
 
+	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
+	if (!lim) {
+		sd_printk(KERN_WARNING, sdkp,
+			"sd_revalidate_disk: Disk limit allocation failure.\n");
+		goto out;
+	}
+
 	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
 	if (!buffer) {
 		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
@@ -3720,14 +3728,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
 
 	sd_spinup_disk(sdkp);
 
-	lim = queue_limits_start_update(sdkp->disk->queue);
+	*lim = queue_limits_start_update(sdkp->disk->queue);
 
 	/*
 	 * Without media there is no reason to ask; moreover, some devices
 	 * react badly if we do.
 	 */
 	if (sdkp->media_present) {
-		sd_read_capacity(sdkp, &lim, buffer);
+		sd_read_capacity(sdkp, lim, buffer);
 		/*
 		 * Some USB/UAS devices return generic values for mode pages
 		 * until the media has been accessed. Trigger a READ operation
@@ -3741,17 +3749,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		 * cause this to be updated correctly and any device which
 		 * doesn't support it should be treated as rotational.
 		 */
-		lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
+		lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
 
 		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
-			sd_read_block_limits(sdkp, &lim);
+			sd_read_block_limits(sdkp, lim);
 			sd_read_block_limits_ext(sdkp);
-			sd_read_block_characteristics(sdkp, &lim);
-			sd_zbc_read_zones(sdkp, &lim, buffer);
+			sd_read_block_characteristics(sdkp, lim);
+			sd_zbc_read_zones(sdkp, lim, buffer);
 		}
 
-		sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
+		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
 
 		sd_print_capacity(sdkp, old_capacity);
 
@@ -3761,45 +3769,45 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_app_tag_own(sdkp, buffer);
 		sd_read_write_same(sdkp, buffer);
 		sd_read_security(sdkp, buffer);
-		sd_config_protection(sdkp, &lim);
+		sd_config_protection(sdkp, lim);
 	}
 
 	/*
 	 * We now have all cache related info, determine how we deal
 	 * with flush requests.
 	 */
-	sd_set_flush_flag(sdkp, &lim);
+	sd_set_flush_flag(sdkp, lim);
 
 	/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
 	dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
 
 	/* Some devices report a maximum block count for READ/WRITE requests. */
 	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
-	lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
+	lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
 
 	if (sd_validate_min_xfer_size(sdkp))
-		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
+		lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
 	else
-		lim.io_min = 0;
+		lim->io_min = 0;
 
 	/*
 	 * Limit default to SCSI host optimal sector limit if set. There may be
 	 * an impact on performance for when the size of a request exceeds this
 	 * host limit.
 	 */
-	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
+	lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
 	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
-		lim.io_opt = min_not_zero(lim.io_opt,
+		lim->io_opt = min_not_zero(lim->io_opt,
 				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
 	}
 
 	sdkp->first_scan = 0;
 
 	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
-	sd_config_write_same(sdkp, &lim);
+	sd_config_write_same(sdkp, lim);
 	kfree(buffer);
 
-	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
+	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
 	if (err)
 		return err;
 
-- 
2.50.1

Re: [PATCH v3] scsi: sd: Fix build warning in sd_revalidate_disk()

[Damien Le Moal]

On 8/8/25 20:30, Abinash Singh wrote:
> A build warning was triggered due to excessive stack usage in
> sd_revalidate_disk():
> 
> drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
> drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> This is caused by a large local struct queue_limits (~400B) allocated
> on the stack. Replacing it with a heap allocation using kmalloc()
> significantly reduces frame usage. Kernel stack is limited (~8 KB),
> and allocating large structs on the stack is discouraged.
> As the function already performs heap allocations (e.g. for buffer),
> this change fits well.
> 
> Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
> ---
> 
> Hi ,
> 
> As I mentioned in v2,.. this is the corresponding
> v3 of that. Only difference is in using __free() attribute
> Please infor  me which one is better.

Matter of taste. Personally, I dislike annotations that hide code.
I'll let Martin decide on this.

> 
> changelog v2->v3:
> 	used __free(kfree) attribute.
> 	no extra goto statements (i.e `free_lim`)
> 
> lim was initialized to NULL because there is a return path
> before its allocation. Early exit will pass uninitlized pointer to
> `kfree`.
> We could move the allocation earlier, before this check:
> 
> 			if (!scsi_device_online(sdp))
>     			goto out;
> However, doing so would result in unnecessary allocation
> if the device is not online, leading to wasted resources.
> 
> Thanks,
> ---
>  drivers/scsi/sd.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4a68b2ab2804..50abbab7e27a 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -34,6 +34,7 @@
>   */
>  
>  #include <linux/bio-integrity.h>
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/fs.h>
>  #include <linux/kernel.h>
> @@ -3696,7 +3697,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	struct scsi_disk *sdkp = scsi_disk(disk);
>  	struct scsi_device *sdp = sdkp->device;
>  	sector_t old_capacity = sdkp->capacity;
> -	struct queue_limits lim;
> +	struct queue_limits *lim __free(kfree) = NULL;
>  	unsigned char *buffer;
>  	unsigned int dev_max;
>  	int err;
> @@ -3711,6 +3712,13 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	if (!scsi_device_online(sdp))
>  		goto out;
>  
> +	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
> +	if (!lim) {
> +		sd_printk(KERN_WARNING, sdkp,
> +			"sd_revalidate_disk: Disk limit allocation failure.\n");
> +		goto out;
> +	}
> +
>  	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
>  	if (!buffer) {
>  		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
> @@ -3720,14 +3728,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  
>  	sd_spinup_disk(sdkp);
>  
> -	lim = queue_limits_start_update(sdkp->disk->queue);
> +	*lim = queue_limits_start_update(sdkp->disk->queue);
>  
>  	/*
>  	 * Without media there is no reason to ask; moreover, some devices
>  	 * react badly if we do.
>  	 */
>  	if (sdkp->media_present) {
> -		sd_read_capacity(sdkp, &lim, buffer);
> +		sd_read_capacity(sdkp, lim, buffer);
>  		/*
>  		 * Some USB/UAS devices return generic values for mode pages
>  		 * until the media has been accessed. Trigger a READ operation
> @@ -3741,17 +3749,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		 * cause this to be updated correctly and any device which
>  		 * doesn't support it should be treated as rotational.
>  		 */
> -		lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
> +		lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
>  
>  		if (scsi_device_supports_vpd(sdp)) {
>  			sd_read_block_provisioning(sdkp);
> -			sd_read_block_limits(sdkp, &lim);
> +			sd_read_block_limits(sdkp, lim);
>  			sd_read_block_limits_ext(sdkp);
> -			sd_read_block_characteristics(sdkp, &lim);
> -			sd_zbc_read_zones(sdkp, &lim, buffer);
> +			sd_read_block_characteristics(sdkp, lim);
> +			sd_zbc_read_zones(sdkp, lim, buffer);
>  		}
>  
> -		sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
> +		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
>  
>  		sd_print_capacity(sdkp, old_capacity);
>  
> @@ -3761,45 +3769,45 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		sd_read_app_tag_own(sdkp, buffer);
>  		sd_read_write_same(sdkp, buffer);
>  		sd_read_security(sdkp, buffer);
> -		sd_config_protection(sdkp, &lim);
> +		sd_config_protection(sdkp, lim);
>  	}
>  
>  	/*
>  	 * We now have all cache related info, determine how we deal
>  	 * with flush requests.
>  	 */
> -	sd_set_flush_flag(sdkp, &lim);
> +	sd_set_flush_flag(sdkp, lim);
>  
>  	/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
>  	dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
>  
>  	/* Some devices report a maximum block count for READ/WRITE requests. */
>  	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
> -	lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
> +	lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
>  
>  	if (sd_validate_min_xfer_size(sdkp))
> -		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
> +		lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
>  	else
> -		lim.io_min = 0;
> +		lim->io_min = 0;
>  
>  	/*
>  	 * Limit default to SCSI host optimal sector limit if set. There may be
>  	 * an impact on performance for when the size of a request exceeds this
>  	 * host limit.
>  	 */
> -	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
> +	lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
>  	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
> -		lim.io_opt = min_not_zero(lim.io_opt,
> +		lim->io_opt = min_not_zero(lim->io_opt,
>  				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
>  	}
>  
>  	sdkp->first_scan = 0;
>  
>  	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
> -	sd_config_write_same(sdkp, &lim);
> +	sd_config_write_same(sdkp, lim);
>  	kfree(buffer);
>  
> -	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
> +	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
>  	if (err)
>  		return err;
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] scsi: sd: Fix build warning in sd_revalidate_disk()

[Damien Le Moal]

On 8/8/25 20:30, Abinash Singh wrote:
> A build warning was triggered due to excessive stack usage in
> sd_revalidate_disk():
> 
> drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
> drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> This is caused by a large local struct queue_limits (~400B) allocated
> on the stack. Replacing it with a heap allocation using kmalloc()
> significantly reduces frame usage. Kernel stack is limited (~8 KB),
> and allocating large structs on the stack is discouraged.
> As the function already performs heap allocations (e.g. for buffer),
> this change fits well.
> 
> Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
> ---
> 
> Hi,
> 
> Thank you very much for your comments.
> I have addressed all your suggestions from v1.
> 
> As you mentioned concerns regarding the readability of
> the __free(kfree) attribute, I have used the classic
> approach in v2. Additionally, I will also send v3
> where the __free() attribute is used instead.
> 
> We can proceed with patch version you
> find more suitable, and do let me know if you have
> any further feedback.
> 
> changelog v1->v2:
> 	moved declarations together
> 	avoided "unreadable" cleanup attribute
> 	splited long line
> 	changed the log message to diiferentiate with buffer allocation
> 
> Thanks,
> 
> ---
>  drivers/scsi/sd.c | 49 +++++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4a68b2ab2804..f5ab2a422df6 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3696,7 +3696,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	struct scsi_disk *sdkp = scsi_disk(disk);
>  	struct scsi_device *sdp = sdkp->device;
>  	sector_t old_capacity = sdkp->capacity;
> -	struct queue_limits lim;
> +	struct queue_limits *lim;
>  	unsigned char *buffer;
>  	unsigned int dev_max;
>  	int err;

If you change this to "int err = 0", then...

> @@ -3711,23 +3711,30 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	if (!scsi_device_online(sdp))
>  		goto out;
>  
> +	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
> +	if (!lim) {
> +		sd_printk(KERN_WARNING, sdkp,
> +			"sd_revalidate_disk: Disk limit allocation failure.\n");
> +		goto out;
> +	}
> +
>  	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
>  	if (!buffer) {
>  		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
>  			  "allocation failure.\n");
> -		goto out;
> +		goto free_lim;

Yout can do a "goto out" here...

>  	}
>  

[...]

>  	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
> -	sd_config_write_same(sdkp, &lim);
> +	sd_config_write_same(sdkp, lim);
>  	kfree(buffer);

Move this line after the "out" label...

>  
> -	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
> -	if (err)
> +	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
> +	if (err) {

just do a goto out here...

> +		kfree(lim);
>  		return err;
> +	}
>  
>  	/*
>  	 * Query concurrent positioning ranges after
> @@ -3819,6 +3828,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	if (sd_zbc_revalidate_zones(sdkp))
>  		set_capacity_and_notify(disk, 0);
>  
> + free_lim:
> +	kfree(lim);
>   out:

And this becomes:

 out:
	kfree(lim);
	kfree(buffer)

	return err;

Much cleaner :)

>  	return 0;
>  }


-- 
Damien Le Moal
Western Digital Research

[PATCH v4] scsi: sd: Fix build warning in sd_revalidate_disk()

[Abinash Singh]

A build warning was triggered due to excessive stack usage in
sd_revalidate_disk():

drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]

This is caused by a large local struct queue_limits (~400B) allocated
on the stack. Replacing it with a heap allocation using kmalloc()
significantly reduces frame usage. Kernel stack is limited (~8 KB),
and allocating large structs on the stack is discouraged.
As the function already performs heap allocations (e.g. for buffer),
this change fits well.

Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
---


Changelog v2->v4:
	initialized `err` with 0
	removed extra label (`free_lim`) and merged it into `out`

Initialized `lim` and `buffer` with NULL, as  kfree()
can only handle `ZERO_OR_NULL_PTR` and a valid pointer.

>	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
>				      "sd_revalidate_disk\n"));
>@@ -3711,6 +3711,13 @@ static int sd_revalidate_disk(struct gendisk *disk)
>	if (!scsi_device_online(sdp))
>		goto out;

Here it will jump to out and uninitalized pointers will be passed to kfree()
>+	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
>+	if (!lim) {
>

Similarly in 2nd goto,.. uninitalized `buffer` will be passed to kfree();

It may cause page fault or make kernel panic.


Thanks

regards
	Abinash



---
 drivers/scsi/sd.c | 53 +++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4a68b2ab2804..c7fbfb801b40 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3696,10 +3696,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	struct scsi_device *sdp = sdkp->device;
 	sector_t old_capacity = sdkp->capacity;
-	struct queue_limits lim;
-	unsigned char *buffer;
+	struct queue_limits *lim = NULL;
+	unsigned char *buffer = NULL;
 	unsigned int dev_max;
-	int err;
+	int err = 0;
 
 	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
 				      "sd_revalidate_disk\n"));
@@ -3711,6 +3711,13 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (!scsi_device_online(sdp))
 		goto out;
 
+	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
+	if (!lim) {
+		sd_printk(KERN_WARNING, sdkp,
+			"sd_revalidate_disk: Disk limit allocation failure.\n");
+		goto out;
+	}
+
 	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
 	if (!buffer) {
 		sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
@@ -3720,14 +3727,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
 
 	sd_spinup_disk(sdkp);
 
-	lim = queue_limits_start_update(sdkp->disk->queue);
+	*lim = queue_limits_start_update(sdkp->disk->queue);
 
 	/*
 	 * Without media there is no reason to ask; moreover, some devices
 	 * react badly if we do.
 	 */
 	if (sdkp->media_present) {
-		sd_read_capacity(sdkp, &lim, buffer);
+		sd_read_capacity(sdkp, lim, buffer);
 		/*
 		 * Some USB/UAS devices return generic values for mode pages
 		 * until the media has been accessed. Trigger a READ operation
@@ -3741,17 +3748,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		 * cause this to be updated correctly and any device which
 		 * doesn't support it should be treated as rotational.
 		 */
-		lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
+		lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
 
 		if (scsi_device_supports_vpd(sdp)) {
 			sd_read_block_provisioning(sdkp);
-			sd_read_block_limits(sdkp, &lim);
+			sd_read_block_limits(sdkp, lim);
 			sd_read_block_limits_ext(sdkp);
-			sd_read_block_characteristics(sdkp, &lim);
-			sd_zbc_read_zones(sdkp, &lim, buffer);
+			sd_read_block_characteristics(sdkp, lim);
+			sd_zbc_read_zones(sdkp, lim, buffer);
 		}
 
-		sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
+		sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
 
 		sd_print_capacity(sdkp, old_capacity);
 
@@ -3761,47 +3768,46 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_app_tag_own(sdkp, buffer);
 		sd_read_write_same(sdkp, buffer);
 		sd_read_security(sdkp, buffer);
-		sd_config_protection(sdkp, &lim);
+		sd_config_protection(sdkp, lim);
 	}
 
 	/*
 	 * We now have all cache related info, determine how we deal
 	 * with flush requests.
 	 */
-	sd_set_flush_flag(sdkp, &lim);
+	sd_set_flush_flag(sdkp, lim);
 
 	/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
 	dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
 
 	/* Some devices report a maximum block count for READ/WRITE requests. */
 	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
-	lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
+	lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
 
 	if (sd_validate_min_xfer_size(sdkp))
-		lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
+		lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
 	else
-		lim.io_min = 0;
+		lim->io_min = 0;
 
 	/*
 	 * Limit default to SCSI host optimal sector limit if set. There may be
 	 * an impact on performance for when the size of a request exceeds this
 	 * host limit.
 	 */
-	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
+	lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
 	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
-		lim.io_opt = min_not_zero(lim.io_opt,
+		lim->io_opt = min_not_zero(lim->io_opt,
 				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
 	}
 
 	sdkp->first_scan = 0;
 
 	set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
-	sd_config_write_same(sdkp, &lim);
-	kfree(buffer);
+	sd_config_write_same(sdkp, lim);
 
-	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
+	err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
 	if (err)
-		return err;
+		goto out;
 
 	/*
 	 * Query concurrent positioning ranges after
@@ -3820,7 +3826,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		set_capacity_and_notify(disk, 0);
 
  out:
-	return 0;
+	kfree(lim);
+	kfree(buffer);
+
+	return err;
 }
 
 /**
-- 
2.50.1

Re: [PATCH v4] scsi: sd: Fix build warning in sd_revalidate_disk()

[Bart Van Assche]

On 8/8/25 11:28 AM, Abinash Singh wrote:
> A build warning was triggered due to excessive stack usage in
> sd_revalidate_disk():
 > [ ... ]

New versions of a patch should be posted as a new email thread instead
of a reply. Otherwise there is a significant chance that the new version
gets overlooked.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4a68b2ab2804..c7fbfb801b40 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3696,10 +3696,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   	struct scsi_disk *sdkp = scsi_disk(disk);
>   	struct scsi_device *sdp = sdkp->device;
>   	sector_t old_capacity = sdkp->capacity;
> -	struct queue_limits lim;
> -	unsigned char *buffer;
> +	struct queue_limits *lim = NULL;
> +	unsigned char *buffer = NULL;
>   	unsigned int dev_max;
> -	int err;
> +	int err = 0;
>   
>   	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
>   				      "sd_revalidate_disk\n"));
> @@ -3711,6 +3711,13 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   	if (!scsi_device_online(sdp))
>   		goto out;
>   
> +	lim = kmalloc(sizeof(*lim), GFP_KERNEL);
> +	if (!lim) {
> +		sd_printk(KERN_WARNING, sdkp,
> +			"sd_revalidate_disk: Disk limit allocation failure.\n");
> +		goto out;
> +	}

Returning a negative value for some errors and zero in case of success
or in case of memory allocation failure is confusing.

Since none of the sd_revalidate_disk() callers care about the value
returned by this function, please change the return type of this
function into 'void'. Since that change is logically independent of 
allocating 'lim' dynamically, that change probably should be a patch on
its own.

Thanks,

Bart.