[PATCH net-next 0/3] net: phy: add and use phy_check_downshift

[PATCH net-next 0/3] net: phy: add and use phy_check_downshift

[Heiner Kallweit]

So far PHY drivers have to check whether a downshift occurred to be
able to notify the user. To make life of drivers authors a little bit
easier move the downshift notification to phylib. phy_check_downshift()
compares the highest mutually advertised speed with the actual value
of phydev->speed (typically read by the PHY driver from a
vendor-specific register) to detect a downshift.

Heiner Kallweit (3):
  net: phy: add and use phy_check_downshift
  net: phy: marvell: remove downshift warning now that phylib takes care
  net: phy: aquantia: remove downshift warning now that phylib takes
    care

 drivers/net/phy/aquantia_main.c | 25 +------------------------
 drivers/net/phy/marvell.c       | 24 ------------------------
 drivers/net/phy/phy-core.c      | 33 +++++++++++++++++++++++++++++++++
 drivers/net/phy/phy.c           |  1 +
 include/linux/phy.h             |  1 +
 5 files changed, 36 insertions(+), 48 deletions(-)

-- 
2.25.1

[PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Heiner Kallweit]

So far PHY drivers have to check whether a downshift occurred to be
able to notify the user. To make life of drivers authors a little bit
easier move the downshift notification to phylib. phy_check_downshift()
compares the highest mutually advertised speed with the actual value
of phydev->speed (typically read by the PHY driver from a
vendor-specific register) to detect a downshift.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-core.c | 33 +++++++++++++++++++++++++++++++++
 drivers/net/phy/phy.c      |  1 +
 include/linux/phy.h        |  1 +
 3 files changed, 35 insertions(+)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index e083e7a76..8e861be73 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -329,6 +329,39 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
 
+/**
+ * phy_check_downshift - check whether downshift occurred
+ * @phydev: The phy_device struct
+ *
+ * Check whether a downshift to a lower speed occurred. If this should be the
+ * case warn the user.
+ */
+bool phy_check_downshift(struct phy_device *phydev)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
+	int i, speed = SPEED_UNKNOWN;
+
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		return false;
+
+	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
+
+	for (i = 0; i < ARRAY_SIZE(settings); i++)
+		if (test_bit(settings[i].bit, common)) {
+			speed = settings[i].speed;
+			break;
+		}
+
+	if (phydev->speed == speed)
+		return false;
+
+	phydev_warn(phydev, "Downshift occurred from negotiated speed %s to actual speed %s, check cabling!\n",
+		    phy_speed_to_str(speed), phy_speed_to_str(phydev->speed));
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(phy_check_downshift);
+
 static int phy_resolve_min_speed(struct phy_device *phydev, bool fdx_only)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d71212a41..067ff5fec 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -507,6 +507,7 @@ static int phy_check_link_status(struct phy_device *phydev)
 		return err;
 
 	if (phydev->link && phydev->state != PHY_RUNNING) {
+		phy_check_downshift(phydev);
 		phydev->state = PHY_RUNNING;
 		phy_link_up(phydev);
 	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index cb5a2182b..4962766b2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -698,6 +698,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
 
 void phy_resolve_aneg_pause(struct phy_device *phydev);
 void phy_resolve_aneg_linkmode(struct phy_device *phydev);
+bool phy_check_downshift(struct phy_device *phydev);
 
 /**
  * phy_read - Convenience function for reading a given PHY register
-- 
2.25.1

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Russell King - ARM Linux admin]

On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
> So far PHY drivers have to check whether a downshift occurred to be
> able to notify the user. To make life of drivers authors a little bit
> easier move the downshift notification to phylib. phy_check_downshift()
> compares the highest mutually advertised speed with the actual value
> of phydev->speed (typically read by the PHY driver from a
> vendor-specific register) to detect a downshift.

My personal position on this is that reporting a downshift will be
sporadic at best, even when the link has negotiated slower.

The reason for this is that either end can decide to downshift.  If
the remote partner downshifts, then the local side has no idea that
a downshift occurred, and can't report that the link was downshifted.

So, is it actually useful to report these events?

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy-core.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/net/phy/phy.c      |  1 +
>  include/linux/phy.h        |  1 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index e083e7a76..8e861be73 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -329,6 +329,39 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
>  
> +/**
> + * phy_check_downshift - check whether downshift occurred
> + * @phydev: The phy_device struct
> + *
> + * Check whether a downshift to a lower speed occurred. If this should be the
> + * case warn the user.
> + */
> +bool phy_check_downshift(struct phy_device *phydev)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> +	int i, speed = SPEED_UNKNOWN;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return false;
> +
> +	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
> +
> +	for (i = 0; i < ARRAY_SIZE(settings); i++)
> +		if (test_bit(settings[i].bit, common)) {
> +			speed = settings[i].speed;
> +			break;
> +		}
> +
> +	if (phydev->speed == speed)
> +		return false;
> +
> +	phydev_warn(phydev, "Downshift occurred from negotiated speed %s to actual speed %s, check cabling!\n",
> +		    phy_speed_to_str(speed), phy_speed_to_str(phydev->speed));
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(phy_check_downshift);
> +
>  static int phy_resolve_min_speed(struct phy_device *phydev, bool fdx_only)
>  {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d71212a41..067ff5fec 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -507,6 +507,7 @@ static int phy_check_link_status(struct phy_device *phydev)
>  		return err;
>  
>  	if (phydev->link && phydev->state != PHY_RUNNING) {
> +		phy_check_downshift(phydev);
>  		phydev->state = PHY_RUNNING;
>  		phy_link_up(phydev);
>  	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index cb5a2182b..4962766b2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -698,6 +698,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
>  
>  void phy_resolve_aneg_pause(struct phy_device *phydev);
>  void phy_resolve_aneg_linkmode(struct phy_device *phydev);
> +bool phy_check_downshift(struct phy_device *phydev);
>  
>  /**
>   * phy_read - Convenience function for reading a given PHY register
> -- 
> 2.25.1
> 
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Heiner Kallweit]

On 19.03.2020 00:21, Russell King - ARM Linux admin wrote:
> On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
>> So far PHY drivers have to check whether a downshift occurred to be
>> able to notify the user. To make life of drivers authors a little bit
>> easier move the downshift notification to phylib. phy_check_downshift()
>> compares the highest mutually advertised speed with the actual value
>> of phydev->speed (typically read by the PHY driver from a
>> vendor-specific register) to detect a downshift.
> 
> My personal position on this is that reporting a downshift will be
> sporadic at best, even when the link has negotiated slower.
> 
> The reason for this is that either end can decide to downshift.  If
> the remote partner downshifts, then the local side has no idea that
> a downshift occurred, and can't report that the link was downshifted.
> 
Right, this warning can't cover the case that remote link partner
downshifts. In this case however ethtool et al should show the reduced
link partner advertisement, and therefore provide a hint why speed
is slow.

> So, is it actually useful to report these events?
> 
To provide an example: A user recently complained that r8169 driver
makes problems on his system:
- it takes long time until link comes up
- link is slow
With iperf he then found out that displayed speed is 1Gbps but actual
link speed is 100Mbps. In the end he found that one pin of his network
port was corroded, therefore the downshift.

The phase of blaming the driver could have been skipped if he would
have seen a downshift warning from the very beginning.

>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy-core.c | 33 +++++++++++++++++++++++++++++++++
>>  drivers/net/phy/phy.c      |  1 +
>>  include/linux/phy.h        |  1 +
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index e083e7a76..8e861be73 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -329,6 +329,39 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
>>  
>> +/**
>> + * phy_check_downshift - check whether downshift occurred
>> + * @phydev: The phy_device struct
>> + *
>> + * Check whether a downshift to a lower speed occurred. If this should be the
>> + * case warn the user.
>> + */
>> +bool phy_check_downshift(struct phy_device *phydev)
>> +{
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>> +	int i, speed = SPEED_UNKNOWN;
>> +
>> +	if (phydev->autoneg == AUTONEG_DISABLE)
>> +		return false;
>> +
>> +	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(settings); i++)
>> +		if (test_bit(settings[i].bit, common)) {
>> +			speed = settings[i].speed;
>> +			break;
>> +		}
>> +
>> +	if (phydev->speed == speed)
>> +		return false;
>> +
>> +	phydev_warn(phydev, "Downshift occurred from negotiated speed %s to actual speed %s, check cabling!\n",
>> +		    phy_speed_to_str(speed), phy_speed_to_str(phydev->speed));
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL_GPL(phy_check_downshift);
>> +
>>  static int phy_resolve_min_speed(struct phy_device *phydev, bool fdx_only)
>>  {
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index d71212a41..067ff5fec 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -507,6 +507,7 @@ static int phy_check_link_status(struct phy_device *phydev)
>>  		return err;
>>  
>>  	if (phydev->link && phydev->state != PHY_RUNNING) {
>> +		phy_check_downshift(phydev);
>>  		phydev->state = PHY_RUNNING;
>>  		phy_link_up(phydev);
>>  	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index cb5a2182b..4962766b2 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -698,6 +698,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
>>  
>>  void phy_resolve_aneg_pause(struct phy_device *phydev);
>>  void phy_resolve_aneg_linkmode(struct phy_device *phydev);
>> +bool phy_check_downshift(struct phy_device *phydev);
>>  
>>  /**
>>   * phy_read - Convenience function for reading a given PHY register
>> -- 
>> 2.25.1
>>
>>
>>
> 

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Russell King - ARM Linux admin]

On Thu, Mar 19, 2020 at 08:30:58AM +0100, Heiner Kallweit wrote:
> On 19.03.2020 00:21, Russell King - ARM Linux admin wrote:
> > On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
> >> So far PHY drivers have to check whether a downshift occurred to be
> >> able to notify the user. To make life of drivers authors a little bit
> >> easier move the downshift notification to phylib. phy_check_downshift()
> >> compares the highest mutually advertised speed with the actual value
> >> of phydev->speed (typically read by the PHY driver from a
> >> vendor-specific register) to detect a downshift.
> > 
> > My personal position on this is that reporting a downshift will be
> > sporadic at best, even when the link has negotiated slower.
> > 
> > The reason for this is that either end can decide to downshift.  If
> > the remote partner downshifts, then the local side has no idea that
> > a downshift occurred, and can't report that the link was downshifted.
> > 
> Right, this warning can't cover the case that remote link partner
> downshifts. In this case however ethtool et al should show the reduced
> link partner advertisement, and therefore provide a hint why speed
> is slow.
> 
> > So, is it actually useful to report these events?
> > 
> To provide an example: A user recently complained that r8169 driver
> makes problems on his system:
> - it takes long time until link comes up
> - link is slow
> With iperf he then found out that displayed speed is 1Gbps but actual
> link speed is 100Mbps. In the end he found that one pin of his network
> port was corroded, therefore the downshift.
> 
> The phase of blaming the driver could have been skipped if he would
> have seen a downshift warning from the very beginning.

This sounds like a good theory to justify it, but it suffers from one
major flaw.

There was indeed a bug - the driver was reporting 1Gbps, whereas the
link was not operating at that speed, but at 100Mbps.  Had that bug
not existed, the kernel would've reported 100Mbps as the speed, and
then your justification in the first paragraph applies - the link
speed is slower than expected.

With that bug in place, this patch does nothing; you're using the same
algorithm to calculate what the speed should be and comparing it with
the same algorithm result reported from phy_resolve_aneg_linkmode().

So, the problem is going to remain.

The only time that this helps is if PHY drivers implement reading a
vendor register to report the actual link speed, and the PHY specific
driver is used.

Also, falling back to the generic PHY driver is going to result in the
same hard-to-debug problem that you refer to above.

So, do we need to print a big fat warning that the generic driver is
being used, and recommend the user uses the tailored driver instead?

There's lots of issues to consider here; it is not as simple as has
been suggested, and I'm not sure that this patch adds particularly
high value by really solving the problem.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Andrew Lunn]

> The only time that this helps is if PHY drivers implement reading a
> vendor register to report the actual link speed, and the PHY specific
> driver is used.

So maybe we either need to implement this reading of the vendor
register as a driver op, or we have a flag indicating the driver is
returning the real speed, not the negotiated speed?

	   Andrew

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Russell King - ARM Linux admin]

On Thu, Mar 19, 2020 at 02:04:29PM +0100, Andrew Lunn wrote:
> > The only time that this helps is if PHY drivers implement reading a
> > vendor register to report the actual link speed, and the PHY specific
> > driver is used.
> 
> So maybe we either need to implement this reading of the vendor
> register as a driver op, or we have a flag indicating the driver is
> returning the real speed, not the negotiated speed?

I'm not sure it's necessary to have another driver op.  How about
this for an idea:

- add a flag to struct phy_device which indicates the status of
  downshift.
- on link-up, check the flag and report whether a downshift occurred,
  printing whether a downshift occurred in phy_print_status() and
  similar places.  (Yes, I know that there are some network drivers
  that don't use phy_print_status().)

The downshift flag could be made tristate - "unknown", "not downshifted"
and "downshifted" - which would enable phy_print_status() to indicate
whether there is downshift supported (and hence whether we need to pay
more attention to what is going on when there is a slow-link report.)

Something like:

For no downshift:
	Link is Up - 1Gbps/Full - flow control off
For downshift:
	Link is Up - 100Mbps/Full (downshifted) - flow control off
For unknown:
	Link is Up - 1Gbps/Full (unknown downshift) - flow control off

which has the effect of being immediately obvious if the driver lacks
support.

We may wish to consider PHYs which support no downshift ability as
well, which should probably set the status to "not downshifted" or
maybe an "unsupported" state.

This way, if we fall back to the generic PHY driver, we'd get the
"unknown" state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Heiner Kallweit]

On 19.03.2020 14:58, Russell King - ARM Linux admin wrote:
> On Thu, Mar 19, 2020 at 02:04:29PM +0100, Andrew Lunn wrote:
>>> The only time that this helps is if PHY drivers implement reading a
>>> vendor register to report the actual link speed, and the PHY specific
>>> driver is used.
>>
>> So maybe we either need to implement this reading of the vendor
>> register as a driver op, or we have a flag indicating the driver is
>> returning the real speed, not the negotiated speed?
> 
> I'm not sure it's necessary to have another driver op.  How about
> this for an idea:
> 
> - add a flag to struct phy_device which indicates the status of
>   downshift.
> - on link-up, check the flag and report whether a downshift occurred,
>   printing whether a downshift occurred in phy_print_status() and
>   similar places.  (Yes, I know that there are some network drivers
>   that don't use phy_print_status().)
> 
> The downshift flag could be made tristate - "unknown", "not downshifted"
> and "downshifted" - which would enable phy_print_status() to indicate
> whether there is downshift supported (and hence whether we need to pay
> more attention to what is going on when there is a slow-link report.)
> 
> Something like:
> 
> For no downshift:
> 	Link is Up - 1Gbps/Full - flow control off
> For downshift:
> 	Link is Up - 100Mbps/Full (downshifted) - flow control off
> For unknown:
> 	Link is Up - 1Gbps/Full (unknown downshift) - flow control off
> 
> which has the effect of being immediately obvious if the driver lacks
> support.
> 
> We may wish to consider PHYs which support no downshift ability as
> well, which should probably set the status to "not downshifted" or
> maybe an "unsupported" state.
> 
> This way, if we fall back to the generic PHY driver, we'd get the
> "unknown" state.
> 

I'd like to split the topics. First we have downshift detection,
then we have downshift reporting/warning.

*Downshift detection*
Prerequisite of course is that the PHY supports reading the actual,
possibly downshifted link speed (typically from a vendor-specific
register). Then the PHY driver has to set phydev->speed to the
actual link speed in the read_status() implementation.

For the actual downshift detection we have two options:
1. PHY provides info about a downshift event in a vendor-specific
   register or as an interrupt source.
2. The generic method, compare actual link speed with the highest
   mutually advertised speed.
So far I don't see a benefit of option 1. The generic method is
easier and reduces complexity in drivers.

The genphy driver is a fallback, and in addition may be intentionally
used for PHY's that have no specific features. A PHY with additional
features in general may or may not work properly with the genphy
driver. Some RTL8168-internal PHY's fail miserably with the genphy
driver. I just had a longer discussion about it caused by the fact
that on some distributions r8169.ko is in initramfs but realtek.ko
is not.
On a side note: Seems that so far the kernel doesn't provide an
option to express a hard module dependency that is not a code
dependency.

*Downshift reporting/warning*
In most cases downshift is caused by some problem with the cabling.
Users like the typical Ubuntu user in most cases are not familiar
with the concept of PHY downshift and what causes a downshift.
Therefore it's not sufficient to just report a downshift, we have
to provide the user with a hint what to do.
Adding the "downshifted" info to phy_print_status() is a good idea,
however I'd see it as an optional addition to the mentioned hint
to the user what to do.
The info "unknown downshift" IMO would just cause confusion. If we
have nothing to say, then why say something. Also users may interpret
"unknown" as "there's something wrong".

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Florian Fainelli]

Le 2020-03-19 à 09:36, Heiner Kallweit a écrit :
> *Downshift reporting/warning*
> In most cases downshift is caused by some problem with the cabling.
> Users like the typical Ubuntu user in most cases are not familiar
> with the concept of PHY downshift and what causes a downshift.
> Therefore it's not sufficient to just report a downshift, we have
> to provide the user with a hint what to do.
> Adding the "downshifted" info to phy_print_status() is a good idea,
> however I'd see it as an optional addition to the mentioned hint
> to the user what to do.
> The info "unknown downshift" IMO would just cause confusion. If we
> have nothing to say, then why say something. Also users may interpret
> "unknown" as "there's something wrong".

Ideally we would also have support for cable testing as well which would
allow users to know what to do next to figure out why their link speed
is not what they had hoped for. That does require a specific PHY driver
though as there is no standard way to obtain that information.

FWIW, a bunch of drivers like tg3, e1000e, igc and possibly others do
report when downshifting occurs, of course not all of those in that list
use the PHY library.
-- 
Florian

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Heiner Kallweit]

On 19.03.2020 17:55, Florian Fainelli wrote:
> Le 2020-03-19 à 09:36, Heiner Kallweit a écrit :
>> *Downshift reporting/warning*
>> In most cases downshift is caused by some problem with the cabling.
>> Users like the typical Ubuntu user in most cases are not familiar
>> with the concept of PHY downshift and what causes a downshift.
>> Therefore it's not sufficient to just report a downshift, we have
>> to provide the user with a hint what to do.
>> Adding the "downshifted" info to phy_print_status() is a good idea,
>> however I'd see it as an optional addition to the mentioned hint
>> to the user what to do.
>> The info "unknown downshift" IMO would just cause confusion. If we
>> have nothing to say, then why say something. Also users may interpret
>> "unknown" as "there's something wrong".
> 
> Ideally we would also have support for cable testing as well which would
> allow users to know what to do next to figure out why their link speed
> is not what they had hoped for. That does require a specific PHY driver
> though as there is no standard way to obtain that information.
> 
> FWIW, a bunch of drivers like tg3, e1000e, igc and possibly others do
> report when downshifting occurs, of course not all of those in that list
> use the PHY library.
> 
Yes, a direct link to cable testing would be nice. With the new
netlink-based ethtool support for cable testing comes closer.
AFAIK Andrew is evaluating what an API could look like.

However for the time being I think a downshift warning in phylib would
be helpful to explain to the user: Check your damned cabling instead
of spamming support forums with blaming the network driver or any other
piece of software or hardware.

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Russell King - ARM Linux admin]

On Thu, Mar 19, 2020 at 05:36:30PM +0100, Heiner Kallweit wrote:
> On 19.03.2020 14:58, Russell King - ARM Linux admin wrote:
> > On Thu, Mar 19, 2020 at 02:04:29PM +0100, Andrew Lunn wrote:
> >>> The only time that this helps is if PHY drivers implement reading a
> >>> vendor register to report the actual link speed, and the PHY specific
> >>> driver is used.
> >>
> >> So maybe we either need to implement this reading of the vendor
> >> register as a driver op, or we have a flag indicating the driver is
> >> returning the real speed, not the negotiated speed?
> > 
> > I'm not sure it's necessary to have another driver op.  How about
> > this for an idea:
> > 
> > - add a flag to struct phy_device which indicates the status of
> >   downshift.
> > - on link-up, check the flag and report whether a downshift occurred,
> >   printing whether a downshift occurred in phy_print_status() and
> >   similar places.  (Yes, I know that there are some network drivers
> >   that don't use phy_print_status().)
> > 
> > The downshift flag could be made tristate - "unknown", "not downshifted"
> > and "downshifted" - which would enable phy_print_status() to indicate
> > whether there is downshift supported (and hence whether we need to pay
> > more attention to what is going on when there is a slow-link report.)
> > 
> > Something like:
> > 
> > For no downshift:
> > 	Link is Up - 1Gbps/Full - flow control off
> > For downshift:
> > 	Link is Up - 100Mbps/Full (downshifted) - flow control off
> > For unknown:
> > 	Link is Up - 1Gbps/Full (unknown downshift) - flow control off
> > 
> > which has the effect of being immediately obvious if the driver lacks
> > support.
> > 
> > We may wish to consider PHYs which support no downshift ability as
> > well, which should probably set the status to "not downshifted" or
> > maybe an "unsupported" state.
> > 
> > This way, if we fall back to the generic PHY driver, we'd get the
> > "unknown" state.
> > 
> 
> I'd like to split the topics. First we have downshift detection,
> then we have downshift reporting/warning.
> 
> *Downshift detection*
> Prerequisite of course is that the PHY supports reading the actual,
> possibly downshifted link speed (typically from a vendor-specific
> register). Then the PHY driver has to set phydev->speed to the
> actual link speed in the read_status() implementation.
> 
> For the actual downshift detection we have two options:
> 1. PHY provides info about a downshift event in a vendor-specific
>    register or as an interrupt source.
> 2. The generic method, compare actual link speed with the highest
>    mutually advertised speed.
> So far I don't see a benefit of option 1. The generic method is
> easier and reduces complexity in drivers.
> 
> The genphy driver is a fallback, and in addition may be intentionally
> used for PHY's that have no specific features. A PHY with additional
> features in general may or may not work properly with the genphy
> driver. Some RTL8168-internal PHY's fail miserably with the genphy
> driver. I just had a longer discussion about it caused by the fact
> that on some distributions r8169.ko is in initramfs but realtek.ko
> is not.
> On a side note: Seems that so far the kernel doesn't provide an
> option to express a hard module dependency that is not a code
> dependency.

So how do we address the "fallback to genphy driver" problem for PHYs
that do mostly work with genphy?  It is very easy to do, for example
by omitting the PHY specific driver from the kernel configuration.
Since the system continues to work in these cases, it may go unnoticed.

> *Downshift reporting/warning*
> In most cases downshift is caused by some problem with the cabling.
> Users like the typical Ubuntu user in most cases are not familiar
> with the concept of PHY downshift and what causes a downshift.
> Therefore it's not sufficient to just report a downshift, we have
> to provide the user with a hint what to do.
> Adding the "downshifted" info to phy_print_status() is a good idea,
> however I'd see it as an optional addition to the mentioned hint
> to the user what to do.
> The info "unknown downshift" IMO would just cause confusion. If we
> have nothing to say, then why say something. Also users may interpret
> "unknown" as "there's something wrong".

So you think reporting not-downshifted and no downshift capability
implemented by the driver should appear to be identical?

You claimed as part of the patch description that a downshifted link
was difficult to diagnose; it seems you aren't actually solving that
problem - and in that case I would suggest that you should not be
mentioning it in the commit log.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Heiner Kallweit]

On 19.03.2020 18:08, Russell King - ARM Linux admin wrote:
> On Thu, Mar 19, 2020 at 05:36:30PM +0100, Heiner Kallweit wrote:
>> On 19.03.2020 14:58, Russell King - ARM Linux admin wrote:
>>> On Thu, Mar 19, 2020 at 02:04:29PM +0100, Andrew Lunn wrote:
>>>>> The only time that this helps is if PHY drivers implement reading a
>>>>> vendor register to report the actual link speed, and the PHY specific
>>>>> driver is used.
>>>>
>>>> So maybe we either need to implement this reading of the vendor
>>>> register as a driver op, or we have a flag indicating the driver is
>>>> returning the real speed, not the negotiated speed?
>>>
>>> I'm not sure it's necessary to have another driver op.  How about
>>> this for an idea:
>>>
>>> - add a flag to struct phy_device which indicates the status of
>>>   downshift.
>>> - on link-up, check the flag and report whether a downshift occurred,
>>>   printing whether a downshift occurred in phy_print_status() and
>>>   similar places.  (Yes, I know that there are some network drivers
>>>   that don't use phy_print_status().)
>>>
>>> The downshift flag could be made tristate - "unknown", "not downshifted"
>>> and "downshifted" - which would enable phy_print_status() to indicate
>>> whether there is downshift supported (and hence whether we need to pay
>>> more attention to what is going on when there is a slow-link report.)
>>>
>>> Something like:
>>>
>>> For no downshift:
>>> 	Link is Up - 1Gbps/Full - flow control off
>>> For downshift:
>>> 	Link is Up - 100Mbps/Full (downshifted) - flow control off
>>> For unknown:
>>> 	Link is Up - 1Gbps/Full (unknown downshift) - flow control off
>>>
>>> which has the effect of being immediately obvious if the driver lacks
>>> support.
>>>
>>> We may wish to consider PHYs which support no downshift ability as
>>> well, which should probably set the status to "not downshifted" or
>>> maybe an "unsupported" state.
>>>
>>> This way, if we fall back to the generic PHY driver, we'd get the
>>> "unknown" state.
>>>
>>
>> I'd like to split the topics. First we have downshift detection,
>> then we have downshift reporting/warning.
>>
>> *Downshift detection*
>> Prerequisite of course is that the PHY supports reading the actual,
>> possibly downshifted link speed (typically from a vendor-specific
>> register). Then the PHY driver has to set phydev->speed to the
>> actual link speed in the read_status() implementation.
>>
>> For the actual downshift detection we have two options:
>> 1. PHY provides info about a downshift event in a vendor-specific
>>    register or as an interrupt source.
>> 2. The generic method, compare actual link speed with the highest
>>    mutually advertised speed.
>> So far I don't see a benefit of option 1. The generic method is
>> easier and reduces complexity in drivers.
>>
>> The genphy driver is a fallback, and in addition may be intentionally
>> used for PHY's that have no specific features. A PHY with additional
>> features in general may or may not work properly with the genphy
>> driver. Some RTL8168-internal PHY's fail miserably with the genphy
>> driver. I just had a longer discussion about it caused by the fact
>> that on some distributions r8169.ko is in initramfs but realtek.ko
>> is not.
>> On a side note: Seems that so far the kernel doesn't provide an
>> option to express a hard module dependency that is not a code
>> dependency.
> 
> So how do we address the "fallback to genphy driver" problem for PHYs
> that do mostly work with genphy?  It is very easy to do, for example
> by omitting the PHY specific driver from the kernel configuration.
> Since the system continues to work in these cases, it may go unnoticed.
> 

A blacklist of PHY ID's known to have hard or soft issues when operated
with the genphy driver could break existing systems. But maybe we could
at least print a warning that genphy on the respective PHY ID is an
emergency fallback only and there may be functional restrictions.

However such a blacklist may be useful for PHY's that are known to not
work properly with genphy. RTL8211B is an example. On this PHY the
MMD registers are used for different proprietary purposes.
Writing to these registers in genphy_config_eee_advert() messes
up the PHY and makes it non-functional.

>> *Downshift reporting/warning*
>> In most cases downshift is caused by some problem with the cabling.
>> Users like the typical Ubuntu user in most cases are not familiar
>> with the concept of PHY downshift and what causes a downshift.
>> Therefore it's not sufficient to just report a downshift, we have
>> to provide the user with a hint what to do.
>> Adding the "downshifted" info to phy_print_status() is a good idea,
>> however I'd see it as an optional addition to the mentioned hint
>> to the user what to do.
>> The info "unknown downshift" IMO would just cause confusion. If we
>> have nothing to say, then why say something. Also users may interpret
>> "unknown" as "there's something wrong".
> 
> So you think reporting not-downshifted and no downshift capability
> implemented by the driver should appear to be identical?
> 
> You claimed as part of the patch description that a downshifted link
> was difficult to diagnose; it seems you aren't actually solving that
> problem - and in that case I would suggest that you should not be
> mentioning it in the commit log.
> 

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Andrew Lunn]

On Wed, Mar 18, 2020 at 11:21:59PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
> > So far PHY drivers have to check whether a downshift occurred to be
> > able to notify the user. To make life of drivers authors a little bit
> > easier move the downshift notification to phylib. phy_check_downshift()
> > compares the highest mutually advertised speed with the actual value
> > of phydev->speed (typically read by the PHY driver from a
> > vendor-specific register) to detect a downshift.
> 
> My personal position on this is that reporting a downshift will be
> sporadic at best, even when the link has negotiated slower.
> 
> The reason for this is that either end can decide to downshift.  If
> the remote partner downshifts, then the local side has no idea that
> a downshift occurred, and can't report that the link was downshifted.
> 
> So, is it actually useful to report these events?

It is better than nothing.

We get one end reporting a downshift, and the peer reporting a slower
than expected negotiated link speed. Combined that gives a good clue.

I also want to start merging cable diagnostics soon. There is enough
of netlink ethtool in place to allow this.

And the same netlink ethool will allow us to extend the API and report
downshift in the ethtool output, not just a kernel message.

	  Andrew

Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

[Andrew Lunn]

On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
> So far PHY drivers have to check whether a downshift occurred to be
> able to notify the user. To make life of drivers authors a little bit
> easier move the downshift notification to phylib. phy_check_downshift()
> compares the highest mutually advertised speed with the actual value
> of phydev->speed (typically read by the PHY driver from a
> vendor-specific register) to detect a downshift.
>  
> +/**
> + * phy_check_downshift - check whether downshift occurred
> + * @phydev: The phy_device struct
> + *
> + * Check whether a downshift to a lower speed occurred. If this should be the
> + * case warn the user.
> + */

Hi Heiner

Might be worth documenting here that phydev->speed needs to contain
the real speed, which typically requires a vendor-specific register.

    Andrew

[PATCH net-next 2/3] net: phy: marvell: remove downshift warning now that phylib takes care

[Heiner Kallweit]

Now that phylib notifies the user of a downshift we can remove
this functionality from the driver.

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

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 9a8badafe..4714ca0e0 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -867,21 +867,6 @@ static int m88e1011_set_tunable(struct phy_device *phydev,
 	}
 }
 
-static void m88e1011_link_change_notify(struct phy_device *phydev)
-{
-	int status;
-
-	if (phydev->state != PHY_RUNNING)
-		return;
-
-	/* we may be on fiber page currently */
-	status = phy_read_paged(phydev, MII_MARVELL_COPPER_PAGE,
-				MII_M1011_PHY_SSR);
-
-	if (status > 0 && status & MII_M1011_PHY_SSR_DOWNSHIFT)
-		phydev_warn(phydev, "Downshift occurred! Cabling may be defective.\n");
-}
-
 static int m88e1116r_config_init(struct phy_device *phydev)
 {
 	int err;
@@ -2201,7 +2186,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2223,7 +2207,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1118,
@@ -2264,7 +2247,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1318S,
@@ -2308,7 +2290,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1149R,
@@ -2364,7 +2345,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1510,
@@ -2390,7 +2370,6 @@ static struct phy_driver marvell_drivers[] = {
 		.set_loopback = genphy_loopback,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
@@ -2413,7 +2392,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1545,
@@ -2436,7 +2414,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2479,7 +2456,6 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
-		.link_change_notify = m88e1011_link_change_notify,
 	},
 };
 
-- 
2.25.1

[PATCH net-next 3/3] net: phy: aquantia: remove downshift warning now that phylib takes care

[Heiner Kallweit]

Now that phylib notifies the user of a downshift we can remove
this functionality from the driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/aquantia_main.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 31927b2c7..837d5eaf9 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -290,17 +290,6 @@ static int aqr_read_status(struct phy_device *phydev)
 	return genphy_c45_read_status(phydev);
 }
 
-static int aqr107_read_downshift_event(struct phy_device *phydev)
-{
-	int val;
-
-	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS1);
-	if (val < 0)
-		return val;
-
-	return !!(val & MDIO_AN_TX_VEND_INT_STATUS1_DOWNSHIFT);
-}
-
 static int aqr107_read_rate(struct phy_device *phydev)
 {
 	int val;
@@ -377,13 +366,7 @@ static int aqr107_read_status(struct phy_device *phydev)
 		break;
 	}
 
-	val = aqr107_read_downshift_event(phydev);
-	if (val <= 0)
-		return val;
-
-	phydev_warn(phydev, "Downshift occurred! Cabling may be defective.\n");
-
-	/* Read downshifted rate from vendor register */
+	/* Read possibly downshifted rate from vendor register */
 	return aqr107_read_rate(phydev);
 }
 
@@ -506,9 +489,6 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (!ret)
 		aqr107_chip_info(phydev);
 
-	/* ensure that a latched downshift event is cleared */
-	aqr107_read_downshift_event(phydev);
-
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
@@ -533,9 +513,6 @@ static int aqcs109_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	/* ensure that a latched downshift event is cleared */
-	aqr107_read_downshift_event(phydev);
-
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
-- 
2.25.1