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

[David Laight <David.Laight@ACULAB.COM>]

From: Martin Blumenstingl
> Sent: 04 January 2023 16:08
> 
> On Wed, Jan 4, 2023 at 4:53 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > 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@realtek.com> 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).
> My understanding is that there's one actual and one potential use-case.
> Let's start with the actual one in
> drivers/net/wireless/realtek/rtw88/rtw8821c.h:
>   struct rtw8821c_efuse {
>       __le16 rtl_id;
>       u8 res0[0x0e];
>       ...
> 
> The second one is a potential one, also in
> drivers/net/wireless/realtek/rtw88/rtw8821c.h if we replace the
> bitfields by an __le16 (which is my understanding how the data is
> modeled in the eFuse):
>   struct rtw8821ce_efuse {
>       ...
>       u8 serial_number[8];
>       __le16 cap_data; /* 0xf4 */
>       ...
> (I'm not sure about the "cap_data" name, but I think you get the point)

Both those seem to be aligned - provided the structure is aligned.

> > Possibly it should be 'u8 data2[2];'
> So you're saying we should replace the __le16 with u8 some_name[2];
> instead, then we don't need the __packed attribute.

But maybe you should look at defining the bitfields differently.
Change to __le16 is probably making it hard for yourself.
Perhaps you could #define a constant for each bitfield
so you can write an access function like:
	#define bitval(field, n) (field[n >> 16] >> ((n >> 8) & 7)) & (n & 0xff))
If 'n' is always a compile time constant the code will be fine.
Then add another define to create the 'n' based on values from the spec.
(Which could be offsets onto 16bit items on odd boundaries.)
Provided nothing crosses byte boundaries it should be fine and the
source code will be reasonably readable.

> > What you may want to do is add compile-time asserts for the
> > sizes of the structures.
> Do I get you right that something like:
>   BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256);
> is what you have in mind?

That looks like the one...

	David

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