Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

[Heiner Kallweit]

Parameter mask of phy_modify() holds the bits to be cleared.
In the mentioned commit parameter mask seems to be inverted in
few cases, what IMO is wrong (see example).
Maybe I miss something, could you please check?

And somehow related:
When adding such helpers, wouldn't it make sense to add
helpers for setting / clearing bits too? Something like:
phy_set_bits(phydev, reg, val) -> phy_modify(phydev, reg, 0, val)

Rgds, Heiner


 int genphy_resume(struct phy_device *phydev)
 {
-	int value;
-
-	value = phy_read(phydev, MII_BMCR);
-	phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);
-
-	return 0;
+	return phy_modify(phydev, MII_BMCR, ~BMCR_PDOWN, 0);
 }

Re: Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

[Russell King - ARM Linux]

On Thu, Jan 04, 2018 at 08:00:53AM +0100, Heiner Kallweit wrote:
> Parameter mask of phy_modify() holds the bits to be cleared.
> In the mentioned commit parameter mask seems to be inverted in
> few cases, what IMO is wrong (see example).

I'd be grateful if you could list those that you think are wrong please.

> Maybe I miss something, could you please check?

It's entirely possible that some are wrong - the patch started out as
having the mask argument inverted, but during its evolution, that was
corrected, and I thought all places had been updated - maybe they were
initially wrong.

I did go through the patch several times before sending it to try to
ensure that it was correct, but must have overlooked some, because the
one you quote is one I definitely looked at several times.  It's highly
likely that if I have another look through the patch, I still won't
spot those that you've found.

> And somehow related:
> When adding such helpers, wouldn't it make sense to add
> helpers for setting / clearing bits too? Something like:
> phy_set_bits(phydev, reg, val) -> phy_modify(phydev, reg, 0, val)

Maybe, but lets try and solve the problems with the existing patch
first.

Thanks for reporting this, and sorry for the hassle.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

[Heiner Kallweit]

Am 04.01.2018 um 12:44 schrieb Russell King - ARM Linux:
> On Thu, Jan 04, 2018 at 08:00:53AM +0100, Heiner Kallweit wrote:
>> Parameter mask of phy_modify() holds the bits to be cleared.
>> In the mentioned commit parameter mask seems to be inverted in
>> few cases, what IMO is wrong (see example).
> 
> I'd be grateful if you could list those that you think are wrong please.
> For function __phy_modify documentation and implementation conflict.
Documentation states "(value & mask) | set" whilst implementation is
"(value & ~mask) | set". Based on the subsequent patches I assume
that your intention is what is documented.

Personally I find "ret & ~mask" more intuitive (see also set_mask_bits
in include/linux/bitops.h) but this may be a question of personal taste.
In kernel code both flavors are used.

+ * Unlocked helper function which allows a PHY register to be modified as
+ * new register value = (old register value & mask) | set
+ */
+int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set)
+{
+	int ret, res;
+
+	ret = __phy_read(phydev, regnum);
+	if (ret >= 0) {
+		res = __phy_write(phydev, regnum, (ret & ~mask) | set);
+		if (res < 0)
+			ret = res;
+	}
+
+	return ret;
+}

Could you please advise whether documentation or implementation reflect
your intention? Then I'll check again which changes I'd consider to
be wrong.

Regards, Heiner

>> Maybe I miss something, could you please check?
> 
> It's entirely possible that some are wrong - the patch started out as
> having the mask argument inverted, but during its evolution, that was
> corrected, and I thought all places had been updated - maybe they were
> initially wrong.
> 
> I did go through the patch several times before sending it to try to
> ensure that it was correct, but must have overlooked some, because the
> one you quote is one I definitely looked at several times.  It's highly
> likely that if I have another look through the patch, I still won't
> spot those that you've found.
> 
>> And somehow related:
>> When adding such helpers, wouldn't it make sense to add
>> helpers for setting / clearing bits too? Something like:
>> phy_set_bits(phydev, reg, val) -> phy_modify(phydev, reg, 0, val)
> 
> Maybe, but lets try and solve the problems with the existing patch
> first.
> 
> Thanks for reporting this, and sorry for the hassle.
> 

Re: Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

[Ivan Khoronzhuk]

+ G.Strashko
The below change also brokes phy connect for am572x..

 int genphy_restart_aneg(struct phy_device *phydev)
 {
-       int ctl = phy_read(phydev, MII_BMCR);
-
-       if (ctl < 0)
-               return ctl;
-
-       ctl |= BMCR_ANENABLE | BMCR_ANRESTART;
-
        /* Don't isolate the PHY if we're negotiating */
-       ctl &= ~BMCR_ISOLATE;
-
-       return phy_write(phydev, MII_BMCR, ctl);
+       return phy_modify(phydev, MII_BMCR, ~BMCR_ISOLATE,
+                         BMCR_ANENABLE | BMCR_ANRESTART);



-- 
Regards,
Ivan Khoronzhuk

Re: Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

[Russell King - ARM Linux]

On Fri, Jan 05, 2018 at 02:44:07AM +0200, Ivan Khoronzhuk wrote:
> + G.Strashko
> The below change also brokes phy connect for am572x..
> 
>  int genphy_restart_aneg(struct phy_device *phydev)
>  {
> -       int ctl = phy_read(phydev, MII_BMCR);
> -
> -       if (ctl < 0)
> -               return ctl;
> -
> -       ctl |= BMCR_ANENABLE | BMCR_ANRESTART;
> -
>         /* Don't isolate the PHY if we're negotiating */
> -       ctl &= ~BMCR_ISOLATE;
> -
> -       return phy_write(phydev, MII_BMCR, ctl);
> +       return phy_modify(phydev, MII_BMCR, ~BMCR_ISOLATE,
> +                         BMCR_ANENABLE | BMCR_ANRESTART);
> 

This should fix it, but it needs a bit more work, as it's also
recently been pointed out that the comment is wrong.  Also, as I
said for the original patch, it needs a thorough review - and
because the last one didn't get that, the problem is now
compounded by two patches needing a thorough review, or both
being looked at as one patch combined.

I'm afraid I've not had time to fully complete this patch yet
because of the recent issues around CPU cache speculation, news
of which broke without warning on Wednesday.

I tested what's in net-next again, and it passes my tests locally -
it's only if I take an interface through an up-down-up sequence
that I see a problem (as discovered by Andrew) which explains why
my testing did not find it.

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
To: Andrew Lunn <andrew@lunn.ch>,Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev@vger.kernel.org
Subject: [PATCH] net: phy: fix wrong masks to phy_modify()

The mask argument for phy_modify() in several locations was inverted.

Fixes: fea23fb591cc ("net: phy: convert read-modify-write to phy_modify()")
Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/at803x.c     |  2 +-
 drivers/net/phy/marvell.c    | 19 +++++++++----------
 drivers/net/phy/phy_device.c |  6 +++---
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index e86c1b8b1b51..a80cce11b252 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -231,7 +231,7 @@ static int at803x_suspend(struct phy_device *phydev)
 
 static int at803x_resume(struct phy_device *phydev)
 {
-	return phy_modify(phydev, MII_BMCR, ~(BMCR_PDOWN | BMCR_ISOLATE), 0);
+	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN | BMCR_ISOLATE, 0);
 }
 
 static int at803x_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 8212c2fd7fe1..5d28063e9327 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -668,7 +668,7 @@ static int m88e3016_config_init(struct phy_device *phydev)
 
 	/* Enable Scrambler and Auto-Crossover */
 	ret = phy_modify(phydev, MII_88E3016_PHY_SPEC_CTRL,
-			 ~MII_88E3016_DISABLE_SCRAMBLER,
+			 MII_88E3016_DISABLE_SCRAMBLER,
 			 MII_88E3016_AUTO_MDIX_CROSSOVER);
 	if (ret < 0)
 		return ret;
@@ -684,9 +684,9 @@ static int m88e1111_config_init_hwcfg_mode(struct phy_device *phydev,
 		mode |= MII_M1111_HWCFG_FIBER_COPPER_AUTO;
 
 	return phy_modify(phydev, MII_M1111_PHY_EXT_SR,
-			  (u16)~(MII_M1111_HWCFG_MODE_MASK |
-				 MII_M1111_HWCFG_FIBER_COPPER_AUTO |
-				 MII_M1111_HWCFG_FIBER_COPPER_RES),
+			  MII_M1111_HWCFG_MODE_MASK |
+			  MII_M1111_HWCFG_FIBER_COPPER_AUTO |
+			  MII_M1111_HWCFG_FIBER_COPPER_RES,
 			  mode);
 }
 
@@ -705,8 +705,7 @@ static int m88e1111_config_init_rgmii_delays(struct phy_device *phydev)
 	}
 
 	return phy_modify(phydev, MII_M1111_PHY_EXT_CR,
-			  (u16)~(MII_M1111_RGMII_RX_DELAY |
-				 MII_M1111_RGMII_TX_DELAY),
+			  MII_M1111_RGMII_RX_DELAY | MII_M1111_RGMII_TX_DELAY,
 			  delay);
 }
 
@@ -833,7 +832,7 @@ static int m88e1510_config_init(struct phy_device *phydev)
 
 		/* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */
 		err = phy_modify(phydev, MII_88E1510_GEN_CTRL_REG_1,
-				 ~MII_88E1510_GEN_CTRL_REG_1_MODE_MASK,
+				 MII_88E1510_GEN_CTRL_REG_1_MODE_MASK,
 				 MII_88E1510_GEN_CTRL_REG_1_MODE_SGMII);
 		if (err < 0)
 			return err;
@@ -957,7 +956,7 @@ static int m88e1145_config_init_rgmii(struct phy_device *phydev)
 		if (err < 0)
 			return err;
 
-		err = phy_modify(phydev, 0x1e, 0xf03f,
+		err = phy_modify(phydev, 0x1e, 0x0fc0,
 				 2 << 9 | /* 36 ohm */
 				 2 << 6); /* 39 ohm */
 		if (err < 0)
@@ -1379,7 +1378,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 
 		/* Setup LED[2] as interrupt pin (active low) */
 		err = __phy_modify(phydev, MII_88E1318S_PHY_LED_TCR,
-				   (u16)~MII_88E1318S_PHY_LED_TCR_FORCE_INT,
+				   MII_88E1318S_PHY_LED_TCR_FORCE_INT,
 				   MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
 				   MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);
 		if (err < 0)
@@ -1419,7 +1418,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 
 		/* Clear WOL status and disable magic packet matching */
 		err = __phy_modify(phydev, MII_88E1318S_PHY_WOL_CTRL,
-				   (u16)~MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE,
+				   MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE,
 				   MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS);
 		if (err < 0)
 			goto error;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e5ddc5ae56d1..e1acb3052515 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1353,7 +1353,7 @@ EXPORT_SYMBOL(genphy_setup_forced);
 int genphy_restart_aneg(struct phy_device *phydev)
 {
 	/* Don't isolate the PHY if we're negotiating */
-	return phy_modify(phydev, MII_BMCR, ~BMCR_ISOLATE,
+	return phy_modify(phydev, MII_BMCR, BMCR_ISOLATE,
 			  BMCR_ANENABLE | BMCR_ANRESTART);
 }
 EXPORT_SYMBOL(genphy_restart_aneg);
@@ -1626,13 +1626,13 @@ EXPORT_SYMBOL(genphy_suspend);
 
 int genphy_resume(struct phy_device *phydev)
 {
-	return phy_modify(phydev, MII_BMCR, ~BMCR_PDOWN, 0);
+	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
 }
 EXPORT_SYMBOL(genphy_resume);
 
 int genphy_loopback(struct phy_device *phydev, bool enable)
 {
-	return phy_modify(phydev, MII_BMCR, ~BMCR_LOOPBACK,
+	return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
 			  enable ? BMCR_LOOPBACK : 0);
 }
 EXPORT_SYMBOL(genphy_loopback);
-- 
2.7.4

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()"

[Andrew Lunn]

On Thu, Jan 04, 2018 at 08:00:53AM +0100, Heiner Kallweit wrote:
> Parameter mask of phy_modify() holds the bits to be cleared.
> In the mentioned commit parameter mask seems to be inverted in
> few cases, what IMO is wrong (see example).
> Maybe I miss something, could you please check?

Hi Heiner

mdio reads/writes can be traced via events. Could you log them for
your board, with and without this patch applied. Hopefully the traces
are similar enough that issues like this can be spotted.

Thanks
	Andrew