b43 : unaligned access on mips

b43 : unaligned access on mips

[matthieu castet]

Hi,

b43_generate_plcp_hdr generate unaligned access on mips with gcc [1] 
from openwrt.

A small testcase [2] show that &plcp->data is access as a 32 bit aligned 
variable (see the "lw      $2,0($4)" and "sw      $2,0($4)").
I don't know enough mips to know if it is a gcc bug (ignoring the packed 
attribute) or something missing in b43 code.


Matthieu



[1]
mipsel-openwrt-linux-uclibc-gcc -v
Using built-in specs.
Target: mipsel-openwrt-linux-uclibc
Configured with: 
/mnt/data/routeur/trunk/build_dir/toolchain-mipsel_gcc-4.1.2_uClibc-0.9.29/gcc-4.1.2/configure 
--prefix=/mnt/data/routeur/trunk/staging_dir/toolchain-mipsel_gcc-4.1.2_uClibc-0.9.29/usr 
--build=i486-linux-gnu --host=i486-linux-gnu 
--target=mipsel-openwrt-linux-uclibc --with-gnu-ld 
--enable-target-optspace --disable-libgomp --disable-libmudflap 
--disable-multilib --disable-nls --disable-libssp --disable-__cxa_atexit 
--enable-languages=c,c++ --enable-shared --enable-threads 
--with-slibdir=/mnt/data/routeur/trunk/staging_dir/toolchain-mipsel_gcc-4.1.2_uClibc-0.9.29/lib
Thread model: posix
gcc version 4.1.2

[2]
$cat test.c
#define __le32 unsigned int
#define u32 unsigned int
#define __u8 unsigned char
#define u8 unsigned char
#define u16 unsigned short
#define cpu_to_le32(x) (x)

#define _b43_declare_plcp_hdr(size) \
         struct b43_plcp_hdr##size {             \
                 union {                         \
                         __le32 data;            \
                         __u8 raw[size];         \
                 } __attribute__((__packed__));  \
         } __attribute__((__packed__))

/* struct b43_plcp_hdr4 */
_b43_declare_plcp_hdr(4);

void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
                                const u16 octets, const u8 bitrate)
{
                 u32 plen;
             __le32 *data = &(plcp->data);
                 plen = octets * 16 / bitrate;
                 *data |= cpu_to_le32(plen << 16);
}

$mipsel-openwrt-linux-uclibc-gcc test.c -Os -S
$cat test.s
         .file   1 "test.c"
         .section .mdebug.abi32
         .previous
         .abicalls
         .text
         .align  2
         .globl  b43_generate_plcp_hdr
         .ent    b43_generate_plcp_hdr
         .type   b43_generate_plcp_hdr, @function
b43_generate_plcp_hdr:
         .frame  $sp,0,$31               # vars= 0, regs= 0/0, args= 0, 
gp= 0
         .mask   0x00000000,0
         .fmask  0x00000000,0
         .set    noreorder
         .set    nomacro

         andi    $5,$5,0xffff
         sll     $5,$5,4
         andi    $6,$6,0x00ff
         bne     $6,$0,1f
         div     $0,$5,$6
         break   7
1:
         lw      $2,0($4)
         mflo    $5
         sll     $5,$5,16
         or      $2,$2,$5
         j       $31
         sw      $2,0($4)

         .set    macro
         .set    reorder
         .end    b43_generate_plcp_hdr
         .ident  "GCC: (GNU) 4.1.2"

Re: b43 : unaligned access on mips

[matthieu castet]

matthieu castet wrote:
> Hi,
> 
> b43_generate_plcp_hdr generate unaligned access on mips with gcc [1] 
> from openwrt.
> 
> A small testcase [2] show that &plcp->data is access as a 32 bit aligned 
> variable (see the "lw      $2,0($4)" and "sw      $2,0($4)").
> I don't know enough mips to know if it is a gcc bug (ignoring the packed 
> attribute) or something missing in b43 code.
For example using "plcp->data" instead "*data" produce the correct code.

Re: b43 : unaligned access on mips

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Tue, 2009-06-02 at 00:00 +0200, matthieu castet wrote:
> matthieu castet wrote:
> > Hi,
> > 
> > b43_generate_plcp_hdr generate unaligned access on mips with gcc [1] 
> > from openwrt.
> > 
> > A small testcase [2] show that &plcp->data is access as a 32 bit aligned 
> > variable (see the "lw      $2,0($4)" and "sw      $2,0($4)").
> > I don't know enough mips to know if it is a gcc bug (ignoring the packed 
> > attribute) or something missing in b43 code.
> For example using "plcp->data" instead "*data" produce the correct code.

That's valid, send a patch.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: b43 : unaligned access on mips

[Michael Buesch]

On Tuesday 02 June 2009 00:00:12 matthieu castet wrote:
> matthieu castet wrote:
> > Hi,
> > 
> > b43_generate_plcp_hdr generate unaligned access on mips with gcc [1] 
> > from openwrt.
> > 
> > A small testcase [2] show that &plcp->data is access as a 32 bit aligned 
> > variable (see the "lw      $2,0($4)" and "sw      $2,0($4)").
> > I don't know enough mips to know if it is a gcc bug (ignoring the packed 
> > attribute) or something missing in b43 code.
> For example using "plcp->data" instead "*data" produce the correct code.

Uhm, I'm not sure. This code has been in the driver as-is forever and I don't
see any unaligned access issues on mips (I checked a month or so ago).
The plcp data structure is also __attribute__((packed)), so there can't be any
unaligned accesses, as gcc is required to _expect_ unaligned accesses on structures
with packed attribute. So it is required to do byte accesses on architectures where
alignment matters.
So I don't think there's an issue in the code.

-- 
Greetings, Michael.

Re: b43 : unaligned access on mips

[matthieu castet]

Hi Michael,

Michael Buesch wrote:
> On Tuesday 02 June 2009 00:00:12 matthieu castet wrote:
>> matthieu castet wrote:
>>> Hi,
>>>
>>> b43_generate_plcp_hdr generate unaligned access on mips with gcc [1] 
>>> from openwrt.
>>>
>>> A small testcase [2] show that &plcp->data is access as a 32 bit aligned 
>>> variable (see the "lw      $2,0($4)" and "sw      $2,0($4)").
>>> I don't know enough mips to know if it is a gcc bug (ignoring the packed 
>>> attribute) or something missing in b43 code.
>> For example using "plcp->data" instead "*data" produce the correct code.
> 
> Uhm, I'm not sure. This code has been in the driver as-is forever and I don't
> see any unaligned access issues on mips (I checked a month or so ago).
Where does this pointer come from ? May be other code change change this 
pointer alignement?
Which toolchain did you use ?
Could you try to build the testcase I posted and see which code it 
generate ?


> The plcp data structure is also __attribute__((packed)), so there can't be any
> unaligned accesses, as gcc is required to _expect_ unaligned accesses on structures
> with packed attribute. So it is required to do byte accesses on architectures where
> alignment matters.
> So I don't think there's an issue in the code.
> 
But the code doesn't use the structure to access the data.
It use by an extra pointer "data", and I believe gcc loose the packed 
info (use byte accesses) when we do the "u32 *data = &plcp->data".


Matthieu

Re: b43 : unaligned access on mips

[Michael Buesch]

On Tuesday 02 June 2009 20:17:48 matthieu castet wrote:
> Hi Michael,
> 
> Michael Buesch wrote:
> > On Tuesday 02 June 2009 00:00:12 matthieu castet wrote:
> >> matthieu castet wrote:
> >>> Hi,
> >>>
> >>> b43_generate_plcp_hdr generate unaligned access on mips with gcc [1] 
> >>> from openwrt.
> >>>
> >>> A small testcase [2] show that &plcp->data is access as a 32 bit aligned 
> >>> variable (see the "lw      $2,0($4)" and "sw      $2,0($4)").
> >>> I don't know enough mips to know if it is a gcc bug (ignoring the packed 
> >>> attribute) or something missing in b43 code.
> >> For example using "plcp->data" instead "*data" produce the correct code.
> > 
> > Uhm, I'm not sure. This code has been in the driver as-is forever and I don't
> > see any unaligned access issues on mips (I checked a month or so ago).
> Where does this pointer come from ? May be other code change change this 
> pointer alignement?
> Which toolchain did you use ?
> Could you try to build the testcase I posted and see which code it 
> generate ?

OpenWRT trunk.

> > The plcp data structure is also __attribute__((packed)), so there can't be any
> > unaligned accesses, as gcc is required to _expect_ unaligned accesses on structures
> > with packed attribute. So it is required to do byte accesses on architectures where
> > alignment matters.
> > So I don't think there's an issue in the code.
> > 
> But the code doesn't use the structure to access the data.
> It use by an extra pointer "data", and I believe gcc loose the packed 
> info (use byte accesses) when we do the "u32 *data = &plcp->data".

Ok, yeah. Maybe they changed semantics then.

Can you send a patch please?
I do actually prefer patches anyway instead of verbose explanations. I'd have
immediately understood what you were talking about, if you'd just sent a patch. ;)

-- 
Greetings, Michael.

Re: b43 : unaligned access on mips

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]

Michael Buesch wrote:
> On Tuesday 02 June 2009 20:17:48 matthieu castet wrote:
> Can you send a patch please?
> I do actually prefer patches anyway instead of verbose explanations. I'd have
> immediately understood what you were talking about, if you'd just sent a patch. ;)
> 
Here you are

[-- Attachment #2: b43_unaligned.diff --]
[-- Type: text/x-diff, Size: 1144 bytes --]

Fix an unaligned access on b43.
unaligned data is read/write with a u32 pointer instead of using the
packed structure.

Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
index a63d888..55f36a7 100644
--- a/drivers/net/wireless/b43/xmit.c
+++ b/drivers/net/wireless/b43/xmit.c
@@ -118,7 +118,6 @@ u8 b43_plcp_get_ratecode_ofdm(const u8 bitrate)
 void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
 			   const u16 octets, const u8 bitrate)
 {
-	__le32 *data = &(plcp->data);
 	__u8 *raw = plcp->raw;
 
 	if (b43_is_ofdm_rate(bitrate)) {
@@ -127,7 +126,7 @@ void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
 		d = b43_plcp_get_ratecode_ofdm(bitrate);
 		B43_WARN_ON(octets & 0xF000);
 		d |= (octets << 5);
-		*data = cpu_to_le32(d);
+		plcp->data = cpu_to_le32(d);
 	} else {
 		u32 plen;
 
@@ -141,7 +140,7 @@ void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
 				raw[1] = 0x04;
 		} else
 			raw[1] = 0x04;
-		*data |= cpu_to_le32(plen << 16);
+		plcp->data |= cpu_to_le32(plen << 16);
 		raw[0] = b43_plcp_get_ratecode_cck(bitrate);
 	}
 }