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(-)
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 */
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" <[email protected]> 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 <[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(-)
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
[email protected]
On Thu, 7 Feb 2008 10:51:52 -0500 "John W. Linville" <[email protected]> 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 <[email protected]> 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.
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" <[email protected]> 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 <[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(-)
>
> 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
On Wed, Feb 06, 2008 at 01:04:48PM -0800, Andrew Morton wrote:
> On Wed, 6 Feb 2008 21:50:23 +0100
> Helge Deller <[email protected]> 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.
On Thu, 7 Feb 2008 14:08:42 -0500
"John W. Linville" <[email protected]> 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 <[email protected]>
> Subject: [PATCH] wavelan: mark hardware interfacing structures as packed
>
> Signed-off-by: John W. Linville <[email protected]>
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.
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" <[email protected]> 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 <[email protected]> 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.
On Sunday 03 February 2008, Andrew Morton wrote:
> 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(-)
> >
> > 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.
On Thu, 7 Feb 2008 15:46:01 -0500
"John W. Linville" <[email protected]> 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.
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 <[email protected]> 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 <[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(-)
>
> 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.
On Wed, 6 Feb 2008 21:47:47 +0000
Russell King - ARM Linux <[email protected]> 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 <[email protected]> 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.
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(-)
>
> 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.
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 <[email protected]> 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 <[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(-)
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
[email protected]
On Wed, 6 Feb 2008 21:50:23 +0100
Helge Deller <[email protected]> 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/
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" <[email protected]> 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 <[email protected]> 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 <[email protected]>
Subject: [PATCH] wavelan: mark hardware interfacing structures as packed
Signed-off-by: John W. Linville <[email protected]>
---
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
[email protected]