> -----Original Message-----
> From: Andrew Lunn <and...@lunn.ch>
> Sent: Sunday, April 4, 2021 7:33 PM
> To: Danielle Ratson <daniel...@nvidia.com>
> Cc: netdev@vger.kernel.org; da...@davemloft.net; k...@kernel.org;
> eric.duma...@gmail.com; mkube...@suse.cz;
> f.faine...@gmail.com; acard...@redhat.com; irussk...@marvell.com;
> gust...@embeddedor.com; magnus.karls...@intel.com;
> ec...@solarflare.com; Ido Schimmel <ido...@nvidia.com>; Jiri Pirko
> <j...@nvidia.com>; mlxsw <ml...@nvidia.com>
> Subject: Re: [PATCH net v2 1/2] ethtool: Add link_mode parameter capability
> bit to ethtool_ops
>
> > @@ -436,12 +436,16 @@ int __ethtool_get_link_ksettings(struct
> > net_device *dev,
> >
> > memset(link_ksettings, 0, sizeof(*link_ksettings));
> >
> > - link_ksettings->link_mode = -1;
> > err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
> > if (err)
> > return err;
> >
> > - if (link_ksettings->link_mode != -1) {
> > + if (dev->ethtool_ops->cap_link_mode_supported &&
> > + link_ksettings->link_mode != -1) {
>
> Is this -1 behaviour documented anywhere? It seems like you just changed its
> meaning. It used to mean, this field has not been set,
> ignore it. Adding the cap_link_mode_supported it now means, we have field has
> been set, but we have no idea what link mode is
> being used.
> So you should probably add something to the documentation of struct
> ethtool_link_ksettings.
>
> I wonder if we should actually add ETHTOOL_LINK_MODE_UNKNOWN to enum
> ethtool_link_mode_bit_indices?
>
> > + if (WARN_ON_ONCE(link_ksettings->link_mode >=
> > + __ETHTOOL_LINK_MODE_MASK_NBITS))
> > + return -EINVAL;
> > +
> > link_info = &link_mode_params[link_ksettings->link_mode];
> > link_ksettings->base.speed = link_info->speed;
> > link_ksettings->lanes = link_info->lanes;
>
> If dev->ethtool_ops->cap_link_mode_supported && link_ksettings->link_mode ==
> -1 should you be setting speed to
> SPEED_UNKNOWN, and lanes to LANE_UNKNOWN? Or is that already the default?
>
> But over all, this API between the core and the driver seems messy. Why not
> just add a helper in common.c which translates link
> mode to speed/duplex/lanes and call it in the driver. Then you don't need
> this capability flags, which i doubt any other driver will ever
> use. And you don't need to worry about drivers returning random values. As
> far as i can see, the link_mode returned by the driver is
> not used for anything other than for this translation. So i don't see a need
> for it outside of the driver. Or maybe i'm missing
> something?
>
> Andrew
Hi Andrew,
Please see my patch here:
https://github.com/daniellerts/linux_mlxsw/commit/72ca614951418843aa87323630c354691d9e50d4
Since a lack of time until the window closes, please see if that is what you
meant and you are ok with it, before I am sending another version.
Thanks,
Danielle
