RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

[David Laight]

From: Martin Blumenstingl
> Sent: 04 January 2023 15:30
> 
> Hi Ping-Ke, Hi David,
> 
> On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:
> [...]
> > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as
> I think this can be done in a separate patch.
> My v2 of this patch has reduced these changes to a minimum, see [0]
> 
> [...]
> > struct rtw8821ce_efuse {
> >    ...
> >    u8 data1;       // offset 0x100
> >    __le16 data2;   // offset 0x101-0x102
> >    ...
> > } __packed;
> >
> > Without __packed, compiler could has pad between data1 and data2,
> > and then get wrong result.
> My understanding is that this is the reason why we need __packed.

True, but does it really have to look like that?
I can't find that version (I don't have a net_next tree).
Possibly it should be 'u8 data2[2];'

Most hardware definitions align everything.

What you may want to do is add compile-time asserts for the
sizes of the structures.

Remember that if you have 16/32 bit fields in packed structures
on some architectures the compile has to generate code that does
byte loads and shifts.

The 'misaligned' property is lost when you take the address - so
you can easily generate a fault.

Adding __packed to a struct is a sledgehammer you really shouldn't need.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)