Hi Vinicius, On Wed, 4 Sep 2019 at 00:26, Vinicius Costa Gomes <[email protected]> wrote: > > Hi, > > Vladimir Oltean <[email protected]> writes: > > > Right. And while we're at it, there's still the potential > > division-by-zero problem which I still don't know how to solve without > > implementing a full-blown __ethtool_get_link_ksettings parser that > > checks against all the possible outputs it can have under the "no > > carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to > > SPEED_UNKNOWN when there is no PHY connected" for details. > > And there's also a third fix to be made: the netdev_dbg should be made > > to print "speed" instead of "ecmd.base.speed". > > For the ksettings part I am thinking on adding something like this to > ethtool.c. Do you think anything is missing (apart from the > documentation)? > > -> > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 95991e43..d37c80b 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -177,6 +177,9 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned > long *dst, > bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32, > const unsigned long *src); > > +u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings > *settings, > + u32 default_speed); > + > /** > * struct ethtool_ops - optional netdev operations > * @get_drvinfo: Report driver/device information. Should only set the > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 6288e69..80e3db3 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -539,6 +539,18 @@ struct ethtool_link_usettings { > } link_modes; > }; > > +u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings > *settings, > + u32 default_speed) > +{ > + if (settings->base.speed == SPEED_UNKNOWN) > + return default_speed; > + > + if (settings->base.speed == 0) > + return default_speed; > + > + return settings->base.speed; > +} > + > /* Internal kernel helper to query a device ethtool_link_settings. */ > int __ethtool_get_link_ksettings(struct net_device *dev, > struct ethtool_link_ksettings > *link_ksettings)
Looks ok to me, but I have no saying over ethtool API. Actually I don't even know whom to ask - the output of ./scripts/get_maintainer.pl net/core/ethtool.c is a bit overwhelming. To avoid conflicts, there needs to be somebody out of us who takes Eric's simplification, with Gustavo's Reported-by tag, and the 2 ethtool & taprio patches to avoid division by zero, and the printing fix, and maybe do the same in cbs. Will you be the one? Should I? Thanks, -Vladimir
