[PATCH v5 0/2] scsi: sd: Fix build warning in sd_revalidate_disk()

[PATCH v5 0/2] scsi: sd: Fix build warning in sd_revalidate_disk()

[Abinash Singh]

This v5 series follows up on v4 of the "scsi: sd: Fix build warning in sd_revalidate_disk()"
patch. In v4 and earlier, the change was sent as a single patch.

As @Bart mentioned about changing the return type of function `sd_revalidate_disk()`

Based on the review/feedback, the change has now been split into two logically
independent patches:

  1. Change sd_revalidate_disk() return type to void.
  2. Fix build warning in sd_revalidate_disk()

The return type change removes unused and potentially misleading return
codes, since none of the callers care about the returned value.

The stack usage fix prevents large structures from being allocated on the
stack, improving safety and avoiding potential kernel stack overflows.

Changes since v4:
  - Split the original single patch into two patches.
  - Started a new email thread (not a reply) as suggested.

Link to v4:
https://lore.kernel.org/all/411260ff-d5c7-4f82-8c47-e66e4828c2b1@acm.org/



Abinash Singh (2):
  scsi: sd: make sd_revalidate_disk() return void
  scsi: sd: Fix build warning in sd_revalidate_disk()

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

-- 
2.50.1

[PATCH v5 1/2] scsi: sd: make sd_revalidate_disk() return void

[Abinash Singh]

The sd_revalidate_disk() function currently returns 0 for
both success and memory allocation failure.Since none of its
callers use the return value, this return code is both unnecessary
and potentially misleading.

Change the return type of sd_revalidate_disk() from int to void and remove all return value handling.
This makes the function semantics clearer and avoids confusion about unused return codes.

Signed-off-by: Abinash Singh <abinashsinghlalotra@gmail.com>
---
 drivers/scsi/sd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4a68b2ab2804..2f9381dcbcce 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -106,7 +106,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, struct queue_limits *lim,
 		unsigned int mode);
 static void sd_config_write_same(struct scsi_disk *sdkp,
 		struct queue_limits *lim);
-static int  sd_revalidate_disk(struct gendisk *);
+static void  sd_revalidate_disk(struct gendisk *);
 static void sd_unlock_native_capacity(struct gendisk *disk);
 static void sd_shutdown(struct device *);
 static void scsi_disk_release(struct device *cdev);
@@ -3691,7 +3691,7 @@ static void sd_read_block_zero(struct scsi_disk *sdkp)
  *	performs disk spin up, read_capacity, etc.
  *	@disk: struct gendisk we care about
  **/
-static int sd_revalidate_disk(struct gendisk *disk)
+static void sd_revalidate_disk(struct gendisk *disk)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
 	struct scsi_device *sdp = sdkp->device;
@@ -3801,7 +3801,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 
 	err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
 	if (err)
-		return err;
+		goto out;
 
 	/*
 	 * Query concurrent positioning ranges after
@@ -3820,7 +3820,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		set_capacity_and_notify(disk, 0);
 
  out:
-	return 0;
+	/* Placeholder for future cleanup */
 }
 
 /**
-- 
2.50.1

[PATCH v5 2/2] 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>
---
 drivers/scsi/sd.c | 48 +++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2f9381dcbcce..7e9c8d08120a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3696,8 +3696,8 @@ static void 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;
 
@@ -3711,6 +3711,13 @@ static void sd_revalidate_disk(struct gendisk *disk)
 	if (!scsi_device_online(sdp))
 		goto out;
 
+	lim = kmalloc(size(*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 void 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 void 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 +3768,44 @@ static void 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)
 		goto out;
 
@@ -3820,7 +3826,9 @@ static void sd_revalidate_disk(struct gendisk *disk)
 		set_capacity_and_notify(disk, 0);
 
  out:
-	/* Placeholder for future cleanup */
+	kfree(lim);
+	kfree(buffer);
+
 }
 
 /**
-- 
2.50.1

Re: [PATCH v5 2/2] scsi: sd: Fix build warning in sd_revalidate_disk()

[kernel test robot]

Hi Abinash,

kernel test robot noticed the following build errors:

[auto build test ERROR on jejb-scsi/for-next]
[also build test ERROR on mkp-scsi/for-next linus/master v6.16 next-20250808]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abinash-Singh/scsi-sd-make-sd_revalidate_disk-return-void/20250809-045941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/20250808205819.29517-3-abinashsinghlalotra%40gmail.com
patch subject: [PATCH v5 2/2] scsi: sd: Fix build warning in sd_revalidate_disk()
config: x86_64-buildonly-randconfig-002-20250809 (https://download.01.org/0day-ci/archive/20250809/202508091640.gvFPjI6O-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250809/202508091640.gvFPjI6O-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508091640.gvFPjI6O-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/scsi/sd.c:3709:16: error: call to undeclared function 'size'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    3709 |         lim = kmalloc(size(*lim), GFP_KERNEL);
         |                       ^
   drivers/scsi/sd.c:3709:16: note: did you mean 'ksize'?
   include/linux/slab.h:491:8: note: 'ksize' declared here
     491 | size_t ksize(const void *objp);
         |        ^
   1 error generated.


vim +/size +3709 drivers/scsi/sd.c

  3683	
  3684	/**
  3685	 *	sd_revalidate_disk - called the first time a new disk is seen,
  3686	 *	performs disk spin up, read_capacity, etc.
  3687	 *	@disk: struct gendisk we care about
  3688	 **/
  3689	static void sd_revalidate_disk(struct gendisk *disk)
  3690	{
  3691		struct scsi_disk *sdkp = scsi_disk(disk);
  3692		struct scsi_device *sdp = sdkp->device;
  3693		sector_t old_capacity = sdkp->capacity;
  3694		struct queue_limits *lim = NULL;
  3695		unsigned char *buffer = NULL;
  3696		unsigned int dev_max;
  3697		int err;
  3698	
  3699		SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
  3700					      "sd_revalidate_disk\n"));
  3701	
  3702		/*
  3703		 * If the device is offline, don't try and read capacity or any
  3704		 * of the other niceties.
  3705		 */
  3706		if (!scsi_device_online(sdp))
  3707			goto out;
  3708	
> 3709		lim = kmalloc(size(*lim), GFP_KERNEL);
  3710		if (!lim) {
  3711			sd_printk(KERN_WARNING, sdkp,
  3712				"sd_revalidate_disk: Disk limit allocation failure.\n");
  3713			goto out;
  3714		}
  3715	
  3716		buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL);
  3717		if (!buffer) {
  3718			sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory "
  3719				  "allocation failure.\n");
  3720			goto out;
  3721		}
  3722	
  3723		sd_spinup_disk(sdkp);
  3724	
  3725		*lim = queue_limits_start_update(sdkp->disk->queue);
  3726	
  3727		/*
  3728		 * Without media there is no reason to ask; moreover, some devices
  3729		 * react badly if we do.
  3730		 */
  3731		if (sdkp->media_present) {
  3732			sd_read_capacity(sdkp, lim, buffer);
  3733			/*
  3734			 * Some USB/UAS devices return generic values for mode pages
  3735			 * until the media has been accessed. Trigger a READ operation
  3736			 * to force the device to populate mode pages.
  3737			 */
  3738			if (sdp->read_before_ms)
  3739				sd_read_block_zero(sdkp);
  3740			/*
  3741			 * set the default to rotational.  All non-rotational devices
  3742			 * support the block characteristics VPD page, which will
  3743			 * cause this to be updated correctly and any device which
  3744			 * doesn't support it should be treated as rotational.
  3745			 */
  3746			lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
  3747	
  3748			if (scsi_device_supports_vpd(sdp)) {
  3749				sd_read_block_provisioning(sdkp);
  3750				sd_read_block_limits(sdkp, lim);
  3751				sd_read_block_limits_ext(sdkp);
  3752				sd_read_block_characteristics(sdkp, lim);
  3753				sd_zbc_read_zones(sdkp, lim, buffer);
  3754			}
  3755	
  3756			sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
  3757	
  3758			sd_print_capacity(sdkp, old_capacity);
  3759	
  3760			sd_read_write_protect_flag(sdkp, buffer);
  3761			sd_read_cache_type(sdkp, buffer);
  3762			sd_read_io_hints(sdkp, buffer);
  3763			sd_read_app_tag_own(sdkp, buffer);
  3764			sd_read_write_same(sdkp, buffer);
  3765			sd_read_security(sdkp, buffer);
  3766			sd_config_protection(sdkp, lim);
  3767		}
  3768	
  3769		/*
  3770		 * We now have all cache related info, determine how we deal
  3771		 * with flush requests.
  3772		 */
  3773		sd_set_flush_flag(sdkp, lim);
  3774	
  3775		/* Initial block count limit based on CDB TRANSFER LENGTH field size. */
  3776		dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
  3777	
  3778		/* Some devices report a maximum block count for READ/WRITE requests. */
  3779		dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
  3780		lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
  3781	
  3782		if (sd_validate_min_xfer_size(sdkp))
  3783			lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
  3784		else
  3785			lim->io_min = 0;
  3786	
  3787		/*
  3788		 * Limit default to SCSI host optimal sector limit if set. There may be
  3789		 * an impact on performance for when the size of a request exceeds this
  3790		 * host limit.
  3791		 */
  3792		lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
  3793		if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
  3794			lim->io_opt = min_not_zero(lim->io_opt,
  3795					logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
  3796		}
  3797	
  3798		sdkp->first_scan = 0;
  3799	
  3800		set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
  3801		sd_config_write_same(sdkp, lim);
  3802	
  3803		err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
  3804		if (err)
  3805			goto out;
  3806	
  3807		/*
  3808		 * Query concurrent positioning ranges after
  3809		 * queue_limits_commit_update() unlocked q->limits_lock to avoid
  3810		 * deadlock with q->sysfs_dir_lock and q->sysfs_lock.
  3811		 */
  3812		if (sdkp->media_present && scsi_device_supports_vpd(sdp))
  3813			sd_read_cpr(sdkp);
  3814	
  3815		/*
  3816		 * For a zoned drive, revalidating the zones can be done only once
  3817		 * the gendisk capacity is set. So if this fails, set back the gendisk
  3818		 * capacity to 0.
  3819		 */
  3820		if (sd_zbc_revalidate_zones(sdkp))
  3821			set_capacity_and_notify(disk, 0);
  3822	
  3823	 out:
  3824		kfree(lim);
  3825		kfree(buffer);
  3826	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki