> -----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
