[PATCH net] net: phy: Don't assume loopback is supported

[PATCH net] net: phy: Don't assume loopback is supported

[Jose Abreu]

Some PHYs may not support loopback mode so we need to check if register
is read-only.

Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework")
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 drivers/net/phy/phy_device.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 49fdd1ee798e..a749639d98c3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1918,8 +1918,24 @@ EXPORT_SYMBOL(genphy_resume);
 
 int genphy_loopback(struct phy_device *phydev, bool enable)
 {
-	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
-			  enable ? BMCR_LOOPBACK : 0);
+	int ret;
+
+	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
+			 enable ? BMCR_LOOPBACK : 0);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read(phydev, MII_BMCR);
+	if (ret < 0)
+		return ret;
+
+	if (enable) {
+		if (ret & BMCR_LOOPBACK)
+			return 0;
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(genphy_loopback);
 
-- 
2.7.4

Re: [PATCH net] net: phy: Don't assume loopback is supported

[David Miller]

From: Jose Abreu <jose.abreu@synopsys.com>
Date: Thu, 14 Mar 2019 11:37:21 +0100

> Some PHYs may not support loopback mode so we need to check if register
> is read-only.
> 
> Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework")
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>

Andrew et al., please review.

Re: [PATCH net] net: phy: Don't assume loopback is supported

[Florian Fainelli]

On 3/14/19 3:37 AM, Jose Abreu wrote:
> Some PHYs may not support loopback mode so we need to check if register
> is read-only.
> 

In that case it may be appropriate to have a specific PHY driver that
implements a set_loopback() method returning -EOPNOTSUPP instead of
changing the generic PHY implementation.

> Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework")
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Joao Pinto <joao.pinto@synopsys.com>

This looks fine anyway:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/net/phy/phy_device.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 49fdd1ee798e..a749639d98c3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1918,8 +1918,24 @@ EXPORT_SYMBOL(genphy_resume);
>  
>  int genphy_loopback(struct phy_device *phydev, bool enable)
>  {
> -	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> -			  enable ? BMCR_LOOPBACK : 0);
> +	int ret;
> +
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> +			 enable ? BMCR_LOOPBACK : 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read(phydev, MII_BMCR);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (enable) {
> +		if (ret & BMCR_LOOPBACK)
> +			return 0;
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(genphy_loopback);
>  
> 


-- 
Florian

Re: [PATCH net] net: phy: Don't assume loopback is supported

[Heiner Kallweit]

On 14.03.2019 11:37, Jose Abreu wrote:
> Some PHYs may not support loopback mode so we need to check if register
> is read-only.
> 
As I read Clause 22 this is a mandatory feature, the related bit is
specified as R/W. Do you have an actual example of a PHY w/o loopback
mode that doesn't allow to change this bit?

> Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework")
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Joao Pinto <joao.pinto@synopsys.com>
> ---
>  drivers/net/phy/phy_device.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 49fdd1ee798e..a749639d98c3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1918,8 +1918,24 @@ EXPORT_SYMBOL(genphy_resume);
>  
>  int genphy_loopback(struct phy_device *phydev, bool enable)
>  {
> -	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> -			  enable ? BMCR_LOOPBACK : 0);
> +	int ret;
> +
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> +			 enable ? BMCR_LOOPBACK : 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read(phydev, MII_BMCR);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (enable) {
> +		if (ret & BMCR_LOOPBACK)
> +			return 0;
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(genphy_loopback);
>  
> 

Re: [PATCH net] net: phy: Don't assume loopback is supported

[Andrew Lunn]

On Fri, Mar 15, 2019 at 03:48:41PM -0700, Florian Fainelli wrote:
> On 3/14/19 3:37 AM, Jose Abreu wrote:
> > Some PHYs may not support loopback mode so we need to check if register
> > is read-only.
> > 
> 
> In that case it may be appropriate to have a specific PHY driver that
> implements a set_loopback() method returning -EOPNOTSUPP instead of
> changing the generic PHY implementation.

Hi Jose

Since Heiner says this is a mandatory feature, we should not really
penalise conformant PHYs just because there is one broken PHY.

Please handle this in the PHY driver, by implementing the set_loopback
call.

	Andrew

Re: [PATCH net] net: phy: Don't assume loopback is supported

[Jose Abreu]

Hi Andrew and Heiner,

On 3/17/2019 6:38 PM, Andrew Lunn wrote:
> On Fri, Mar 15, 2019 at 03:48:41PM -0700, Florian Fainelli wrote:
>> On 3/14/19 3:37 AM, Jose Abreu wrote:
>>> Some PHYs may not support loopback mode so we need to check if register
>>> is read-only.
>>>
>>
>> In that case it may be appropriate to have a specific PHY driver that
>> implements a set_loopback() method returning -EOPNOTSUPP instead of
>> changing the generic PHY implementation.
> 
> Hi Jose
> 
> Since Heiner says this is a mandatory feature, we should not really
> penalise conformant PHYs just because there is one broken PHY.

We provide PHYs to our customers and in the documentation I have
this can be an optional feature that HW team can choose to have
or not, making the bit read-only or r/w.

Heiner, can you please confirm there is no Clause 22 "pitfalls" /
"hidden comments" that allow this bitfield to be read-only ?

Thanks,
Jose Miguel Abreu

Re: [PATCH net] net: phy: Don't assume loopback is supported

[Andrew Lunn]

> We provide PHYs to our customers and in the documentation I have
> this can be an optional feature that HW team can choose to have
> or not, making the bit read-only or r/w.
> 
> Heiner, can you please confirm there is no Clause 22 "pitfalls" /
> "hidden comments" that allow this bitfield to be read-only ?

Hi Jose

I have the 802.3 standard from 2015. It should be free to download
from the IEEE. So you can go get it yourself.

Section 22.2.4.1.2 defines loopback. I don't see anything which makes
it optional. The only wiggle room you have is where in the PHY the
loopback actually takes place. That is implementation specific, but it
recommends you make it as late as possible in the path so as to test
as much as possible.

If your PHY does not implement loopback, i would say it breaks the
standard. We try to keep workarounds for brokenness in the specific
PHY driver, not the generic code.

    Andrew