On Tue, Mar 26, 2019 at 09:24:38AM +0100, Andrew Lunn wrote:
> > > +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0
> > > +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff
> > > +
> > > enum phy_tunable_id {
> > > ETHTOOL_PHY_ID_UNSPEC,
> > > ETHTOOL_PHY_DOWNSHIFT,
> > > + ETHTOOL_PHY_FAST_LINK_DOWN,
> > > /*
> > > * Add your fresh new phy tunable attribute above and remember to update
> > > * phy_tunable_strings[] in net/core/ethtool.c
> >
> > It would be nice to have a short summary around here explaining how is
> > the value interpreted. While it's obvious from the second patch, one
> > shouldn't have to go into driver specific implementation to find out.
> >
> > I also wonder if the range 0-254 ms is sufficient. Would it be possible
> > that there is some other hardware which would support e.g. 300 ms?
>
> The default, as defined by the 802.3 standard, is i think 750ms.
>
> The Marvel PHY also supports 50ms, 20ms and 0ms, if i remember
> correctly.
The reason why I asked about this is that PHY tunables are supposed to
be universal, not specific to a particular driver, and there might be
other hardware supporting the feature with different set of supported
values.
> One problem we have here is discovery. How does the user find out the
> values the driver supports. For a netlink socket API, extended errors
> could be used to pass back a string indicating the supported
> values. For the old ethtool, i think all we have is -EINVAL, which is
> not very helpful.
As supported values are determined by the driver, we would need to pass
extack to ethtool_ops handler - but that is something we will want to do
eventually (ideally, for all ethtool_ops handlers).
AFAICS the implementation in patch 2/2 rounds user supplied value to
closest value supported by hardware so that user doesn't have to guess
which values are supported. But it would still deserve a warning via
netlink extack, IMHO.
Michal
