> -----Original Message-----
> From: Jakub Kicinski <k...@kernel.org>
> Sent: Monday, October 12, 2020 1:38 AM
> To: Ido Schimmel <ido...@idosch.org>
> Cc: netdev@vger.kernel.org; da...@davemloft.net; Jiri Pirko
> <j...@nvidia.com>; Danielle Ratson <daniel...@nvidia.com>;
> and...@lunn.ch; f.faine...@gmail.com; mkube...@suse.cz; mlxsw
> <ml...@nvidia.com>; Ido Schimmel <ido...@nvidia.com>;
> johan...@sipsolutions.net
> Subject: Re: [PATCH net-next 1/6] ethtool: Extend link modes settings uAPI
> with lanes
>
> On Sat, 10 Oct 2020 18:41:14 +0300 Ido Schimmel wrote:
> > From: Danielle Ratson <daniel...@nvidia.com>
> >
> > Currently, when auto negotiation is on, the user can advertise all the
> > linkmodes which correspond to a specific speed, but does not have a
> > similar selector for the number of lanes. This is significant when a
> > specific speed can be achieved using different number of lanes. For
> > example, 2x50 or 4x25.
> >
> > Add 'ETHTOOL_A_LINKMODES_LANES' attribute and expand 'struct
> > ethtool_link_settings' with lanes field in order to implement a new
> > lanes-selector that will enable the user to advertise a specific
> > number of lanes as well.
> >
> > When auto negotiation is off, lanes parameter can be forced only if
> > the driver supports it. Add a capability bit in 'struct ethtool_ops'
> > that allows ethtool know if the driver can handle the lanes parameter
> > when auto negotiation is off, so if it does not, an error message will
> > be returned when trying to set lanes.
>
> What's the use for this in practical terms? Isn't the lane count basically
> implied by the module that gets plugged in?
The use is to enable the user to decide how to achieve a certain speed.
For example, if he wants to get 100G and the port has 4 lanes, the speed can be
achieved it using both 2 lanes of 50G and 4 lanes of 25G, as a port with 4
lanes width can work in 2 lanes mode with double speed each.
So, by specifying "lanes 2" he will achieve 100G using 2 lanes of 50G.
>
> > +/* Lanes, 1, 2, 4 or 8. */
> > +#define ETHTOOL_LANES_1 1
> > +#define ETHTOOL_LANES_2 2
> > +#define ETHTOOL_LANES_4 4
> > +#define ETHTOOL_LANES_8 8
>
> Not an extremely useful set of defines, not sure Michal would agree.
>
> > +#define ETHTOOL_LANES_UNKNOWN 0
>
> > struct link_mode_info {
> > int speed;
> > + int lanes;
>
> why signed?
I have aligned it to the speed.
>
> > u8 duplex;
> > };
>
> > @@ -274,16 +277,17 @@ const struct nla_policy
> ethnl_linkmodes_set_policy[] = {
> > [ETHTOOL_A_LINKMODES_SPEED] = { .type = NLA_U32 },
> > [ETHTOOL_A_LINKMODES_DUPLEX] = { .type = NLA_U8 },
> > [ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG] = { .type =
> NLA_U8 },
> > + [ETHTOOL_A_LINKMODES_LANES] = { .type = NLA_U32 },
>
> NLA_POLICY_VALIDATE_FN(), not sure why the types for this
> validation_type are limited.. Johannes, just an abundance of caution?
>
> > + } else if (!lsettings->autoneg) {
> > + /* If autoneg is off and lanes parameter is not passed from
> user,
> > + * set the lanes parameter to UNKNOWN.
> > + */
> > + ksettings->lanes = ETHTOOL_LANES_UNKNOWN;
>
> you assume UNKNOWN is zero by doing !lanes in auto_linkmodes - that's
> inconsistent.
I didn't do !lanes... maybe you mean !req_lanes which is different parameter
that specifies if the lanes parameter was passed from user space.
>
> > + }
> > +
> > ret = ethnl_update_bitset(ksettings->link_modes.advertising,
> > __ETHTOOL_LINK_MODE_MASK_NBITS,
> > tb[ETHTOOL_A_LINKMODES_OURS],
> link_mode_names,
