| Message ID | 20260824014703.56502-1-kmehltretter@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: cfi_cmdset_0001: shrink do_write_buffer() stack frame | expand |
On Mon, 24 Aug 2026 03:47:03 +0200 Karl Mehltretter <kmehltretter@gmail.com> wrote: > arm32 allmodconfig fails to build with gcc: > > drivers/mtd/chips/cfi_cmdset_0001.c:1883:1: error: the frame size of 1296 > bytes is larger than 1280 bytes [-Werror=frame-larger-than=] > > With MTD_MAP_BANK_WIDTH_32 a map_word is 32 bytes, and with KASAN_STACK > every temporary gets its own redzoned slot. do_write_buffer() builds a > map_word for each of the twelve commands it issues, and those temporaries > dominate its frame. Looks to me like pretty much all of mtd/map.h should be real functions and not #defines or inlines. The bloat from all those expansions is going to be significant. They only time they get optimised to anything small is when only CONFIG_MTD_MAP_BANK_WIDTH_1 is set. The execution time of all those that do real io (especially reads) will be dominated by the io access itself. David > > Issue the commands through a small noinline helper that takes the > command as a plain value and builds the map_word in its own frame. The > sequence of reads and writes to the chip is unchanged. > > The helper must not be inlined, and reusing one map_word local is not > enough: clang cannot assume the callee does not observe the local, so it > still returns each CMD() into a per-call-site temporary, and the frame > grows (1760 -> 1824 bytes in cfi_intelext_writev(), into which clang > inlines do_write_buffer()). > > Frame sizes with MTD_MAP_BANK_WIDTH_32 and KASAN_STACK: > > gcc 15.2 do_write_buffer() 1296 -> 520 > clang 21 cfi_intelext_writev() 1760 -> 896 > > The helper itself takes 168 (gcc) / 160 (clang) bytes. > > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Closes: https://lore.kernel.org/all/20260408211407.2295175-1-andriy.shevchenko@linux.intel.com/ > Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com> > Link: https://lore.kernel.org/all/87ik9cfm6g.fsf@bootlin.com/ > Assisted-by: LLM > Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com> > --- > drivers/mtd/chips/cfi_cmdset_0001.c | 42 ++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c > index b73596a8e021..a7c77c4e76ae 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0001.c > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c > @@ -1716,12 +1716,24 @@ static int cfi_intelext_write_words (struct mtd_info *mtd, loff_t to , size_t le > } > > > +/* > + * Keep noinline: inlined, the map_word temporaries put do_write_buffer() over > + * the frame-size limit with MTD_MAP_BANK_WIDTH_32 and KASAN_STACK. > + */ > +static noinline void __xipram cfi_write_cmd(struct map_info *map, > + unsigned long cmd, unsigned long adr) > +{ > + struct cfi_private *cfi = map->fldrv_priv; > + > + map_write(map, CMD(cmd), adr); > +} > + > static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > unsigned long adr, const struct kvec **pvec, > unsigned long *pvec_seek, int len) > { > struct cfi_private *cfi = map->fldrv_priv; > - map_word status, write_cmd, datum; > + map_word status, datum; > unsigned long cmd_adr; > int ret, wbufsize, word_gap, words; > const struct kvec *vec; > @@ -1740,9 +1752,6 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > if (is_LH28F640BF(cfi)) > cmd_adr = adr; > > - /* Let's determine this according to the interleave only once */ > - write_cmd = (cfi->cfiq->P_ID != P_ID_INTEL_PERFORMANCE) ? CMD(0xe8) : CMD(0xe9); > - > mutex_lock(&chip->mutex); > ret = get_chip(map, chip, cmd_adr, FL_WRITING); > if (ret) { > @@ -1759,7 +1768,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > So we must check here and reset those bits if they're set. Otherwise > we're just pissing in the wind */ > if (chip->state != FL_STATUS) { > - map_write(map, CMD(0x70), cmd_adr); > + cfi_write_cmd(map, 0x70, cmd_adr); > chip->state = FL_STATUS; > } > status = map_read(map, cmd_adr); > @@ -1767,21 +1776,22 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > xip_enable(map, chip, cmd_adr); > printk(KERN_WARNING "SR.4 or SR.5 bits set in buffer write (status %lx). Clearing.\n", status.x[0]); > xip_disable(map, chip, cmd_adr); > - map_write(map, CMD(0x50), cmd_adr); > - map_write(map, CMD(0x70), cmd_adr); > + cfi_write_cmd(map, 0x50, cmd_adr); > + cfi_write_cmd(map, 0x70, cmd_adr); > } > > chip->state = FL_WRITING_TO_BUFFER; > - map_write(map, write_cmd, cmd_adr); > + cfi_write_cmd(map, (cfi->cfiq->P_ID != P_ID_INTEL_PERFORMANCE) ? 0xe8 : 0xe9, > + cmd_adr); > ret = WAIT_TIMEOUT(map, chip, cmd_adr, 0, 0); > if (ret) { > /* Argh. Not ready for write to buffer */ > map_word Xstatus = map_read(map, cmd_adr); > - map_write(map, CMD(0x70), cmd_adr); > + cfi_write_cmd(map, 0x70, cmd_adr); > chip->state = FL_STATUS; > status = map_read(map, cmd_adr); > - map_write(map, CMD(0x50), cmd_adr); > - map_write(map, CMD(0x70), cmd_adr); > + cfi_write_cmd(map, 0x50, cmd_adr); > + cfi_write_cmd(map, 0x70, cmd_adr); > xip_enable(map, chip, cmd_adr); > printk(KERN_ERR "%s: Chip not ready for buffer write. Xstatus = %lx, status = %lx\n", > map->name, Xstatus.x[0], status.x[0]); > @@ -1800,7 +1810,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > } > > /* Write length of data to come */ > - map_write(map, CMD(words), cmd_adr ); > + cfi_write_cmd(map, words, cmd_adr); > > /* Write data */ > vec = *pvec; > @@ -1837,7 +1847,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > *pvec_seek = vec_seek; > > /* GO GO GO */ > - map_write(map, CMD(0xd0), cmd_adr); > + cfi_write_cmd(map, 0xd0, cmd_adr); > chip->state = FL_WRITING; > > ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr, > @@ -1845,7 +1855,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > chip->buffer_write_time, > chip->buffer_write_time_max); > if (ret) { > - map_write(map, CMD(0x70), cmd_adr); > + cfi_write_cmd(map, 0x70, cmd_adr); > chip->state = FL_STATUS; > xip_enable(map, chip, cmd_adr); > printk(KERN_ERR "%s: buffer write error (status timeout)\n", map->name); > @@ -1858,8 +1868,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > unsigned long chipstatus = MERGESTATUS(status); > > /* reset status */ > - map_write(map, CMD(0x50), cmd_adr); > - map_write(map, CMD(0x70), cmd_adr); > + cfi_write_cmd(map, 0x50, cmd_adr); > + cfi_write_cmd(map, 0x70, cmd_adr); > xip_enable(map, chip, cmd_adr); > > if (chipstatus & 0x02) {
Hi David & Vignesh, On 24/08/2026 at 08:56:32 +01, David Laight <david.laight.linux@gmail.com> wrote: > On Mon, 24 Aug 2026 03:47:03 +0200 > Karl Mehltretter <kmehltretter@gmail.com> wrote: > >> arm32 allmodconfig fails to build with gcc: >> >> drivers/mtd/chips/cfi_cmdset_0001.c:1883:1: error: the frame size of 1296 >> bytes is larger than 1280 bytes [-Werror=frame-larger-than=] >> >> With MTD_MAP_BANK_WIDTH_32 a map_word is 32 bytes, and with KASAN_STACK >> every temporary gets its own redzoned slot. do_write_buffer() builds a >> map_word for each of the twelve commands it issues, and those temporaries >> dominate its frame. > > Looks to me like pretty much all of mtd/map.h should be real functions > and not #defines or inlines. > > The bloat from all those expansions is going to be significant. > They only time they get optimised to anything small is when only > CONFIG_MTD_MAP_BANK_WIDTH_1 is set. > > The execution time of all those that do real io (especially reads) > will be dominated by the io access itself. David, may I ask if you acknowledge the fix? Vignesh, same question, maybe you also test that there is no apparent regression? Thanks, Miquèl
On Mon, 24 Aug 2026 03:47:03 +0200, Karl Mehltretter wrote: > arm32 allmodconfig fails to build with gcc: > > drivers/mtd/chips/cfi_cmdset_0001.c:1883:1: error: the frame size of 1296 > bytes is larger than 1280 bytes [-Werror=frame-larger-than=] > > With MTD_MAP_BANK_WIDTH_32 a map_word is 32 bytes, and with KASAN_STACK > every temporary gets its own redzoned slot. do_write_buffer() builds a > map_word for each of the twelve commands it issues, and those temporaries > dominate its frame. > > [...] Applied to mtd/fixes, thanks! [1/1] mtd: cfi_cmdset_0001: shrink do_write_buffer() stack frame commit: 39b975ff208610301bc400460ec245caeffa4ee0 Patche(s) should be available on mtd/linux.git and will be part of the next PR (provided that no robot complains by then). Kind regards, Miquèl
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c index b73596a8e021..a7c77c4e76ae 100644 --- a/drivers/mtd/chips/cfi_cmdset_0001.c +++ b/drivers/mtd/chips/cfi_cmdset_0001.c @@ -1716,12 +1716,24 @@ static int cfi_intelext_write_words (struct mtd_info *mtd, loff_t to , size_t le } +/* + * Keep noinline: inlined, the map_word temporaries put do_write_buffer() over + * the frame-size limit with MTD_MAP_BANK_WIDTH_32 and KASAN_STACK. + */ +static noinline void __xipram cfi_write_cmd(struct map_info *map, + unsigned long cmd, unsigned long adr) +{ + struct cfi_private *cfi = map->fldrv_priv; + + map_write(map, CMD(cmd), adr); +} + static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, unsigned long adr, const struct kvec **pvec, unsigned long *pvec_seek, int len) { struct cfi_private *cfi = map->fldrv_priv; - map_word status, write_cmd, datum; + map_word status, datum; unsigned long cmd_adr; int ret, wbufsize, word_gap, words; const struct kvec *vec; @@ -1740,9 +1752,6 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, if (is_LH28F640BF(cfi)) cmd_adr = adr; - /* Let's determine this according to the interleave only once */ - write_cmd = (cfi->cfiq->P_ID != P_ID_INTEL_PERFORMANCE) ? CMD(0xe8) : CMD(0xe9); - mutex_lock(&chip->mutex); ret = get_chip(map, chip, cmd_adr, FL_WRITING); if (ret) { @@ -1759,7 +1768,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, So we must check here and reset those bits if they're set. Otherwise we're just pissing in the wind */ if (chip->state != FL_STATUS) { - map_write(map, CMD(0x70), cmd_adr); + cfi_write_cmd(map, 0x70, cmd_adr); chip->state = FL_STATUS; } status = map_read(map, cmd_adr); @@ -1767,21 +1776,22 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, xip_enable(map, chip, cmd_adr); printk(KERN_WARNING "SR.4 or SR.5 bits set in buffer write (status %lx). Clearing.\n", status.x[0]); xip_disable(map, chip, cmd_adr); - map_write(map, CMD(0x50), cmd_adr); - map_write(map, CMD(0x70), cmd_adr); + cfi_write_cmd(map, 0x50, cmd_adr); + cfi_write_cmd(map, 0x70, cmd_adr); } chip->state = FL_WRITING_TO_BUFFER; - map_write(map, write_cmd, cmd_adr); + cfi_write_cmd(map, (cfi->cfiq->P_ID != P_ID_INTEL_PERFORMANCE) ? 0xe8 : 0xe9, + cmd_adr); ret = WAIT_TIMEOUT(map, chip, cmd_adr, 0, 0); if (ret) { /* Argh. Not ready for write to buffer */ map_word Xstatus = map_read(map, cmd_adr); - map_write(map, CMD(0x70), cmd_adr); + cfi_write_cmd(map, 0x70, cmd_adr); chip->state = FL_STATUS; status = map_read(map, cmd_adr); - map_write(map, CMD(0x50), cmd_adr); - map_write(map, CMD(0x70), cmd_adr); + cfi_write_cmd(map, 0x50, cmd_adr); + cfi_write_cmd(map, 0x70, cmd_adr); xip_enable(map, chip, cmd_adr); printk(KERN_ERR "%s: Chip not ready for buffer write. Xstatus = %lx, status = %lx\n", map->name, Xstatus.x[0], status.x[0]); @@ -1800,7 +1810,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, } /* Write length of data to come */ - map_write(map, CMD(words), cmd_adr ); + cfi_write_cmd(map, words, cmd_adr); /* Write data */ vec = *pvec; @@ -1837,7 +1847,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, *pvec_seek = vec_seek; /* GO GO GO */ - map_write(map, CMD(0xd0), cmd_adr); + cfi_write_cmd(map, 0xd0, cmd_adr); chip->state = FL_WRITING; ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr, @@ -1845,7 +1855,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, chip->buffer_write_time, chip->buffer_write_time_max); if (ret) { - map_write(map, CMD(0x70), cmd_adr); + cfi_write_cmd(map, 0x70, cmd_adr); chip->state = FL_STATUS; xip_enable(map, chip, cmd_adr); printk(KERN_ERR "%s: buffer write error (status timeout)\n", map->name); @@ -1858,8 +1868,8 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, unsigned long chipstatus = MERGESTATUS(status); /* reset status */ - map_write(map, CMD(0x50), cmd_adr); - map_write(map, CMD(0x70), cmd_adr); + cfi_write_cmd(map, 0x50, cmd_adr); + cfi_write_cmd(map, 0x70, cmd_adr); xip_enable(map, chip, cmd_adr); if (chipstatus & 0x02) {
arm32 allmodconfig fails to build with gcc: drivers/mtd/chips/cfi_cmdset_0001.c:1883:1: error: the frame size of 1296 bytes is larger than 1280 bytes [-Werror=frame-larger-than=] With MTD_MAP_BANK_WIDTH_32 a map_word is 32 bytes, and with KASAN_STACK every temporary gets its own redzoned slot. do_write_buffer() builds a map_word for each of the twelve commands it issues, and those temporaries dominate its frame. Issue the commands through a small noinline helper that takes the command as a plain value and builds the map_word in its own frame. The sequence of reads and writes to the chip is unchanged. The helper must not be inlined, and reusing one map_word local is not enough: clang cannot assume the callee does not observe the local, so it still returns each CMD() into a per-call-site temporary, and the frame grows (1760 -> 1824 bytes in cfi_intelext_writev(), into which clang inlines do_write_buffer()). Frame sizes with MTD_MAP_BANK_WIDTH_32 and KASAN_STACK: gcc 15.2 do_write_buffer() 1296 -> 520 clang 21 cfi_intelext_writev() 1760 -> 896 The helper itself takes 168 (gcc) / 160 (clang) bytes. Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Closes: https://lore.kernel.org/all/20260408211407.2295175-1-andriy.shevchenko@linux.intel.com/ Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com> Link: https://lore.kernel.org/all/87ik9cfm6g.fsf@bootlin.com/ Assisted-by: LLM Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com> --- drivers/mtd/chips/cfi_cmdset_0001.c | 42 ++++++++++++++++++----------- 1 file changed, 26 insertions(+), 16 deletions(-)