mtd: cfi: reduce stack size

PATCH Applied Arnd Bergmann 3 participants 9 messages Mar 10, 2015 – Mar 11, 2015 Memory technology devices (MTD)

Share & tools

Arnd Bergmann

author

2015-03-10 16:48 UTC #0 lore raw

PATCH v3 1/1 mtd: cfi: reduce stack size

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 <[email protected]>

---
v3: more whitespace changes
v2: whitespace changes

include/linux/mtd/map.h +11 -1

index 47c59991491b..29975c73a953 100644
@@ -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)
 {

Arnd Bergmann

author

2015-03-10 16:51 UTC #1 lore raw

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

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 <[email protected]>

---
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.

include/linux/mtd/map.h +24 ~~-18~~

index 5f487d776411..47c59991491b 100644
@@ -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();

Joe Perches

2015-03-10 18:33 UTC #2 lore raw

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

Quoted 4 lines · 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.

trivia:

include/linux/mtd/map.h

Quoted 1 line

include/linux/mtd/map.h +0 -0

[]

include/linux/mtd/map.h line 80

Quoted 6 lines

include/linux/mtd/map.h +1 -1

@@ -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?

include/linux/mtd/map.h static inline int map_bankwidth_supported(int w) · line 184

Quoted 8 lines

include/linux/mtd/map.h +1 -1

 #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?

Brian Norris

2015-03-10 19:58 UTC #3 lore raw

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

Quoted 25 lines · Joe Perches

Quoted 4 lines · 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.
trivia:
include/linux/mtd/map.h
Quoted 1 line
include/linux/mtd/map.h +0 -0
[]
include/linux/mtd/map.h line 80
Quoted 6 lines
include/linux/mtd/map.h +1 -1
@@ -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?
Quoted 6 lines
include/linux/mtd/map.h +0 -0
 #ifdef CONFIG_MTD_MAP_BANK_WIDTH_8
 # ifdef map_bankwidth
@@ -181,7 +181,7 @@ static inline int map_bankwidth_supported(int w)
 	}
 }
 

Quoted 4 lines

include/linux/mtd/map.h static inline int map_bankwidth_supported(int w) · line 184
Quoted 2 lines
include/linux/mtd/map.h +1 -1
-#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

Joe Perches

2015-03-10 20:28 UTC #4 lore raw

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

Quoted 26 lines · Brian Norris

Quoted 25 lines · Joe Perches
Quoted 4 lines · 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.
trivia:
include/linux/mtd/map.h
Quoted 1 line
include/linux/mtd/map.h +0 -0
[]
include/linux/mtd/map.h line 80
Quoted 6 lines
include/linux/mtd/map.h +1 -1
@@ -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?
Quoted 6 lines
include/linux/mtd/map.h +0 -0
 #ifdef CONFIG_MTD_MAP_BANK_WIDTH_8
 # ifdef map_bankwidth
@@ -181,7 +181,7 @@ static inline int map_bankwidth_supported(int w)
 	}
 }
 

Quoted 6 lines

Quoted 4 lines
include/linux/mtd/map.h static inline int map_bankwidth_supported(int w) · line 184
Quoted 2 lines
include/linux/mtd/map.h +1 -1
-#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.

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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.

Brian Norris

2015-03-11 23:50 UTC #5 lore raw

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

In reply to Arnd Bergmann

Quoted 10 lines · 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 <[email protected]>
---
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

Brian Norris

2015-03-10 19:41 UTC #6 lore raw

In reply to Arnd Bergmann

Quoted 4 lines · 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:

Quoted 8 lines

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?

Quoted 29 lines

Signed-off-by Arnd Bergmann <[email protected]>

---
v3: more whitespace changes
v2: whitespace changes

include/linux/mtd/map.h +11 -1

index 47c59991491b..29975c73a953 100644
@@ -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)
 {

Arnd Bergmann

author

2015-03-10 21:00 UTC #7 lore raw

Quoted 14 lines · Brian Norris

Quoted 4 lines · 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:
Quoted 8 lines
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.

Quoted 3 lines

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

Brian Norris

Applied

2015-03-11 23:49 UTC #8 lore raw

In reply to Arnd Bergmann

Quoted 13 lines · 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.

Quoted 4 lines

Signed-off-by Arnd Bergmann <[email protected]>
---
v3: more whitespace changes
v2: whitespace changes

Applied to l2-mtd.git. Thanks!

Brian