[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250807183000.31465-1-abinashsinghlalotra@gmail.com>
Date: Fri, 8 Aug 2025 00:00:00 +0530
From: Abinash Singh <abinashsinghlalotra@...il.com>
To: James.Bottomley@...senPartnership.com
Cc: martin.petersen@...cle.com,
linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org,
Abinash Singh <abinashsinghlalotra@...il.com>
Subject: [PATCH RFC] scsi: sd: Fix build warning in sd_revalidate_disk()
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@...il.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
Powered by blists - more mailing lists