Re: [PATCH net-next 1/2] ethtool: add PHY Fast Link Down support

[Heiner Kallweit <hkallweit1@gmail.com>]

On 26.03.2019 10:17, Michal Kubecek wrote:
> 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.
> 
Not to confuse Dave with the discussion:
This is not about whether the patch is wrong or right, but about how
a future architecture based on ethtool-nl could look like.

Like Michael said, based on the current ethtool architecture it's simple:
Driver will choose closest supported value. When reading back the setting
you will get the exact value chosen by the driver.

> Michal
> .
> 
Heiner