diff mbox series

[net-next,resend,1/2] enetc: Fix endianness issues for enetc_ethtool

Message ID 20201119101215.19223-2-claudiu.manoil@nxp.com
State Accepted
Headers show
Series enetc: Clean endianness warnings up | expand

Commit Message

Claudiu Manoil Nov. 19, 2020, 10:12 a.m. UTC
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(-)

Comments

Denis Kirjanov Nov. 19, 2020, 10:37 a.m. UTC | #1
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
>
>
Claudiu Manoil Nov. 19, 2020, 1:22 p.m. UTC | #2
>-----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 */
};
Jakub Kicinski Nov. 19, 2020, 5:54 p.m. UTC | #3
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.
Brandeburg, Jesse Nov. 23, 2020, 10:42 p.m. UTC | #4
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 mbox series

Patch

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;