Re: [PATCH v3 1/1] mtd: cfi_cmdset_0001: Factor out do_write_buffer_locked() to reduce stack frame

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: David Laight <david.laight.linux@gmail.com>,
	 Lukas Wunner <lukas@wunner.de>,
	 linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	 Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	 Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v3 1/1] mtd: cfi_cmdset_0001: Factor out do_write_buffer_locked() to reduce stack frame
Date: Mon, 27 Apr 2026 17:38:31 +0200	[thread overview]
Message-ID: <87ik9cfm6g.fsf@bootlin.com> (raw)
In-Reply-To: <ad41TpzL4onrcKzn@ashevche-desk.local> (Andy Shevchenko's message of "Tue, 14 Apr 2026 15:38:38 +0300")

Hello,

On 14/04/2026 at 15:38:38 +03, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Apr 09, 2026 at 12:28:46PM +0100, David Laight wrote:
>> On Thu, 9 Apr 2026 09:58:28 +0200
>> Lukas Wunner <lukas@wunner.de> wrote:
>> > On Thu, Apr 09, 2026 at 08:26:11AM +0100, David Laight wrote:
>> > > On Wed,  8 Apr 2026 23:11:48 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
>> > > > Compiler is not happy about used stack frame:
>> > > > 
>> > > > drivers/mtd/chips/cfi_cmdset_0001.c: In function 'do_write_buffer':
>> > > > drivers/mtd/chips/cfi_cmdset_0001.c:1887:1: error: the frame size of 1296 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
>> > > > 
>> > > > Fix this by factoring out do_write_buffer_locked().  
>> > > 
>> > > Does this just split the large stack frame between two nested functions?
>> > > I'd also expect the compiler to inline do_write_buffer_locked() so it
>> > > makes little difference.
>> > > OTOH I can't immediately see where the large stack frame comes from.  
>> > 
>> > The error occurs for an allmodconfig build on arm, which implies
>> > CONFIG_KASAN_STACK=y and thus increases stack usage vis-à-vis a
>> > "regular" build.
>> > 
>> > Stack usage is high here because of the three "map_word" types,
>> > which can each be up to 256 unsigned longs (32 * 8), see the
>> > definitions of MAX_MAP_LONGS, MAX_MAP_BANKWIDTH, map_word in
>> > include/linux/mtd/map.h.
>> 
>> Ugg - that code is horrid.
>> Returning structures by value isn't really a good idea.

Looks like the primary reason for the stack over usage, no? Isn't
playing with inline and refactoring just a tiny fix that prevents
problem by just a couple of bytes?

I haven't looked too carefully, but could we (Andy?) have a fix that
reduces the number of map_word (as suggested, IIUC) and/or avoid passing
them by value? I can also take this cleanup if enclosed in a bigger
series, I don't mind because it may make the code easier to read as
well, but I feel like this is not a proper fix. If it is, please explain
to me again :-)

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/