| Message ID | 1623393419-2521-9-git-send-email-liweihang@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,1/8] net: phy: add a blank line after declarations | expand |
From: Weihang Li > Sent: 11 June 2021 07:37 > > Prefer __packed over __attribute__((__packed__)). > > Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com> > Signed-off-by: Weihang Li <liweihang@huawei.com> > --- > drivers/net/phy/mscc/mscc_ptp.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h > index da34653..01f78b4 100644 ... > /* Represents an entry in the timestamping FIFO */ > struct vsc85xx_ts_fifo { > u32 ns; > u64 secs:48; > u8 sig[16]; > -} __attribute__((__packed__)); > +} __packed; Hmmmm I'd take some convincing that 'u64 secs:48' is anything other than 'implementation defined'. So using it to map a hardware structure seems wrong. If this does map a hardware structure it ought to have 'endianness' annotations. If it doesn't then why the bitfield and why packed? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 2021/6/14 22:28, David Laight wrote: > From: Weihang Li >> Sent: 11 June 2021 07:37 >> >> Prefer __packed over __attribute__((__packed__)). >> >> Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com> >> Signed-off-by: Weihang Li <liweihang@huawei.com> >> --- >> drivers/net/phy/mscc/mscc_ptp.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h >> index da34653..01f78b4 100644 > ... >> /* Represents an entry in the timestamping FIFO */ >> struct vsc85xx_ts_fifo { >> u32 ns; >> u64 secs:48; >> u8 sig[16]; >> -} __attribute__((__packed__)); >> +} __packed; > > Hmmmm I'd take some convincing that 'u64 secs:48' is anything > other than 'implementation defined'. > So using it to map a hardware structure seems wrong. > > If this does map a hardware structure it ought to have > 'endianness' annotations. > If it doesn't then why the bitfield and why packed? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > > Hi David, Thank you for your attention. You are right, I found the contents of structure vsc85xx_ts_fifo is got from hardware. But I'm not sure if any issues or warnings will be introduced into this driver after just changing 'u64 secs:48' to '__be64 secs:48'. Let's keep this patch as it is. I cc the developers of the code, maybe they didn't realize it or had some reasons to define it like that. Thanks Weihang
From: liweihang > Sent: 16 June 2021 07:17 > > On 2021/6/14 22:28, David Laight wrote: > > From: Weihang Li > >> Sent: 11 June 2021 07:37 > >> > >> Prefer __packed over __attribute__((__packed__)). > >> > >> Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com> > >> Signed-off-by: Weihang Li <liweihang@huawei.com> > >> --- > >> drivers/net/phy/mscc/mscc_ptp.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h > >> index da34653..01f78b4 100644 > > ... > >> /* Represents an entry in the timestamping FIFO */ > >> struct vsc85xx_ts_fifo { > >> u32 ns; > >> u64 secs:48; > >> u8 sig[16]; > >> -} __attribute__((__packed__)); > >> +} __packed; > > > > Hmmmm I'd take some convincing that 'u64 secs:48' is anything > > other than 'implementation defined'. > > So using it to map a hardware structure seems wrong. > > > > If this does map a hardware structure it ought to have > > 'endianness' annotations. > > If it doesn't then why the bitfield and why packed? > > > > David > > Hi David, > > Thank you for your attention. You are right, I found the contents of structure > vsc85xx_ts_fifo is got from hardware. But I'm not sure if any issues or warnings > will be introduced into this driver after just changing 'u64 secs:48' to '__be64 > secs:48'. I've just checked what this structure looks like - see https://godbolt.org/z/h4EqbMoso Without any 'packed' annotations 'u64 secs:48' is aligned to an 8 byte boundary, but is only 6 bytes wide (I don't use bitfields) so the offset of 'sig' is 6 more than 'secs'. But the size of the whole structure looks wrong. I'd expect a hardware fifo so be a power of 2 big. This one is 26 bytes (as above) or 28 bytes if the 'packed' is only applied to 'secs' (which removed the 4 byte pad before it while still allowing aligned 4-byte accesses to the structure. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/phy/mscc/mscc_ptp.h b/drivers/net/phy/mscc/mscc_ptp.h index da34653..01f78b4 100644 --- a/drivers/net/phy/mscc/mscc_ptp.h +++ b/drivers/net/phy/mscc/mscc_ptp.h @@ -450,14 +450,14 @@ struct vsc85xx_ptphdr { __be16 seq_id; u8 ctrl; u8 log_interval; -} __attribute__((__packed__)); +} __packed; /* Represents an entry in the timestamping FIFO */ struct vsc85xx_ts_fifo { u32 ns; u64 secs:48; u8 sig[16]; -} __attribute__((__packed__)); +} __packed; struct vsc85xx_ptp { struct phy_device *phydev;