| Message ID | 1623393419-2521-9-git-send-email-liweihang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: fix some coding-style issues | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: richardcochran@gmail.com linux@armlinux.org.uk quentin.schulz@bootlin.com ceggers@arri.de atenart@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Fri, Jun 11, 2021 at 02:36:59PM +0800, Weihang Li wrote: > From: Wenpeng Liang <liangwenpeng@huawei.com> > > Prefer __packed over __attribute__((__packed__)). > > Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com> > Signed-off-by: Weihang Li <liweihang@huawei.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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;