[PATCH net-next 0/2] ethtool: add support for Fast Link Down as new PHY tunable

[PATCH net-next 0/2] ethtool: add support for Fast Link Down as new PHY tunable

[Heiner Kallweit]

This adds support for Fast Link Down as new PHY tunable.
Fast Link Down reduces the time until a link down event is reported
for 1000BaseT. According to the standard it's 750ms what is too long
for several use cases.

This is the kernel-related series, the ethtool userspace extension
I'd submit once the kernel part has been applied.

Heiner Kallweit (2):
  ethtool: add PHY Fast Link Down support
  net: phy: marvell: add PHY tunable fast link down support for 88E1540

 drivers/net/phy/marvell.c    | 108 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/ethtool.h |   4 ++
 net/core/ethtool.c           |   2 +
 3 files changed, 114 insertions(+)

-- 
2.21.0

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

[Heiner Kallweit]

This adds support for Fast Link Down as new PHY tunable.
Fast Link Down reduces the time until a link down event is reported
for 1000BaseT. According to the standard it's 750ms what is too long
for several use cases.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/uapi/linux/ethtool.h | 4 ++++
 net/core/ethtool.c           | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 3652b239d..2c7136adc 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -252,9 +252,13 @@ struct ethtool_tunable {
 #define DOWNSHIFT_DEV_DEFAULT_COUNT	0xff
 #define DOWNSHIFT_DEV_DISABLE		0
 
+#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
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b1eb32419..387d67eb7 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -136,6 +136,7 @@ static const char
 phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_ID_UNSPEC]     = "Unspec",
 	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
+	[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
 };
 
 static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
@@ -2432,6 +2433,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
 {
 	switch (tuna->id) {
 	case ETHTOOL_PHY_DOWNSHIFT:
+	case ETHTOOL_PHY_FAST_LINK_DOWN:
 		if (tuna->len != sizeof(u8) ||
 		    tuna->type_id != ETHTOOL_TUNABLE_U8)
 			return -EINVAL;
-- 
2.21.0

[PATCH net-next 2/2] net: phy: marvell: add PHY tunable fast link down support for 88E1540

[Heiner Kallweit]

1000BaseT standard requires that a link is reported as down earliest
after 750ms. Several use case however require a much faster detecion
of a broken link. Fast Link Down supports this by intentionally
violating a the standard. This patch exposes the Fast Link Down
feature of 88E1540 and 88E6390. These PHY's can be found as internal
PHY's in several switches: 88E6352, 88E6240, 88E6176, 88E6172,
and 88E6390(X). Fast Link Down and EEE are mutually exclusive.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/marvell.c | 108 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3ccba37bd..65350186d 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -29,6 +29,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <linux/marvell_phy.h>
+#include <linux/bitfield.h>
 #include <linux/of.h>
 
 #include <linux/io.h>
@@ -91,6 +92,14 @@
 #define MII_88E1510_TEMP_SENSOR		0x1b
 #define MII_88E1510_TEMP_SENSOR_MASK	0xff
 
+#define MII_88E1540_COPPER_CTRL3	0x1a
+#define MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_MASK	GENMASK(11, 10)
+#define MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_00MS	0
+#define MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_10MS	1
+#define MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_20MS	2
+#define MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_40MS	3
+#define MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN		BIT(9)
+
 #define MII_88E6390_MISC_TEST		0x1b
 #define MII_88E6390_MISC_TEST_SAMPLE_1S		0
 #define MII_88E6390_MISC_TEST_SAMPLE_10MS	BIT(14)
@@ -1025,6 +1034,101 @@ static int m88e1145_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int m88e1540_get_fld(struct phy_device *phydev, u8 *msecs)
+{
+	int val;
+
+	val = phy_read(phydev, MII_88E1540_COPPER_CTRL3);
+	if (val < 0)
+		return val;
+
+	if (!(val & MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN)) {
+		*msecs = ETHTOOL_PHY_FAST_LINK_DOWN_OFF;
+		return 0;
+	}
+
+	val = FIELD_GET(MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_MASK, val);
+
+	switch (val) {
+	case MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_00MS:
+		*msecs = 0;
+		break;
+	case MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_10MS:
+		*msecs = 10;
+		break;
+	case MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_20MS:
+		*msecs = 20;
+		break;
+	case MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_40MS:
+		*msecs = 40;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int m88e1540_set_fld(struct phy_device *phydev, const u8 *msecs)
+{
+	struct ethtool_eee eee;
+	int val, ret;
+
+	if (*msecs == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)
+		return phy_clear_bits(phydev, MII_88E1540_COPPER_CTRL3,
+				      MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN);
+
+	/* According to the Marvell data sheet EEE must be disabled for
+	 * Fast Link Down detection to work properly
+	 */
+	ret = phy_ethtool_get_eee(phydev, &eee);
+	if (!ret && eee.eee_enabled) {
+		phydev_warn(phydev, "Fast Link Down detection requires EEE to be disabled!\n");
+		return -EBUSY;
+	}
+
+	if (*msecs <= 5)
+		val = MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_00MS;
+	else if (*msecs <= 15)
+		val = MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_10MS;
+	else if (*msecs <= 30)
+		val = MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_20MS;
+	else
+		val = MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_40MS;
+
+	val = FIELD_PREP(MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_MASK, val);
+
+	ret = phy_modify(phydev, MII_88E1540_COPPER_CTRL3,
+			 MII_88E1540_COPPER_CTRL3_LINK_DOWN_DELAY_MASK, val);
+	if (ret)
+		return ret;
+
+	return phy_set_bits(phydev, MII_88E1540_COPPER_CTRL3,
+			    MII_88E1540_COPPER_CTRL3_FAST_LINK_DOWN);
+}
+
+static int m88e1540_get_tunable(struct phy_device *phydev,
+				struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_FAST_LINK_DOWN:
+		return m88e1540_get_fld(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int m88e1540_set_tunable(struct phy_device *phydev,
+				struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_FAST_LINK_DOWN:
+		return m88e1540_set_fld(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 /* The VOD can be out of specification on link up. Poke an
  * undocumented register, in an undocumented page, with a magic value
  * to fix this.
@@ -2247,6 +2351,8 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1545,
@@ -2307,6 +2413,8 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		.get_tunable = m88e1540_get_tunable,
+		.set_tunable = m88e1540_set_tunable,
 	},
 };
 
-- 
2.21.0

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

[Michal Kubecek]

On Sun, Mar 24, 2019 at 04:02:43PM +0100, Heiner Kallweit wrote:
> This adds support for Fast Link Down as new PHY tunable.
> Fast Link Down reduces the time until a link down event is reported
> for 1000BaseT. According to the standard it's 750ms what is too long
> for several use cases.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/uapi/linux/ethtool.h | 4 ++++
>  net/core/ethtool.c           | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 3652b239d..2c7136adc 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -252,9 +252,13 @@ struct ethtool_tunable {
>  #define DOWNSHIFT_DEV_DEFAULT_COUNT	0xff
>  #define DOWNSHIFT_DEV_DISABLE		0
>  
> +#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?

Michal Kubecek

> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index b1eb32419..387d67eb7 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -136,6 +136,7 @@ static const char
>  phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>  	[ETHTOOL_ID_UNSPEC]     = "Unspec",
>  	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
> +	[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
>  };
>  
>  static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> @@ -2432,6 +2433,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
>  {
>  	switch (tuna->id) {
>  	case ETHTOOL_PHY_DOWNSHIFT:
> +	case ETHTOOL_PHY_FAST_LINK_DOWN:
>  		if (tuna->len != sizeof(u8) ||
>  		    tuna->type_id != ETHTOOL_TUNABLE_U8)
>  			return -EINVAL;
> -- 
> 2.21.0
> 
> 

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

[Heiner Kallweit]

On 25.03.2019 18:49, Michal Kubecek wrote:
> On Sun, Mar 24, 2019 at 04:02:43PM +0100, Heiner Kallweit wrote:
>> This adds support for Fast Link Down as new PHY tunable.
>> Fast Link Down reduces the time until a link down event is reported
>> for 1000BaseT. According to the standard it's 750ms what is too long
>> for several use cases.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/uapi/linux/ethtool.h | 4 ++++
>>  net/core/ethtool.c           | 2 ++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index 3652b239d..2c7136adc 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -252,9 +252,13 @@ struct ethtool_tunable {
>>  #define DOWNSHIFT_DEV_DEFAULT_COUNT	0xff
>>  #define DOWNSHIFT_DEV_DISABLE		0
>>  
>> +#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.
> 
OK

> 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 relevant use cases (according to the Marvell spec) require link down
recognition in <50ms. Something >200ms I think wouldn't be considered
as "fast" in general.

And for something completely different, all mails to John are bounced currently:
rejected because 209.85.128.65 is in a black list at 0spam.fusionzero.com Received: from mail-wm1-f65.google.com
I tried from another email address, but same result.

> Michal Kubecek
> 
Heiner

>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index b1eb32419..387d67eb7 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -136,6 +136,7 @@ static const char
>>  phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>>  	[ETHTOOL_ID_UNSPEC]     = "Unspec",
>>  	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
>> +	[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
>>  };
>>  
>>  static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
>> @@ -2432,6 +2433,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
>>  {
>>  	switch (tuna->id) {
>>  	case ETHTOOL_PHY_DOWNSHIFT:
>> +	case ETHTOOL_PHY_FAST_LINK_DOWN:
>>  		if (tuna->len != sizeof(u8) ||
>>  		    tuna->type_id != ETHTOOL_TUNABLE_U8)
>>  			return -EINVAL;
>> -- 
>> 2.21.0
>>
>>
> 

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

[Andrew Lunn]

> > +#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.

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.

	Andrew

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

[Michal Kubecek]

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

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

[Heiner Kallweit]

On 26.03.2019 09:24, 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.
> 
Clause 40. From what I've found this applies to 1000BaseT only.

> The Marvel PHY also supports 50ms, 20ms and 0ms, if i remember
> correctly.
> 
0, 10, 20, 40ms (at least for 88E1540 and 88E6390)

> 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.
> 
> 	Andrew
> .
> 
Heiner

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

[Heiner Kallweit]

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

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

[Andrew Lunn]

> 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.

Hi Heiner

Yes, i read you actual patch later.

Rounding down to the nearest seems sensible. When you write the man
page patch for ethtool, please document this.

     Andrew