RE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Danielle Ratson]

> -----Original Message-----
> From: Danielle Ratson <danieller@xxxxxxxxxx>
> Sent: Tuesday, 9 April 2024 16:34
> To: Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; corbet@xxxxxxx; sdf@xxxxxxxxxx;
> kory.maincent@xxxxxxxxxxx; maxime.chevallier@xxxxxxxxxxx;
> vladimir.oltean@xxxxxxx; przemyslaw.kitszel@xxxxxxxxx;
> ahmed.zaki@xxxxxxxxx; richardcochran@xxxxxxxxx; shayagr@xxxxxxxxxx;
> paul.greenwalt@xxxxxxxxx; jiri@xxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; mlxsw <mlxsw@xxxxxxxxxx>; Petr Machata
> <petrm@xxxxxxxxxx>; Ido Schimmel <idosch@xxxxxxxxxx>
> Subject: RE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
> 
> > -----Original Message-----
> > From: Russell King <linux@xxxxxxxxxxxxxxx>
> > Sent: Monday, 8 April 2024 17:55
> > To: Danielle Ratson <danieller@xxxxxxxxxx>
> > Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx;
> > kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; corbet@xxxxxxx; sdf@xxxxxxxxxx;
> > kory.maincent@xxxxxxxxxxx; maxime.chevallier@xxxxxxxxxxx;
> > vladimir.oltean@xxxxxxx; przemyslaw.kitszel@xxxxxxxxx;
> > ahmed.zaki@xxxxxxxxx; richardcochran@xxxxxxxxx; shayagr@xxxxxxxxxx;
> > paul.greenwalt@xxxxxxxxx; jiri@xxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> > linux- kernel@xxxxxxxxxxxxxxx; mlxsw <mlxsw@xxxxxxxxxx>; Petr Machata
> > <petrm@xxxxxxxxxx>; Ido Schimmel <idosch@xxxxxxxxxx>
> > Subject: Re: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
> > supporting CDB commands
> >
> > On Mon, Apr 08, 2024 at 03:53:37PM +0300, Danielle Ratson wrote:
> > > +/**
> > > + * struct ethtool_cmis_cdb_request - CDB commands request fields as
> > decribed in
> > > + *				the CMIS standard
> > > + * @id: Command ID.
> > > + * @epl_len: EPL memory length.
> > > + * @lpl_len: LPL memory length.
> > > + * @chk_code: Check code for the previous field and the payload.
> > > + * @resv1: Added to match the CMIS standard request continuity.
> > > + * @resv2: Added to match the CMIS standard request continuity.
> > > + * @payload: Payload for the CDB commands.
> > > + */
> > > +struct ethtool_cmis_cdb_request {
> > > +	__be16 id;
> > > +	struct_group(body,
> > > +		u16 epl_len;
> >
> > u16 with a struct that also uses __be16 looks suspicious.
> >
> > > +		u8 lpl_len;
> > > +		u8 chk_code;
> > > +		u8 resv1;
> > > +		u8 resv2;
> > > +		u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
> > > +	);
> >
> > Does it matter if the compiler inserts some padding before this struct group?
> 
> Yes it should matter since I copy this struct into a payload and it should be
> transferred like that to match the CMIS specification, but if I use __be16 for
> both id and epl_len it might resolve the issue isn’t it?
> 
> >
> > > +/**
> > > + * struct ethtool_cmis_cdb_rpl_hdr - CDB commands reply header
> > > +arguments
> > > + * @rpl_len: Reply length.
> > > + * @rpl_chk_code: Reply check code.
> > > + */
> > > +struct ethtool_cmis_cdb_rpl_hdr {
> > > +	u8 rpl_len;
> > > +	u8 rpl_chk_code;
> >
> > Does it matter if the compiler adds some padding here?
> 
> Kind of the same idea, it is matter if the compiler adds padding since the reply
> is read and extracted into this struct like it is written in the CMIS specification.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Hi Russell, 

Please see below pahole's output after changing ethtool_cmis_cdb_request ::epl_len from u16 to __be16.

$ pahole -C 'ethtool_cmis_cdb_request' ./vmlinux
struct ethtool_cmis_cdb_request {
        __be16                     id;                   /*     0     2 */
        union {
                struct {
                        __be16     epl_len;              /*     2     2 */
                        u8         lpl_len;              /*     4     1 */
                        u8         chk_code;             /*     5     1 */
                        u8         resv1;                /*     6     1 */
                        u8         resv2;                /*     7     1 */
                        u8         payload[120];         /*     8   120 */
                };                                       /*     2   126 */
                struct {
                        __be16     epl_len;              /*     2     2 */
                        u8         lpl_len;              /*     4     1 */
                        u8         chk_code;             /*     5     1 */
                        u8         resv1;                /*     6     1 */
                        u8         resv2;                /*     7     1 */
                        u8         payload[120];         /*     8   120 */
                } body;                                  /*     2   126 */
        };                                               /*     2   126 */

        /* size: 128, cachelines: 2, members: 2 */
};

[danieller@dev-r-vrt-155 linux_mlxsw]$ pahole -C 'ethtool_cmis_cdb_rpl' ./vmlinux
struct ethtool_cmis_cdb_rpl {
        struct ethtool_cmis_cdb_rpl_hdr hdr;             /*     0     2 */
        u8                         payload[120];         /*     2   120 */

        /* size: 122, cachelines: 2, members: 2 */
        /* last cacheline: 58 bytes */
};

[danieller@dev-r-vrt-155 linux_mlxsw]$ pahole -C 'ethtool_cmis_cdb_rpl_hdr' ./vmlinux
struct ethtool_cmis_cdb_rpl_hdr {
        u8                         rpl_len;              /*     0     1 */
        u8                         rpl_chk_code;         /*     1     1 */

        /* size: 2, cachelines: 1, members: 2 */
        /* last cacheline: 2 bytes */
};

If a further fix is needed, you are welcome to let me know.

Thanks,
Danielle