[PATCH v3] mtd: cfi: reduce stack size

[PATCH v3] mtd: cfi: reduce stack size

[Arnd Bergmann]

The cfi_staa_write_buffers function uses a large amount of kernel stack
whenever CONFIG_MTD_MAP_BANK_WIDTH_32 is set, and that results in a
warning on ARM allmodconfig builds:

drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
drivers/mtd/chips/cfi_cmdset_0020.c:651:1: warning: the frame size of 1208 bytes is larger than 1024 bytes [-Wframe-larger-than=]

It turns out that this is largely a result of a suboptimal implementation
of map_word_andequal(). Replacing this function with a straightforward
one reduces the stack size in this function by exactly 200 bytes,
shrinks the .text segment for this file from 27648 bytes to 26608 bytes,
and makes the warning go away.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v3: more whitespace changes
v2: whitespace changes

diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
index 47c59991491b..29975c73a953 100644
--- a/include/linux/mtd/map.h
+++ b/include/linux/mtd/map.h
@@ -316,7 +316,17 @@ static inline map_word map_word_or(struct map_info *map, map_word val1, map_word
 	return r;
 }
 
-#define map_word_andequal(m, a, b, z) map_word_equal(m, z, map_word_and(m, a, b))
+static inline int map_word_andequal(struct map_info *map, map_word val1, map_word val2, map_word val3)
+{
+	int i;
+
+	for (i = 0; i < map_words(map); i++) {
+		if ((val1.x[i] & val2.x[i]) != val3.x[i])
+			return 0;
+	}
+
+	return 1;
+}
 
 static inline int map_word_bitsset(struct map_info *map, map_word val1, map_word val2)
 {

[PATCH] mtd: clean up whitespace in linux/mtd/map.h

[Arnd Bergmann]

As the only comments I got for the "mtd: cfi: reduce stack size"
patch were about whitespace changes, it appears necessary to fix
up the rest of the file as well, which contains the exact same
mistakes.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
The two patches can be applied in either order, or this one dropped
entirely, I don't care as long as the other patch can get merged,
or get some constructive feedback.

diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
index 5f487d776411..47c59991491b 100644
--- a/include/linux/mtd/map.h
+++ b/include/linux/mtd/map.h
@@ -77,7 +77,7 @@
 /* ensure we never evaluate anything shorted than an unsigned long
  * to zero, and ensure we'll never miss the end of an comparison (bjd) */
 
-#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1))/ sizeof(unsigned long))
+#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1)) / sizeof(unsigned long))
 
 #ifdef CONFIG_MTD_MAP_BANK_WIDTH_8
 # ifdef map_bankwidth
@@ -181,7 +181,7 @@ static inline int map_bankwidth_supported(int w)
 	}
 }
 
-#define MAX_MAP_LONGS ( ((MAX_MAP_BANKWIDTH*8) + BITS_PER_LONG - 1) / BITS_PER_LONG )
+#define MAX_MAP_LONGS (((MAX_MAP_BANKWIDTH * 8) + BITS_PER_LONG - 1) / BITS_PER_LONG)
 
 typedef union {
 	unsigned long x[MAX_MAP_LONGS];
@@ -264,20 +264,22 @@ void unregister_mtd_chip_driver(struct mtd_chip_driver *);
 struct mtd_info *do_map_probe(const char *name, struct map_info *map);
 void map_destroy(struct mtd_info *mtd);
 
-#define ENABLE_VPP(map) do { if(map->set_vpp) map->set_vpp(map, 1); } while(0)
-#define DISABLE_VPP(map) do { if(map->set_vpp) map->set_vpp(map, 0); } while(0)
+#define ENABLE_VPP(map) do { if (map->set_vpp) map->set_vpp(map, 1); } while (0)
+#define DISABLE_VPP(map) do { if (map->set_vpp) map->set_vpp(map, 0); } while (0)
 
 #define INVALIDATE_CACHED_RANGE(map, from, size) \
-	do { if(map->inval_cache) map->inval_cache(map, from, size); } while(0)
+	do { if (map->inval_cache) map->inval_cache(map, from, size); } while (0)
 
 
 static inline int map_word_equal(struct map_info *map, map_word val1, map_word val2)
 {
 	int i;
-	for (i=0; i<map_words(map); i++) {
+
+	for (i = 0; i < map_words(map); i++) {
 		if (val1.x[i] != val2.x[i])
 			return 0;
 	}
+
 	return 1;
 }
 
@@ -286,9 +288,9 @@ static inline map_word map_word_and(struct map_info *map, map_word val1, map_wor
 	map_word r;
 	int i;
 
-	for (i=0; i<map_words(map); i++) {
+	for (i = 0; i < map_words(map); i++)
 		r.x[i] = val1.x[i] & val2.x[i];
-	}
+
 	return r;
 }
 
@@ -297,9 +299,9 @@ static inline map_word map_word_clr(struct map_info *map, map_word val1, map_wor
 	map_word r;
 	int i;
 
-	for (i=0; i<map_words(map); i++) {
+	for (i = 0; i < map_words(map); i++)
 		r.x[i] = val1.x[i] & ~val2.x[i];
-	}
+
 	return r;
 }
 
@@ -308,9 +310,9 @@ static inline map_word map_word_or(struct map_info *map, map_word val1, map_word
 	map_word r;
 	int i;
 
-	for (i=0; i<map_words(map); i++) {
+	for (i = 0; i < map_words(map); i++)
 		r.x[i] = val1.x[i] | val2.x[i];
-	}
+
 	return r;
 }
 
@@ -320,10 +322,11 @@ static inline int map_word_bitsset(struct map_info *map, map_word val1, map_word
 {
 	int i;
 
-	for (i=0; i<map_words(map); i++) {
+	for (i = 0; i < map_words(map); i++) {
 		if (val1.x[i] & val2.x[i])
 			return 1;
 	}
+
 	return 0;
 }
 
@@ -355,14 +358,16 @@ static inline map_word map_word_load_partial(struct map_info *map, map_word orig
 
 	if (map_bankwidth_is_large(map)) {
 		char *dest = (char *)&orig;
+
 		memcpy(dest+start, buf, len);
 	} else {
-		for (i=start; i < start+len; i++) {
+		for (i = start; i < start+len; i++) {
 			int bitpos;
+
 #ifdef __LITTLE_ENDIAN
-			bitpos = i*8;
+			bitpos = i * 8;
 #else /* __BIG_ENDIAN */
-			bitpos = (map_bankwidth(map)-1-i)*8;
+			bitpos = (map_bankwidth(map) - 1 - i) * 8;
 #endif
 			orig.x[0] &= ~(0xff << bitpos);
 			orig.x[0] |= (unsigned long)buf[i-start] << bitpos;
@@ -384,9 +389,10 @@ static inline map_word map_word_ff(struct map_info *map)
 
 	if (map_bankwidth(map) < MAP_FF_LIMIT) {
 		int bw = 8 * map_bankwidth(map);
+
 		r.x[0] = (1UL << bw) - 1;
 	} else {
-		for (i=0; i<map_words(map); i++)
+		for (i = 0; i < map_words(map); i++)
 			r.x[i] = ~0UL;
 	}
 	return r;
@@ -407,7 +413,7 @@ static inline map_word inline_map_read(struct map_info *map, unsigned long ofs)
 		r.x[0] = __raw_readq(map->virt + ofs);
 #endif
 	else if (map_bankwidth_is_large(map))
-		memcpy_fromio(r.x, map->virt+ofs, map->bankwidth);
+		memcpy_fromio(r.x, map->virt + ofs, map->bankwidth);
 	else
 		BUG();
 

[PATCH] mtd: clean up whitespace in linux/mtd/map.h

[Joe Perches]

On Tue, 2015-03-10 at 17:51 +0100, Arnd Bergmann wrote:
> As the only comments I got for the "mtd: cfi: reduce stack size"
> patch were about whitespace changes, it appears necessary to fix
> up the rest of the file as well, which contains the exact same
> mistakes.

trivia:

> diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
[]
> @@ -77,7 +77,7 @@
>  /* ensure we never evaluate anything shorted than an unsigned long
>   * to zero, and ensure we'll never miss the end of an comparison (bjd) */
>  
> -#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1))/ sizeof(unsigned long))
> +#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1)) / sizeof(unsigned long))

DIV_ROUND_UP?
 
>  #ifdef CONFIG_MTD_MAP_BANK_WIDTH_8
>  # ifdef map_bankwidth
> @@ -181,7 +181,7 @@ static inline int map_bankwidth_supported(int w)
>  	}
>  }
>  
> -#define MAX_MAP_LONGS ( ((MAX_MAP_BANKWIDTH*8) + BITS_PER_LONG - 1) / BITS_PER_LONG )
> +#define MAX_MAP_LONGS (((MAX_MAP_BANKWIDTH * 8) + BITS_PER_LONG - 1) / BITS_PER_LONG)

BITS_TO_LONGS?

[PATCH] mtd: clean up whitespace in linux/mtd/map.h

[Brian Norris]

On Tue, Mar 10, 2015 at 11:33:36AM -0700, Joe Perches wrote:
> On Tue, 2015-03-10 at 17:51 +0100, Arnd Bergmann wrote:
> > As the only comments I got for the "mtd: cfi: reduce stack size"
> > patch were about whitespace changes, it appears necessary to fix
> > up the rest of the file as well, which contains the exact same
> > mistakes.
> 
> trivia:
> 
> > diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
> []
> > @@ -77,7 +77,7 @@
> >  /* ensure we never evaluate anything shorted than an unsigned long
> >   * to zero, and ensure we'll never miss the end of an comparison (bjd) */
> >  
> > -#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1))/ sizeof(unsigned long))
> > +#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1)) / sizeof(unsigned long))
> 
> DIV_ROUND_UP?
>  
> >  #ifdef CONFIG_MTD_MAP_BANK_WIDTH_8
> >  # ifdef map_bankwidth
> > @@ -181,7 +181,7 @@ static inline int map_bankwidth_supported(int w)
> >  	}
> >  }
> >  
> > -#define MAX_MAP_LONGS ( ((MAX_MAP_BANKWIDTH*8) + BITS_PER_LONG - 1) / BITS_PER_LONG )
> > +#define MAX_MAP_LONGS (((MAX_MAP_BANKWIDTH * 8) + BITS_PER_LONG - 1) / BITS_PER_LONG)
> 
> BITS_TO_LONGS?

It seems the $subject patch is really not that necessary, as it was just
inspired by similarly trivial comments. But I thought CodingStyle
was supposed to mostly be a guide for new code, not a charter to "fix
up" old code like drivers/mtd/{chips,maps}.

So I would have been happy with ignoring the whitespace comments on the
v1 stack usage patch (esp. since it *did* match the existing style), and
avoiding the ensuing comments about helper macros. IMO, it's pretty
silly when a simple patch to fix a real issue turns into an extended
search for other trivial issues.

I'll probably take both of Arnd's patches as they stand, but any more
trivial requests to stable code like this should come in the form of
real patches, not respins of Arnd's patch.

Disclaimer: I'm probably just as guilty of adding trivial tangential
comments/requests that distract from the original issue. So I'm partly
speaking to myself here.

Happy coding,
Brian

[PATCH] mtd: clean up whitespace in linux/mtd/map.h

[Joe Perches]

On Tue, 2015-03-10 at 12:58 -0700, Brian Norris wrote:
> On Tue, Mar 10, 2015 at 11:33:36AM -0700, Joe Perches wrote:
> > On Tue, 2015-03-10 at 17:51 +0100, Arnd Bergmann wrote:
> > > As the only comments I got for the "mtd: cfi: reduce stack size"
> > > patch were about whitespace changes, it appears necessary to fix
> > > up the rest of the file as well, which contains the exact same
> > > mistakes.
> > 
> > trivia:
> > 
> > > diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
> > []
> > > @@ -77,7 +77,7 @@
> > >  /* ensure we never evaluate anything shorted than an unsigned long
> > >   * to zero, and ensure we'll never miss the end of an comparison (bjd) */
> > >  
> > > -#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1))/ sizeof(unsigned long))
> > > +#define map_calc_words(map) ((map_bankwidth(map) + (sizeof(unsigned long)-1)) / sizeof(unsigned long))
> > 
> > DIV_ROUND_UP?
> >  
> > >  #ifdef CONFIG_MTD_MAP_BANK_WIDTH_8
> > >  # ifdef map_bankwidth
> > > @@ -181,7 +181,7 @@ static inline int map_bankwidth_supported(int w)
> > >  	}
> > >  }
> > >  
> > > -#define MAX_MAP_LONGS ( ((MAX_MAP_BANKWIDTH*8) + BITS_PER_LONG - 1) / BITS_PER_LONG )
> > > +#define MAX_MAP_LONGS (((MAX_MAP_BANKWIDTH * 8) + BITS_PER_LONG - 1) / BITS_PER_LONG)
> > 
> > BITS_TO_LONGS?
> 
> It seems the $subject patch is really not that necessary,

Coding style patches rarely are.

> as it was just
> inspired by similarly trivial comments. But I thought CodingStyle
> was supposed to mostly be a guide for new code, not a charter to "fix
> up" old code like drivers/mtd/{chips,maps}.

'tisn't but consistency has its own virtue.

> So I would have been happy with ignoring the whitespace comments on the
> v1 stack usage patch (esp. since it *did* match the existing style), and
> avoiding the ensuing comments about helper macros. IMO, it's pretty
> silly when a simple patch to fix a real issue turns into an extended
> search for other trivial issues.
> 
> I'll probably take both of Arnd's patches as they stand,

No worries.   The comments weren't meant to derail
the original patches.

>  but any more
> trivial requests to stable code like this should come in the form of
> real patches, not respins of Arnd's patch.

I'm not respinning the patches.
If Arnd wants to do more work, that's up to him.

[PATCH] mtd: clean up whitespace in linux/mtd/map.h

[Brian Norris]

On Tue, Mar 10, 2015 at 05:51:55PM +0100, Arnd Bergmann wrote:
> As the only comments I got for the "mtd: cfi: reduce stack size"
> patch were about whitespace changes, it appears necessary to fix
> up the rest of the file as well, which contains the exact same
> mistakes.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> The two patches can be applied in either order, or this one dropped
> entirely, I don't care as long as the other patch can get merged,
> or get some constructive feedback.

Also applied to l2-mtd.git.

Brian

[PATCH v3] mtd: cfi: reduce stack size

[Brian Norris]

On Tue, Mar 10, 2015 at 05:48:37PM +0100, Arnd Bergmann wrote:
> The cfi_staa_write_buffers function uses a large amount of kernel stack
> whenever CONFIG_MTD_MAP_BANK_WIDTH_32 is set, and that results in a
> warning on ARM allmodconfig builds:
> 
> drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
> drivers/mtd/chips/cfi_cmdset_0020.c:651:1: warning: the frame size of 1208 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> It turns out that this is largely a result of a suboptimal implementation
> of map_word_andequal(). Replacing this function with a straightforward
> one reduces the stack size in this function by exactly 200 bytes,
> shrinks the .text segment for this file from 27648 bytes to 26608 bytes,
> and makes the warning go away.

Patch looks OK to me, and those results are nice.

Did you (or anyone else) do any testing?

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v3: more whitespace changes
> v2: whitespace changes
> 
> diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
> index 47c59991491b..29975c73a953 100644
> --- a/include/linux/mtd/map.h
> +++ b/include/linux/mtd/map.h
> @@ -316,7 +316,17 @@ static inline map_word map_word_or(struct map_info *map, map_word val1, map_word
>  	return r;
>  }
>  
> -#define map_word_andequal(m, a, b, z) map_word_equal(m, z, map_word_and(m, a, b))
> +static inline int map_word_andequal(struct map_info *map, map_word val1, map_word val2, map_word val3)
> +{
> +	int i;
> +
> +	for (i = 0; i < map_words(map); i++) {
> +		if ((val1.x[i] & val2.x[i]) != val3.x[i])
> +			return 0;
> +	}
> +
> +	return 1;
> +}
>  
>  static inline int map_word_bitsset(struct map_info *map, map_word val1, map_word val2)
>  {
> 

[PATCH v3] mtd: cfi: reduce stack size

[Arnd Bergmann]

On Tuesday 10 March 2015, Brian Norris wrote:
> On Tue, Mar 10, 2015 at 05:48:37PM +0100, Arnd Bergmann wrote:
> > The cfi_staa_write_buffers function uses a large amount of kernel stack
> > whenever CONFIG_MTD_MAP_BANK_WIDTH_32 is set, and that results in a
> > warning on ARM allmodconfig builds:
> > 
> > drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
> > drivers/mtd/chips/cfi_cmdset_0020.c:651:1: warning: the frame size of 1208 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > 
> > It turns out that this is largely a result of a suboptimal implementation
> > of map_word_andequal(). Replacing this function with a straightforward
> > one reduces the stack size in this function by exactly 200 bytes,
> > shrinks the .text segment for this file from 27648 bytes to 26608 bytes,
> > and makes the warning go away.
> 
> Patch looks OK to me, and those results are nice.
> 
> Did you (or anyone else) do any testing?

No runtime tests, I only built them, sorry.

	Arnd

[PATCH v3] mtd: cfi: reduce stack size

[Brian Norris]

On Tue, Mar 10, 2015 at 05:48:37PM +0100, Arnd Bergmann wrote:
> The cfi_staa_write_buffers function uses a large amount of kernel stack
> whenever CONFIG_MTD_MAP_BANK_WIDTH_32 is set, and that results in a
> warning on ARM allmodconfig builds:
> 
> drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
> drivers/mtd/chips/cfi_cmdset_0020.c:651:1: warning: the frame size of 1208 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> It turns out that this is largely a result of a suboptimal implementation
> of map_word_andequal(). Replacing this function with a straightforward
> one reduces the stack size in this function by exactly 200 bytes,
> shrinks the .text segment for this file from 27648 bytes to 26608 bytes,
> and makes the warning go away.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v3: more whitespace changes
> v2: whitespace changes

Applied to l2-mtd.git. Thanks!

Brian