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