Skip to contentSkip to conversation

mtd: spi-nor: avoid holes in struct spi_mem_op

PATCH Applied Arnd Bergmann 4 participants 5 messages Jul 19, 2023 – Jul 27, 2023

Subsystems (3)
Memory technology devices (MTD) SPI SPI NOR
lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

PATCH 1/1 mtd: spi-nor: avoid holes in struct spi_mem_op

From: Arnd Bergmann <[email protected]>

gcc gets confused when -ftrivial-auto-var-init=pattern is used on sparse bit fields such as 'struct spi_mem_op', which caused the previous false positive warning about an uninitialized variable:

drivers/mtd/spi-nor/spansion.c: error: 'op' is used uninitialized [-Werror=uninitialized]

In fact, the variable is fully initialized and gcc does not see it being used, so the warning is entirely bogus. The problem appears to be a misoptimization in the initialization of single bit fields when the rest of the bytes are not initialized.

A previous workaround added another initialization, which ended up shutting up the warning in spansion.c, though it apparently still happens in other files as reported by Peter Foley in the gcc bugzilla. The workaround of adding a fake initialization seems particularly bad because it would set values that can never be correct but prevent the compiler from warning about actually missing initializations.

Revert the broken workaround and instead pad the structure to only have bitfields that add up to full bytes, which should avoid this behavior in all drivers.

I also filed a new bug against gcc with what I found, so this can hopefully be addressed in future gcc releases. At the moment, only gcc-12 and gcc-13 are affected.

Cc Peter Foley <[email protected]>
Cc Pedro Falcato <[email protected]>
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110743
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108402
Link https://godbolt.org/z/efMMsG1Kx
Fixes 420c4495b5e56 ("mtd: spi-nor: spansion: make sure local struct does not contain garbage")
Signed-off-by Arnd Bergmann <[email protected]>
---
 drivers/mtd/spi-nor/spansion.c | 4 ++--
 include/linux/spi/spi-mem.h    | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)
drivers/mtd/spi-nor/spansion.c +2 -2
index 6d6466a3436e6..8397cf3cc3306 100644
@@ -361,7 +361,7 @@ static int cypress_nor_determine_addr_mode_by_sr1(struct spi_nor *nor,
  */
 static int cypress_nor_set_addr_mode_nbytes(struct spi_nor *nor)
 {
-	struct spi_mem_op op = {};
+	struct spi_mem_op op;
 	u8 addr_mode;
 	int ret;
 

@@ -492,7 +492,7 @@ s25fs256t_post_bfpt_fixup(struct spi_nor *nor,
 			  const struct sfdp_parameter_header *bfpt_header,
 			  const struct sfdp_bfpt *bfpt)
 {
-	struct spi_mem_op op = {};
+	struct spi_mem_op op;
 	int ret;
 
 	ret = cypress_nor_set_addr_mode_nbytes(nor);
include/linux/spi/spi-mem.h +4 -0
index 8e984d75f5b6c..6b0a7dc48a4b7 100644
@@ -101,6 +101,7 @@ struct spi_mem_op {
 		u8 nbytes;
 		u8 buswidth;
 		u8 dtr : 1;
+		u8 __pad : 7;
 		u16 opcode;
 	} cmd;
 

@@ -108,6 +109,7 @@ struct spi_mem_op {
 		u8 nbytes;
 		u8 buswidth;
 		u8 dtr : 1;
+		u8 __pad : 7;
 		u64 val;
 	} addr;
 

@@ -115,12 +117,14 @@ struct spi_mem_op {
 		u8 nbytes;
 		u8 buswidth;
 		u8 dtr : 1;
+		u8 __pad : 7;
 	} dummy;
 
 	struct {
 		u8 buswidth;
 		u8 dtr : 1;
 		u8 ecc : 1;
+		u8 __pad : 6;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
 		union {
Signature · 1 line
2.39.2
maintainer Acked-by on v1 1/1
#1 lore raw
Quoted 5 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

gcc gets confused when -ftrivial-auto-var-init=pattern is used on sparse bit fields such as 'struct spi_mem_op', which caused the previous false positive warning about an uninitialized variable:

Acked-by Mark Brown <[email protected]>
Acked-by on v1 1/1
#2 lore raw
In reply to Arnd Bergmann
Quoted 28 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

gcc gets confused when -ftrivial-auto-var-init=pattern is used on sparse bit fields such as 'struct spi_mem_op', which caused the previous false positive warning about an uninitialized variable:

drivers/mtd/spi-nor/spansion.c: error: 'op' is used uninitialized [-Werror=uninitialized]

In fact, the variable is fully initialized and gcc does not see it being used, so the warning is entirely bogus. The problem appears to be a misoptimization in the initialization of single bit fields when the rest of the bytes are not initialized.

A previous workaround added another initialization, which ended up shutting up the warning in spansion.c, though it apparently still happens in other files as reported by Peter Foley in the gcc bugzilla. The workaround of adding a fake initialization seems particularly bad because it would set values that can never be correct but prevent the compiler from warning about actually missing initializations.

Revert the broken workaround and instead pad the structure to only have bitfields that add up to full bytes, which should avoid this behavior in all drivers.

I also filed a new bug against gcc with what I found, so this can hopefully be addressed in future gcc releases. At the moment, only gcc-12 and gcc-13 are affected.

Quoted 7 lines
Cc Peter Foley <[email protected]>
Cc Pedro Falcato <[email protected]>
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110743
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108402
Link https://godbolt.org/z/efMMsG1Kx
Fixes 420c4495b5e56 ("mtd: spi-nor: spansion: make sure local struct does not contain garbage")
Signed-off-by Arnd Bergmann <[email protected]>
Acked-by Tudor Ambarus <[email protected]>

Miquel, would you please take this through mtd/fixes?

Cheers,
ta

maintainer
#3 lore raw

Hi Tudor,

[email protected] wrote on Thu, 20 Jul 2023 07:50:33 +0100:

Quoted 37 lines · Arnd Bergmann
Quoted 28 lines

From: Arnd Bergmann <[email protected]>

gcc gets confused when -ftrivial-auto-var-init=pattern is used on sparse bit fields such as 'struct spi_mem_op', which caused the previous false positive warning about an uninitialized variable:

drivers/mtd/spi-nor/spansion.c: error: 'op' is used uninitialized [-Werror=uninitialized]

In fact, the variable is fully initialized and gcc does not see it being used, so the warning is entirely bogus. The problem appears to be a misoptimization in the initialization of single bit fields when the rest of the bytes are not initialized.

A previous workaround added another initialization, which ended up shutting up the warning in spansion.c, though it apparently still happens in other files as reported by Peter Foley in the gcc bugzilla. The workaround of adding a fake initialization seems particularly bad because it would set values that can never be correct but prevent the compiler from warning about actually missing initializations.

Revert the broken workaround and instead pad the structure to only have bitfields that add up to full bytes, which should avoid this behavior in all drivers.

I also filed a new bug against gcc with what I found, so this can hopefully be addressed in future gcc releases. At the moment, only gcc-12 and gcc-13 are affected.

Quoted 7 lines
Cc Peter Foley <[email protected]>
Cc Pedro Falcato <[email protected]>
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110743
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108402
Link https://godbolt.org/z/efMMsG1Kx
Fixes 420c4495b5e56 ("mtd: spi-nor: spansion: make sure local struct does not contain garbage")
Signed-off-by Arnd Bergmann <[email protected]>
Quoted 3 lines
Acked-by Tudor Ambarus <[email protected]>

Miquel, would you please take this through mtd/fixes?

I will!

Thanks,
Miquèl

maintainer Applied
#4 lore raw
In reply to Arnd Bergmann
Quoted 28 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

gcc gets confused when -ftrivial-auto-var-init=pattern is used on sparse bit fields such as 'struct spi_mem_op', which caused the previous false positive warning about an uninitialized variable:

drivers/mtd/spi-nor/spansion.c: error: 'op' is used uninitialized [-Werror=uninitialized]

In fact, the variable is fully initialized and gcc does not see it being used, so the warning is entirely bogus. The problem appears to be a misoptimization in the initialization of single bit fields when the rest of the bytes are not initialized.

A previous workaround added another initialization, which ended up shutting up the warning in spansion.c, though it apparently still happens in other files as reported by Peter Foley in the gcc bugzilla. The workaround of adding a fake initialization seems particularly bad because it would set values that can never be correct but prevent the compiler from warning about actually missing initializations.

Revert the broken workaround and instead pad the structure to only have bitfields that add up to full bytes, which should avoid this behavior in all drivers.

I also filed a new bug against gcc with what I found, so this can hopefully be addressed in future gcc releases. At the moment, only gcc-12 and gcc-13 are affected.

Quoted 9 lines
Cc Peter Foley <[email protected]>
Cc Pedro Falcato <[email protected]>
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110743
Link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108402
Link https://godbolt.org/z/efMMsG1Kx
Fixes 420c4495b5e56 ("mtd: spi-nor: spansion: make sure local struct does not contain garbage")
Signed-off-by Arnd Bergmann <[email protected]>
Acked-by Mark Brown <[email protected]>
Acked-by Tudor Ambarus <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel