[net-next,1/2] ethtool: add PHY Fast Link Down support
diff mbox series

Message ID 506ae10e-5d93-f64e-a615-c320010a5529@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • ethtool: add support for Fast Link Down as new PHY tunable
Related show

Commit Message

Heiner Kallweit March 24, 2019, 3:02 p.m. UTC
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(+)

Comments

Michal Kubecek March 25, 2019, 5:49 p.m. UTC | #1
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
> 
>
Heiner Kallweit March 25, 2019, 6:10 p.m. UTC | #2
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
>>
>>
>
Andrew Lunn March 26, 2019, 8:24 a.m. UTC | #3
> > +#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
Michal Kubecek March 26, 2019, 9:17 a.m. UTC | #4
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
Heiner Kallweit March 26, 2019, 6:24 p.m. UTC | #5
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
Heiner Kallweit March 26, 2019, 6:28 p.m. UTC | #6
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
Andrew Lunn March 27, 2019, 8:48 a.m. UTC | #7
> 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

Patch
diff mbox series

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;