[PATCH] WAVELAN - compile-time check for struct sizes

[PATCH] WAVELAN - compile-time check for struct sizes

[Helge Deller]

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 <deller@gmx.de>

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

diff --git a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
index a1f8a16..ffe50e2 100644
--- a/drivers/net/wireless/wavelan.c
+++ b/drivers/net/wireless/wavelan.c
@@ -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
-	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);
+	BUILD_BUG_ON(sizeof(mmr_t) != MMR_SIZE);
+	BUILD_BUG_ON(sizeof(ha_t) != HA_SIZE);
 
 	dev = alloc_etherdev(sizeof(net_local));
 	if (!dev)
diff --git a/drivers/net/wireless/wavelan.p.h b/drivers/net/wireless/wavelan.p.h
index fe24281..b33ac47 100644
--- a/drivers/net/wireless/wavelan.p.h
+++ b/drivers/net/wireless/wavelan.p.h
@@ -400,7 +400,6 @@
  */
 #undef SET_PSA_CRC		/* Calculate and set the CRC on PSA (slower) */
 #define USE_PSA_CONFIG		/* Use info from the PSA. */
-#undef STRUCT_CHECK		/* Verify padding of structures. */
 #undef EEPROM_IS_PROTECTED	/* doesn't seem to be necessary */
 #define MULTICAST_AVOID		/* Avoid extra multicast (I'm sceptical). */
 #undef SET_MAC_ADDRESS		/* Experimental */
diff --git a/drivers/net/wireless/wavelan_cs.c b/drivers/net/wireless/wavelan_cs.c
index 577c647..f510eee 100644
--- a/drivers/net/wireless/wavelan_cs.c
+++ b/drivers/net/wireless/wavelan_cs.c
@@ -71,27 +71,6 @@ static void wv_nwid_filter(unsigned char mode, net_local *lp);
  * (wavelan modem or i82593)
  */
 
-#ifdef STRUCT_CHECK
-/*------------------------------------------------------------------*/
-/*
- * Sanity routine to verify the sizes of the various WaveLAN interface
- * structures.
- */
-static char *
-wv_structuct_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");
-
-#undef	SC
-
-  return((char *) NULL);
-} /* wv_structuct_check */
-#endif	/* STRUCT_CHECK */
-
 /******************* MODEM MANAGEMENT SUBROUTINES *******************/
 /*
  * Useful subroutines to manage the modem of the wavelan
@@ -3794,14 +3773,10 @@ wv_hw_config(struct net_device *	dev)
   printk(KERN_DEBUG "%s: ->wv_hw_config()\n", dev->name);
 #endif
 
-#ifdef STRUCT_CHECK
-  if(wv_structuct_check() != (char *) NULL)
-    {
-      printk(KERN_WARNING "%s: wv_hw_config: structure/compiler botch: \"%s\"\n",
-	     dev->name, wv_structuct_check());
-      return FALSE;
-    }
-#endif	/* STRUCT_CHECK == 1 */
+  /* compile-time check the sizes of structures */
+  BUILD_BUG_ON(sizeof(psa_t) != PSA_SIZE);
+  BUILD_BUG_ON(sizeof(mmw_t) != MMW_SIZE);
+  BUILD_BUG_ON(sizeof(mmr_t) != MMR_SIZE);
 
   /* Reset the pcmcia interface */
   if(wv_pcmcia_reset(dev) == FALSE)
diff --git a/drivers/net/wireless/wavelan_cs.p.h b/drivers/net/wireless/wavelan_cs.p.h
index 4b9de00..3627401 100644
--- a/drivers/net/wireless/wavelan_cs.p.h
+++ b/drivers/net/wireless/wavelan_cs.p.h
@@ -459,7 +459,6 @@
 #undef WAVELAN_ROAMING_EXT	/* Enable roaming wireless extensions */
 #undef SET_PSA_CRC		/* Set the CRC in PSA (slower) */
 #define USE_PSA_CONFIG		/* Use info from the PSA */
-#undef STRUCT_CHECK		/* Verify padding of structures */
 #undef EEPROM_IS_PROTECTED	/* Doesn't seem to be necessary */
 #define MULTICAST_AVOID		/* Avoid extra multicast (I'm sceptical) */
 #undef SET_MAC_ADDRESS		/* Experimental */

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Andrew Morton]

On Sun, 13 Jan 2008 15:16:34 +0100 Helge Deller <deller@gmx.de> 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 <deller@gmx.de>
> 
>  wavelan.c      |   34 +++++-----------------------------
>  wavelan.p.h    |    1 -
>  wavelan_cs.c   |   33 ++++-----------------------------
>  wavelan_cs.p.h |    1 -
>  4 files changed, 9 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
> index a1f8a16..ffe50e2 100644
> --- a/drivers/net/wireless/wavelan.c
> +++ b/drivers/net/wireless/wavelan.c
> @@ -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
> -	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.

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Helge Deller]

On Sunday 03 February 2008, Andrew Morton wrote:
> On Sun, 13 Jan 2008 15:16:34 +0100 Helge Deller <deller@gmx.de> 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 <deller@gmx.de>
> > 
> >  wavelan.c      |   34 +++++-----------------------------
> >  wavelan.p.h    |    1 -
> >  wavelan_cs.c   |   33 ++++-----------------------------
> >  wavelan_cs.p.h |    1 -
> >  4 files changed, 9 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
> > index a1f8a16..ffe50e2 100644
> > --- a/drivers/net/wireless/wavelan.c
> > +++ b/drivers/net/wireless/wavelan.c
> > @@ -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
> > -	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.

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 ?

Helge

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

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Andrew Morton]

On Wed, 6 Feb 2008 21:50:23 +0100
Helge Deller <deller@gmx.de> wrote:

> > > +	/* 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.
> 
> 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

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

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Russell King - ARM Linux]

On Wed, Feb 06, 2008 at 01:04:48PM -0800, Andrew Morton wrote:
> On Wed, 6 Feb 2008 21:50:23 +0100
> Helge Deller <deller@gmx.de> wrote:
> 
> > > > +	/* 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.
> > 
> > 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
> 
> > 
> > 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.

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Andrew Morton]

On Wed, 6 Feb 2008 21:47:47 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Wed, Feb 06, 2008 at 01:04:48PM -0800, Andrew Morton wrote:
> > On Wed, 6 Feb 2008 21:50:23 +0100
> > Helge Deller <deller@gmx.de> wrote:
> > 
> > > > > +	/* 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.
> > > 
> > > 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
> > 
> > > 
> > > 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?

yup.

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

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[John W. Linville]

On Wed, Feb 06, 2008 at 01:59:50PM -0800, Andrew Morton wrote:
> On Wed, 6 Feb 2008 21:47:47 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > I assume that it's the second BUILD_BUG_ON() which is triggering?
> 
> yup.
> 
> > 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 <linville@tuxdriver.com>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/wavelan.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/wavelan.h b/drivers/net/wireless/wavelan.h
index 27172cd..964db3e 100644
--- a/drivers/net/wireless/wavelan.h
+++ b/drivers/net/wireless/wavelan.h
@@ -100,7 +100,7 @@ struct ha_t
 	unsigned short	ha_piop1;	/* Program I/O Port 1 */
 	unsigned short	ha_pior2;	/* Program I/O Address Register Port 2 */
 	unsigned short	ha_piop2;	/* Program I/O Port 2 */
-};
+} __attribute__ ((packed));
 
 #define HA_SIZE		16
 
@@ -202,7 +202,7 @@ struct psa_t
   unsigned char	psa_conf_status;	/* [0x3C] Conf Status, bit 0=1:config*/
   unsigned char	psa_crc[2];		/* [0x3D] CRC-16 over PSA */
   unsigned char	psa_crc_status;		/* [0x3F] CRC Valid Flag */
-};
+} __attribute__ ((packed));
 
 #define	PSA_SIZE	64
 
@@ -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
 
-- 
1.5.3.3

-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Russell King - ARM Linux]

On Thu, Feb 07, 2008 at 10:51:52AM -0500, John W. Linville wrote:
> On Wed, Feb 06, 2008 at 01:59:50PM -0800, Andrew Morton wrote:
> > On Wed, 6 Feb 2008 21:47:47 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > I assume that it's the second BUILD_BUG_ON() which is triggering?
> > 
> > yup.
> > 
> > > 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 <linville@tuxdriver.com>
> Subject: [PATCH] wavelan: mark hardware interfacing structures as packed
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
>  drivers/net/wireless/wavelan.h |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/wavelan.h b/drivers/net/wireless/wavelan.h
> index 27172cd..964db3e 100644
> --- a/drivers/net/wireless/wavelan.h
> +++ b/drivers/net/wireless/wavelan.h
> @@ -100,7 +100,7 @@ struct ha_t
>  	unsigned short	ha_piop1;	/* Program I/O Port 1 */
>  	unsigned short	ha_pior2;	/* Program I/O Address Register Port 2 */
>  	unsigned short	ha_piop2;	/* Program I/O Port 2 */
> -};
> +} __attribute__ ((packed));

No need.

>  
>  #define HA_SIZE		16
>  
> @@ -202,7 +202,7 @@ struct psa_t
>    unsigned char	psa_conf_status;	/* [0x3C] Conf Status, bit 0=1:config*/
>    unsigned char	psa_crc[2];		/* [0x3D] CRC-16 over PSA */
>    unsigned char	psa_crc_status;		/* [0x3F] CRC Valid Flag */
> -};
> +} __attribute__ ((packed));
>  
>  #define	PSA_SIZE	64

No need.

>  
> @@ -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));;

Needed.

>  
>  #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));

Needed.

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Andrew Morton]

On Thu, 7 Feb 2008 10:51:52 -0500 "John W. Linville" <linville@tuxdriver.com> wrote:

> On Wed, Feb 06, 2008 at 01:59:50PM -0800, Andrew Morton wrote:
> > On Wed, 6 Feb 2008 21:47:47 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > I assume that it's the second BUILD_BUG_ON() which is triggering?
> > 
> > yup.
> > 
> > > 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...

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.

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[John W. Linville]

On Thu, Feb 07, 2008 at 10:49:12AM -0800, Andrew Morton wrote:
> On Thu, 7 Feb 2008 10:51:52 -0500 "John W. Linville" <linville@tuxdriver.com> wrote:
> 
> > On Wed, Feb 06, 2008 at 01:59:50PM -0800, Andrew Morton wrote:
> > > On Wed, 6 Feb 2008 21:47:47 +0000
> > > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 
> > > > I assume that it's the second BUILD_BUG_ON() which is triggering?
> > > 
> > > yup.
> > > 
> > > > 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...
> 
> 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.

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 <linville@tuxdriver.com>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/wavelan.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/wavelan.h b/drivers/net/wireless/wavelan.h
index 27172cd..fde4613 100644
--- a/drivers/net/wireless/wavelan.h
+++ b/drivers/net/wireless/wavelan.h
@@ -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
 
-- 
1.5.3.3

-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Russell King - ARM Linux]

On Thu, Feb 07, 2008 at 02:08:42PM -0500, John W. Linville wrote:
> On Thu, Feb 07, 2008 at 10:49:12AM -0800, Andrew Morton wrote:
> > On Thu, 7 Feb 2008 10:51:52 -0500 "John W. Linville" <linville@tuxdriver.com> wrote:
> > 
> > > On Wed, Feb 06, 2008 at 01:59:50PM -0800, Andrew Morton wrote:
> > > > On Wed, 6 Feb 2008 21:47:47 +0000
> > > > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > > 
> > > > > I assume that it's the second BUILD_BUG_ON() which is triggering?
> > > > 
> > > > yup.
> > > > 
> > > > > 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...
> > 
> > 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.
> 
> 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.

Well, I didn't look properly at ha_t and the unions that make it up
(#$@%@! typedefs.)

union hacs_u
{
        unsigned short  hu_command;             /* Command register */
        unsigned short  hu_status;              /* Status Register */
};

This is what needs to be packed, otherwise sizeof() will be 4.  With
that done, ha_t should come out at 16 bytes.

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Andrew Morton]

On Thu, 7 Feb 2008 14:08:42 -0500
"John W. Linville" <linville@tuxdriver.com> wrote:

> > 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.
> 
> 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 <linville@tuxdriver.com>
> Subject: [PATCH] wavelan: mark hardware interfacing structures as packed
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

ha_t is still triggering with this patch.

This incremental patch:

--- a/drivers/net/wireless/wavelan.h~a
+++ a/drivers/net/wireless/wavelan.h
@@ -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
_

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

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[John W. Linville]

On Thu, Feb 07, 2008 at 12:02:38PM -0800, Andrew Morton wrote:
> On Thu, 7 Feb 2008 14:08:42 -0500
> "John W. Linville" <linville@tuxdriver.com> wrote:

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

> ha_t is still triggering with this patch.
> 
> This incremental patch:

<snip>

> 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 <linville@tuxdriver.com>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed

With assists from Russell King <rmk+kernel@arm.linux.org.uk> and
Andrew Morton <akpm@linux-foundation.org>...

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/wavelan.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/wavelan.h b/drivers/net/wireless/wavelan.h
index 27172cd..9ab3605 100644
--- a/drivers/net/wireless/wavelan.h
+++ b/drivers/net/wireless/wavelan.h
@@ -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
 
-- 
1.5.3.3

-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Andrew Morton]

On Thu, 7 Feb 2008 15:46:01 -0500
"John W. Linville" <linville@tuxdriver.com> wrote:

> > fixes things up.  If forces `union hacs_u' to be two bytes, not four.
> 
> OK, so I'll queue this one...'salright?

yup, that compiles OK for me.

Re: [PATCH] WAVELAN - compile-time check for struct sizes

[Rafael J. Wysocki]

On Thursday, 7 of February 2008, John W. Linville wrote:
> On Thu, Feb 07, 2008 at 12:02:38PM -0800, Andrew Morton wrote:
> > On Thu, 7 Feb 2008 14:08:42 -0500
> > "John W. Linville" <linville@tuxdriver.com> wrote:
> 
> > > Subject: [PATCH] wavelan: mark hardware interfacing structures as packed
> 
> > ha_t is still triggering with this patch.
> > 
> > This incremental patch:
> 
> <snip>
> 
> > fixes things up.  If forces `union hacs_u' to be two bytes, not four.
> 
> OK, so I'll queue this one...'salright?

Can you please tell me what the status of this patch is?

Thanks,
Rafael


> ---
> 
> From: John W. Linville <linville@tuxdriver.com>
> Subject: [PATCH] wavelan: mark hardware interfacing structures as packed
> 
> With assists from Russell King <rmk+kernel@arm.linux.org.uk> and
> Andrew Morton <akpm@linux-foundation.org>...
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
>  drivers/net/wireless/wavelan.h |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/wavelan.h b/drivers/net/wireless/wavelan.h
> index 27172cd..9ab3605 100644
> --- a/drivers/net/wireless/wavelan.h
> +++ b/drivers/net/wireless/wavelan.h
> @@ -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
>  
> -- 
> 1.5.3.3
> 



-- 
"Premature optimization is the root of all evil." - Donald Knuth