| Message ID | 20201119101215.19223-2-claudiu.manoil@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | enetc: Clean endianness warnings up | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
On 11/19/20, Claudiu Manoil <claudiu.manoil@nxp.com> wrote: > These particular fields are specified in the H/W reference > manual as having network byte order format, so enforce big > endian annotation for them and clear the related sparse > warnings in the process. > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > index 68ef4f959982..04efccd11162 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > @@ -472,10 +472,10 @@ struct enetc_cmd_rfse { > u8 smac_m[6]; > u8 dmac_h[6]; > u8 dmac_m[6]; > - u32 sip_h[4]; > - u32 sip_m[4]; > - u32 dip_h[4]; > - u32 dip_m[4]; > + __be32 sip_h[4]; > + __be32 sip_m[4]; > + __be32 dip_h[4]; > + __be32 dip_m[4]; > u16 ethtype_h; > u16 ethtype_m; > u16 ethtype4_h; Hi Claudiu, Why the struct is declared without packed? I'm seeing that the structure is used in dma transfers in the driver > -- > 2.17.1 > >
>-----Original Message----- >From: Denis Kirjanov <kda@linux-powerpc.org> >Sent: Thursday, November 19, 2020 12:37 PM >To: Claudiu Manoil <claudiu.manoil@nxp.com> >Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; David S . >Miller <davem@davemloft.net> >Subject: Re: [PATCH net-next resend 1/2] enetc: Fix endianness issues for >enetc_ethtool > >On 11/19/20, Claudiu Manoil <claudiu.manoil@nxp.com> wrote: >> These particular fields are specified in the H/W reference >> manual as having network byte order format, so enforce big >> endian annotation for them and clear the related sparse >> warnings in the process. >> >> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> >> --- >> drivers/net/ethernet/freescale/enetc/enetc_hw.h | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> b/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> index 68ef4f959982..04efccd11162 100644 >> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h >> @@ -472,10 +472,10 @@ struct enetc_cmd_rfse { >> u8 smac_m[6]; >> u8 dmac_h[6]; >> u8 dmac_m[6]; >> - u32 sip_h[4]; >> - u32 sip_m[4]; >> - u32 dip_h[4]; >> - u32 dip_m[4]; >> + __be32 sip_h[4]; >> + __be32 sip_m[4]; >> + __be32 dip_h[4]; >> + __be32 dip_m[4]; >> u16 ethtype_h; >> u16 ethtype_m; >> u16 ethtype4_h; > >Hi Claudiu, > >Why the struct is declared without packed? >I'm seeing that the structure is used in dma transfers in the driver > Probably it should, for extra measure, but as it is right now the structure is packed, according to pahole: struct enetc_cmd_rfse { u8 smac_h[6]; /* 0 6 */ u8 smac_m[6]; /* 6 6 */ u8 dmac_h[6]; /* 12 6 */ u8 dmac_m[6]; /* 18 6 */ __be32 sip_h[4]; /* 24 16 */ __be32 sip_m[4]; /* 40 16 */ __be32 dip_h[4]; /* 56 16 */ /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */ __be32 dip_m[4]; /* 72 16 */ u16 ethtype_h; /* 88 2 */ u16 ethtype_m; /* 90 2 */ u16 ethtype4_h; /* 92 2 */ u16 ethtype4_m; /* 94 2 */ u16 sport_h; /* 96 2 */ u16 sport_m; /* 98 2 */ u16 dport_h; /* 100 2 */ u16 dport_m; /* 102 2 */ u16 vlan_h; /* 104 2 */ u16 vlan_m; /* 106 2 */ u8 proto_h; /* 108 1 */ u8 proto_m; /* 109 1 */ u16 flags; /* 110 2 */ u16 result; /* 112 2 */ u16 mode; /* 114 2 */ /* size: 116, cachelines: 2, members: 23 */ /* last cacheline: 52 bytes */ };
On Thu, 19 Nov 2020 13:37:21 +0300 Denis Kirjanov wrote: > On 11/19/20, Claudiu Manoil <claudiu.manoil@nxp.com> wrote: > > These particular fields are specified in the H/W reference > > manual as having network byte order format, so enforce big > > endian annotation for them and clear the related sparse > > warnings in the process. > > > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > --- > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > index 68ef4f959982..04efccd11162 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h > > @@ -472,10 +472,10 @@ struct enetc_cmd_rfse { > > u8 smac_m[6]; > > u8 dmac_h[6]; > > u8 dmac_m[6]; > > - u32 sip_h[4]; > > - u32 sip_m[4]; > > - u32 dip_h[4]; > > - u32 dip_m[4]; > > + __be32 sip_h[4]; > > + __be32 sip_m[4]; > > + __be32 dip_h[4]; > > + __be32 dip_m[4]; > > u16 ethtype_h; > > u16 ethtype_m; > > u16 ethtype4_h; > > Hi Claudiu, > > Why the struct is declared without packed? > I'm seeing that the structure is used in dma transfers in the driver We prefer not to pack structs unnecessarily in netdev, because it forces compilers to do inefficient loads on some arches. If the structure is laid out correctly according to normal C data layout rules it should be left alone. You can add compile times assertions on the size of the structures to make double-sure things don't break.
Claudiu Manoil wrote: > These particular fields are specified in the H/W reference > manual as having network byte order format, so enforce big > endian annotation for them and clear the related sparse > warnings in the process. > > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> Thanks for fixing these warnings! Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h index 68ef4f959982..04efccd11162 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h @@ -472,10 +472,10 @@ struct enetc_cmd_rfse { u8 smac_m[6]; u8 dmac_h[6]; u8 dmac_m[6]; - u32 sip_h[4]; - u32 sip_m[4]; - u32 dip_h[4]; - u32 dip_m[4]; + __be32 sip_h[4]; + __be32 sip_m[4]; + __be32 dip_h[4]; + __be32 dip_m[4]; u16 ethtype_h; u16 ethtype_m; u16 ethtype4_h;
These particular fields are specified in the H/W reference manual as having network byte order format, so enforce big endian annotation for them and clear the related sparse warnings in the process. Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc_hw.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)