[PATCH net-next] net: phy: add GBit master / slave error detection

[PATCH net-next] net: phy: add GBit master / slave error detection

[Heiner Kallweit]

Certain PHY's have issues when operating in GBit slave mode and can
be forced to master mode. Examples are RTL8211C, also the Micrel PHY
driver has a DT setting to force master mode.
If two such chips are link partners the autonegotiation will fail.
Standard defines a self-clearing on read, latched-high bit to
indicate this error. Check this bit to inform the user.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 5 +++++
 include/uapi/linux/mii.h     | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b9f5f40a..249c6f75 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1551,6 +1551,11 @@ int genphy_read_status(struct phy_device *phydev)
 			if (lpagb < 0)
 				return lpagb;
 
+			if (lpagb & LPA_1000MSFAIL) {
+				phydev_err(phydev, "Master/Slave resolution failed, maybe conflicting manual settings?\n");
+				return -ENOLINK;
+			}
+
 			adv = phy_read(phydev, MII_CTRL1000);
 			if (adv < 0)
 				return adv;
diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index b5c2fdcf..a5062165 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -136,6 +136,7 @@
 #define CTL1000_ENABLE_MASTER	0x1000
 
 /* 1000BASE-T Status register */
+#define LPA_1000MSFAIL		0x8000	/* Master/Slave resolution failure */
 #define LPA_1000LOCALRXOK	0x2000	/* Link partner local receiver status */
 #define LPA_1000REMRXOK		0x1000	/* Link partner remote receiver status */
 #define LPA_1000FULL		0x0800	/* Link partner 1000BASE-T full duplex */
-- 
2.18.0

Re: [PATCH net-next] net: phy: add GBit master / slave error detection

[Florian Fainelli]

On 07/17/2018 11:14 PM, Heiner Kallweit wrote:
> Certain PHY's have issues when operating in GBit slave mode and can
> be forced to master mode. Examples are RTL8211C, also the Micrel PHY
> driver has a DT setting to force master mode.
> If two such chips are link partners the autonegotiation will fail.
> Standard defines a self-clearing on read, latched-high bit to
> indicate this error. Check this bit to inform the user.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 5 +++++
>  include/uapi/linux/mii.h     | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index b9f5f40a..249c6f75 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1551,6 +1551,11 @@ int genphy_read_status(struct phy_device *phydev)
>  			if (lpagb < 0)
>  				return lpagb;
>  
> +			if (lpagb & LPA_1000MSFAIL) {
> +				phydev_err(phydev, "Master/Slave resolution failed, maybe conflicting manual settings?\n");
> +				return -ENOLINK;
> +			}
> +

You should also be able to read whether Master/Slave was configured as
automatic or manual, and possibly issue a different message when
automatic/forced is specified?

AFAIR there was a patch a while ago from Mellanox guys that was possibly
extending the link notification with an error cause, this sounds like
something that could be useful to report to user space somehow to help
troubleshoot link down events.

Thanks for your improvements to PHYLIB.
-- 
Florian

Re: [PATCH net-next] net: phy: add GBit master / slave error detection

[Heiner Kallweit]

On 18.07.2018 11:22, Florian Fainelli wrote:
> 
> 
> On 07/17/2018 11:14 PM, Heiner Kallweit wrote:
>> Certain PHY's have issues when operating in GBit slave mode and can
>> be forced to master mode. Examples are RTL8211C, also the Micrel PHY
>> driver has a DT setting to force master mode.
>> If two such chips are link partners the autonegotiation will fail.
>> Standard defines a self-clearing on read, latched-high bit to
>> indicate this error. Check this bit to inform the user.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy_device.c | 5 +++++
>>  include/uapi/linux/mii.h     | 1 +
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index b9f5f40a..249c6f75 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1551,6 +1551,11 @@ int genphy_read_status(struct phy_device *phydev)
>>  			if (lpagb < 0)
>>  				return lpagb;
>>  
>> +			if (lpagb & LPA_1000MSFAIL) {
>> +				phydev_err(phydev, "Master/Slave resolution failed, maybe conflicting manual settings?\n");
>> +				return -ENOLINK;
>> +			}
>> +
> 
> You should also be able to read whether Master/Slave was configured as
> automatic or manual, and possibly issue a different message when
> automatic/forced is specified?
> 
Indeed, this could be done, based on the local automatic/manual setting.

> AFAIR there was a patch a while ago from Mellanox guys that was possibly
> extending the link notification with an error cause, this sounds like
> something that could be useful to report to user space somehow to help
> troubleshoot link down events.
> 
Do you by chance have a reference to this patch? There's heavy development
on the Mellanox drivers with a lot of patches.

> Thanks for your improvements to PHYLIB.
> 

Re: [PATCH net-next] net: phy: add GBit master / slave error detection

[Andrew Lunn]

> > AFAIR there was a patch a while ago from Mellanox guys that was possibly
> > extending the link notification with an error cause, this sounds like
> > something that could be useful to report to user space somehow to help
> > troubleshoot link down events.
> > 
> Do you by chance have a reference to this patch? There's heavy development
> on the Mellanox drivers with a lot of patches.

Hi Heiner, Florian

A general mechanism has been added to allow error messages to be
reported via netlink sockets. I think wifi was the first to actually
make use of it, since i think Johannes Berg did the core work, but
other parts of the stack have also started using it.

Just picking a commit at random, maybe not the best of examples:

Fixes: 768075ebc238 ("nl80211: add a few extended error strings to key parsing")

       Andrew

Re: [PATCH net-next] net: phy: add GBit master / slave error detection

[Heiner Kallweit]

On 19.07.2018 16:46, Andrew Lunn wrote:
>>> AFAIR there was a patch a while ago from Mellanox guys that was possibly
>>> extending the link notification with an error cause, this sounds like
>>> something that could be useful to report to user space somehow to help
>>> troubleshoot link down events.
>>>
>> Do you by chance have a reference to this patch? There's heavy development
>> on the Mellanox drivers with a lot of patches.
> 
> Hi Heiner, Florian
> 
> A general mechanism has been added to allow error messages to be
> reported via netlink sockets. I think wifi was the first to actually
> make use of it, since i think Johannes Berg did the core work, but
> other parts of the stack have also started using it.
> 
I think you mean the devlink interface. With regard to nl80211 I'm aware
that it's used for communication between Wifi core and userspace tools /
daemons like hostapd and wpa_supplicant.
The devlink use case reminds me of swconfig for switch port configuration
under OpenWRT.

For our use case I think this is overkill, and I wonder what the
userspace tool would be that consumes our link down error info.

Heiner

> Just picking a commit at random, maybe not the best of examples:
> 
> Fixes: 768075ebc238 ("nl80211: add a few extended error strings to key parsing")
> 
>        Andrew
> 

Re: [PATCH net-next] net: phy: add GBit master / slave error detection

[Andrew Lunn]

On Thu, Jul 19, 2018 at 11:11:53PM +0200, Heiner Kallweit wrote:
> On 19.07.2018 16:46, Andrew Lunn wrote:
> >>> AFAIR there was a patch a while ago from Mellanox guys that was possibly
> >>> extending the link notification with an error cause, this sounds like
> >>> something that could be useful to report to user space somehow to help
> >>> troubleshoot link down events.
> >>>
> >> Do you by chance have a reference to this patch? There's heavy development
> >> on the Mellanox drivers with a lot of patches.
> > 
> > Hi Heiner, Florian
> > 
> > A general mechanism has been added to allow error messages to be
> > reported via netlink sockets. I think wifi was the first to actually
> > make use of it, since i think Johannes Berg did the core work, but
> > other parts of the stack have also started using it.
> > 
> I think you mean the devlink interface.

devlink uses netlink, so it can use these extended error message. But
all forms of netlink sockets can use this.

What Florian might be referring to, is that when netif_carrier_off()
or netif_carrier_on() is called, a netlink message is sent to
userspace. Tools link ip monitor can be used to display these events.
I think Florian is suggesting a more detailed message could be display
about master/slave issues. However, how you get that information to
rtnl_fill_ifinfo() i don't know.

	   Andrew