lkml.org 
[lkml]   [2024]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for supporting CDB commands
Date
> -----Original Message-----
> From: Danielle Ratson <danieller@nvidia.com>
> Sent: Tuesday, 9 April 2024 16:34
> To: Russell King <linux@armlinux.org.uk>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net; sdf@google.com;
> kory.maincent@bootlin.com; maxime.chevallier@bootlin.com;
> vladimir.oltean@nxp.com; przemyslaw.kitszel@intel.com;
> ahmed.zaki@intel.com; richardcochran@gmail.com; shayagr@amazon.com;
> paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata
> <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> Subject: RE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
>
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Monday, 8 April 2024 17:55
> > To: Danielle Ratson <danieller@nvidia.com>
> > Cc: netdev@vger.kernel.org; davem@davemloft.net;
> edumazet@google.com;
> > kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net; sdf@google.com;
> > kory.maincent@bootlin.com; maxime.chevallier@bootlin.com;
> > vladimir.oltean@nxp.com; przemyslaw.kitszel@intel.com;
> > ahmed.zaki@intel.com; richardcochran@gmail.com; shayagr@amazon.com;
> > paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org;
> > linux- kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata
> > <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> > 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
\
 
 \ /
  Last update: 2024-05-27 16:40    [W:0.137 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site