WAVELAN - compile-time check for struct sizes

Convert optional struct size checks to non-optional compile-time checks. Furthermore BUILD_BUG_ON() which will be optimized away by the compiler.

Signed-off-by Helge Deller <[email protected]>

 wavelan.c      |   34 +++++-----------------------------
 wavelan.p.h    |    1 -
 wavelan_cs.c   |   33 ++++-----------------------------
 wavelan_cs.p.h |    1 -
 4 files changed, 9 insertions(+), 60 deletions(-)

drivers/net/wireless/wavelan.c +0 -22

index a1f8a16..ffe50e2 100644
@@ -49,27 +49,6 @@ static int __init wv_psa_to_irq(u8 irqval)
 	return -1;
 }
 
-#ifdef STRUCT_CHECK
-/*------------------------------------------------------------------*/
-/*
- * Sanity routine to verify the sizes of the various WaveLAN interface
- * structures.
- */
-static char *wv_struct_check(void)
-{
-#define	SC(t,s,n)	if (sizeof(t) != s) return(n);
-
-	SC(psa_t, PSA_SIZE, "psa_t");
-	SC(mmw_t, MMW_SIZE, "mmw_t");
-	SC(mmr_t, MMR_SIZE, "mmr_t");
-	SC(ha_t, HA_SIZE, "ha_t");
-
-#undef	SC
-
-	return ((char *) NULL);
-}				/* wv_struct_check */
-#endif				/* STRUCT_CHECK */
-
 /********************* HOST ADAPTER SUBROUTINES *********************/
 /*
  * Useful subroutines to manage the WaveLAN ISA interface

@@ -4215,14 +4194,11 @@ struct net_device * __init wavelan_probe(int unit)
 	int i;
 	int r = 0;
 
-#ifdef	STRUCT_CHECK

Quoted 48 lines · On Sun, 13 Jan 2008 15:16:34 +0100 Helge Deller <[email protected]> wrote:

Convert optional struct size checks to non-optional compile-time checks. Furthermore BUILD_BUG_ON() which will be optimized away by the compiler.

Signed-off-by Helge Deller <[email protected]>

 wavelan.c      |   34 +++++-----------------------------
 wavelan.p.h    |    1 -
 wavelan_cs.c   |   33 ++++-----------------------------
 wavelan_cs.p.h |    1 -
 4 files changed, 9 insertions(+), 60 deletions(-)

drivers/net/wireless/wavelan.c +0 -22

index a1f8a16..ffe50e2 100644
@@ -49,27 +49,6 @@ static int __init wv_psa_to_irq(u8 irqval)
 	return -1;
 }
 
-#ifdef STRUCT_CHECK
-/*------------------------------------------------------------------*/
-/*
- * Sanity routine to verify the sizes of the various WaveLAN interface
- * structures.
- */
-static char *wv_struct_check(void)
-{
-#define	SC(t,s,n)	if (sizeof(t) != s) return(n);
-
-	SC(psa_t, PSA_SIZE, "psa_t");
-	SC(mmw_t, MMW_SIZE, "mmw_t");
-	SC(mmr_t, MMR_SIZE, "mmr_t");
-	SC(ha_t, HA_SIZE, "ha_t");
-
-#undef	SC
-
-	return ((char *) NULL);
-}				/* wv_struct_check */
-#endif				/* STRUCT_CHECK */
-
 /********************* HOST ADAPTER SUBROUTINES *********************/
 /*
  * Useful subroutines to manage the WaveLAN ISA interface

@@ -4215,14 +4194,11 @@ struct net_device * __init wavelan_probe(int unit)
 	int i;
 	int r = 0;
 
-#ifdef	STRUCT_CHECK

drivers/net/wireless/wavelan.c struct net_device * __init wavelan_probe(int unit) · line 4199

Quoted 10 lines

drivers/net/wireless/wavelan.c +3 -7

-	if (wv_struct_check() != (char *) NULL) {
-		printk(KERN_WARNING
-		       "%s: wavelan_probe(): structure/compiler botch: \"%s\"\n",
-		       dev->name, wv_struct_check());
-		return -ENODEV;
-	}
-#endif				/* STRUCT_CHECK */
+	/* compile-time check the sizes of structures */
+	BUILD_BUG_ON(sizeof(psa_t) != PSA_SIZE);
+	BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);

This assertion is now triggering with arm allmodconfig.

Rafael, please track this as a post-2.6.24 regression.

Quoted 6 lines

drivers/net/wireless/wavelan.c struct net_device * __init wavelan_probe(int unit) · line 4199

Quoted 3 lines

drivers/net/wireless/wavelan.c +3 -0

+	/* compile-time check the sizes of structures */
+	BUILD_BUG_ON(sizeof(psa_t) != PSA_SIZE);
+	BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);

This assertion is now triggering with arm allmodconfig.

Quoted 1 line

Rafael, please track this as a post-2.6.24 regression.

Hello Andrew,

with which arm platform did you found this assertion to trigger ?
I tried a few (e.g. ARM-poodle and CONFIG_ARCH_SA1100 w/ISA) but didn't saw it breaking. Maybe you could send me you .config file ?

PS: I tried Linus' current git tree which now includes my patch above as well.

Quoted 6 lines · On Wed, 6 Feb 2008 21:50:23 +0100 Helge Deller <[email protected]> wrote:

Quoted 6 lines

drivers/net/wireless/wavelan.c struct net_device * __init wavelan_probe(int unit) · line 4199

Quoted 3 lines

drivers/net/wireless/wavelan.c +3 -0

+	/* compile-time check the sizes of structures */
+	BUILD_BUG_ON(sizeof(psa_t) != PSA_SIZE);
+	BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);

This assertion is now triggering with arm allmodconfig.

Quoted 8 lines

Quoted 1 line

Rafael, please track this as a post-2.6.24 regression.

Hello Andrew,

with which arm platform did you found this assertion to trigger ?
I tried a few (e.g. ARM-poodle and CONFIG_ARCH_SA1100 w/ISA) but didn't saw it breaking. Maybe you could send me you .config file ?

allmodconfig

Quoted 2 lines

PS: I tried Linus' current git tree which now includes my patch above as well.

The assertion triggers with current mainline. I'm using gcc-3.4.5, from
http://userweb.kernel.org/~akpm/cross-compilers/

Quoted 21 lines · Andrew Morton

Quoted 6 lines · On Wed, 6 Feb 2008 21:50:23 +0100 Helge Deller <[email protected]> wrote:

Quoted 6 lines

drivers/net/wireless/wavelan.c struct net_device * __init wavelan_probe(int unit) · line 4199

Quoted 3 lines

drivers/net/wireless/wavelan.c +3 -0

+	/* compile-time check the sizes of structures */
+	BUILD_BUG_ON(sizeof(psa_t) != PSA_SIZE);
+	BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);

This assertion is now triggering with arm allmodconfig.

Quoted 8 lines

Quoted 1 line

Rafael, please track this as a post-2.6.24 regression.

Hello Andrew,

with which arm platform did you found this assertion to trigger ?
I tried a few (e.g. ARM-poodle and CONFIG_ARCH_SA1100 w/ISA) but didn't saw it breaking. Maybe you could send me you .config file ?

allmodconfig

Quoted 1 line

Quoted 4 lines

Quoted 1 line

PS: I tried Linus' current git tree which now includes my patch above as well.

The assertion triggers with current mainline. I'm using gcc-3.4.5, from
http://userweb.kernel.org/~akpm/cross-compilers/

I assume that it's the second BUILD_BUG_ON() which is triggering?

Given that:

#define MMW_SIZE 37

is not a multiple of sizeof(unsigned long) this is hardly surprising.

If structures are used to define a layout of something and must not contain compiler padding, it must be packed. Given these structures contain just unsigned char, there's no concerns about >8bit loads becoming less efficient.

Quoted 1 line · On Wed, 6 Feb 2008 21:47:47 +0000 Russell King - ARM Linux…

I assume that it's the second BUILD_BUG_ON() which is triggering?

yup.

Quoted 10 lines

Given that:

#define MMW_SIZE 37

is not a multiple of sizeof(unsigned long) this is hardly surprising.

If structures are used to define a layout of something and must not contain compiler padding, it must be packed. Given these structures contain just unsigned char, there's no concerns about >8bit loads becoming less efficient.

Quoted 17 lines · Andrew Morton

Quoted 1 line · On Wed, 6 Feb 2008 21:47:47 +0000 Russell King - ARM Linux…

I assume that it's the second BUILD_BUG_ON() which is triggering?

yup.

Quoted 10 lines

Given that:

#define MMW_SIZE 37

is not a multiple of sizeof(unsigned long) this is hardly surprising.

If structures are used to define a layout of something and must not contain compiler padding, it must be packed. Given these structures contain just unsigned char, there's no concerns about >8bit loads becoming less efficient.

Does a patch like this suffice? I haven't checked whether such a patch implies that the BUILD_BUG_ON()'s become unnecessary...

Does anyone actually have this hardware to test?

---

From: John W. Linville <[email protected]>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

Signed-off-by John W. Linville <[email protected]>

---
 drivers/net/wireless/wavelan.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/net/wireless/wavelan.h +0 -0

index 27172cd..964db3e 100644

Quoted 13 lines · Andrew Morton

Quoted 1 line · On Wed, 6 Feb 2008 21:47:47 +0000 Russell King - ARM Linux…

I assume that it's the second BUILD_BUG_ON() which is triggering?

yup.

Quoted 6 lines

Given that:

#define MMW_SIZE 37

is not a multiple of sizeof(unsigned long) this is hardly surprising.

Quoted 4 lines

Quoted 4 lines

If structures are used to define a layout of something and must not contain compiler padding, it must be packed. Given these structures contain just unsigned char, there's no concerns about >8bit loads becoming less efficient.

Does a patch like this suffice? I haven't checked whether such a patch implies that the BUILD_BUG_ON()'s become unnecessary...

Quoted 14 lines · On Thu, 7 Feb 2008 10:51:52 -0500 "John W. Linville" <[email protected]>…

Quoted 13 lines · Andrew Morton

Quoted 1 line · On Wed, 6 Feb 2008 21:47:47 +0000 Russell King - ARM Linux…

I assume that it's the second BUILD_BUG_ON() which is triggering?

yup.

Quoted 6 lines

Given that:

#define MMW_SIZE 37

is not a multiple of sizeof(unsigned long) this is hardly surprising.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

If structures are used to define a layout of something and must not contain compiler padding, it must be packed. Given these structures contain just unsigned char, there's no concerns about >8bit loads becoming less efficient.

Does a patch like this suffice? I haven't checked whether such a patch implies that the BUILD_BUG_ON()'s become unnecessary...

With your patch applied and arm allmodconfig, this

        BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);

triggers

Without your patch applied, these two

        BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);
        BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);

are triggering.

Quoted 35 lines · Andrew Morton

Quoted 14 lines · On Thu, 7 Feb 2008 10:51:52 -0500 "John W. Linville" <[email protected]>…

Quoted 13 lines · Andrew Morton

Quoted 1 line · On Wed, 6 Feb 2008 21:47:47 +0000 Russell King - ARM Linux…

I assume that it's the second BUILD_BUG_ON() which is triggering?

yup.

Quoted 6 lines

Given that:

#define MMW_SIZE 37

is not a multiple of sizeof(unsigned long) this is hardly surprising.

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

If structures are used to define a layout of something and must not contain compiler padding, it must be packed. Given these structures contain just unsigned char, there's no concerns about >8bit loads becoming less efficient.

Does a patch like this suffice? I haven't checked whether such a patch implies that the BUILD_BUG_ON()'s become unnecessary...

With your patch applied and arm allmodconfig, this

        BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);

triggers

Without your patch applied, these two

        BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);
        BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);

Quoted 1 line

are triggering.

The ha_t one triggers either way? Hmmm...

Russell suggested that the ha_t and psa_t packed attributes were unnecessary, so I'll include the reduced version just in case the above is a typo.

Quoted 4 lines

With your patch applied and arm allmodconfig, this

        BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);

Quoted 8 lines

triggers

Without your patch applied, these two

        BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);
        BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);

are triggering.

The ha_t one triggers either way? Hmmm...

Russell suggested that the ha_t and psa_t packed attributes were unnecessary, so I'll include the reduced version just in case the above is a typo.

---

From: John W. Linville <[email protected]>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

Signed-off-by John W. Linville <[email protected]>

Quoted 1 line · On Thu, 7 Feb 2008 14:08:42 -0500 "John W. Linville" <[email protected]>…

Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

ha_t is still triggering with this patch.

This incremental patch:

fixes things up. If forces `union hacs_u' to be two bytes, not four.

Quoted 1 line

fixes things up. If forces `union hacs_u' to be two bytes, not four.

OK, so I'll queue this one...'salright?

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Thu, 7 Feb 2008 14:08:42 -0500 "John W. Linville" <[email protected]>…

Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

ha_t is still triggering with this patch.

Quoted 1 line

This incremental patch:

<snip>

Quoted 1 line

fixes things up. If forces `union hacs_u' to be two bytes, not four.

OK, so I'll queue this one...'salright?

---

From: John W. Linville <[email protected]>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

With assists from Russell King <[email protected]> and Andrew Morton <[email protected]>...

Signed-off-by John W. Linville <[email protected]>

---
 drivers/net/wireless/wavelan.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/net/wireless/wavelan.h +3 -3

index 27172cd..9ab3605 100644
@@ -85,7 +85,7 @@ union hacs_u
 #define		HASR_MMC_INTR		0x0002	/* Interrupt request from MMC */
 #define		HASR_MMC_BUSY		0x0004	/* MMC busy indication */
 #define		HASR_PSA_BUSY		0x0008	/* LAN parameter storage area busy */
-};
+} __attribute__ ((packed));
 
 typedef struct ha_t	ha_t;
 struct ha_t

@@ -292,7 +292,7 @@ struct mmw_t
 #define	MMW_EXT_ANT_INTERNAL	0x00	/* Internal antenna */
 #define	MMW_EXT_ANT_EXTERNAL	0x03	/* External antenna */
 #define	MMW_EXT_ANT_IQ_TEST	0x1C	/* IQ test pattern (set to 0) */
-};
+} __attribute__ ((packed));
 
 #define	MMW_SIZE	37
 

@@ -347,7 +347,7 @@ struct mmr_t
   unsigned char	mmr_unused4[1];		/* unused */
   unsigned char	mmr_fee_data_l;		/* Read data from EEPROM (low) */
   unsigned char	mmr_fee_data_h;		/* Read data from EEPROM (high) */
-};
+} __attribute__ ((packed));
 
 #define	MMR_SIZE	36
 

Signature · 1 line

1.5.3.3