[PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[Russell King - ARM Linux admin]

Hi,

tinywrkb, please can you test this series to ensure that it fixes
your problem - the previous version has turned out to be a non-starter
as it introduces more problems, thanks!

The following series attempts to address an issue spotted by tinywrkb
with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
the negotiated link.

Before commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in
genphy_read_status"), we would read not only the link partner's
advertisement, but also our own advertisement from the PHY registers,
and use both to derive the PHYs current link mode.  This works when the
AR8035 downgrades the speed, because it appears that the AR8035 clears
link mode bits in the advertisement registers as part of the downgrade.

Commentary: what is not yet known is whether the AR8035 restores the
            advertisement register when the link goes down to the
	    previous state.

However, since the above referenced commit, we no longer use the PHYs
advertisement registers, instead converting the link partner's
advertisement to the ethtool link mode array, and combine that with
phylib's cached version of our advertisement - which is not updated on
speed downgrade.

This results in phylib disagreeing with the actual operating mode of
the PHY.

Commentary: I wonder how many more PHY drivers are broken by this
	    commit, but have yet to be discovered.

The obvious way to address this would be to disable the downgrade
feature, and indeed this does fix the problem in tinywrkb's case - his
link partner instead downgrades the speed by reducing its
advertisement, resulting in phylib correctly evaluating a slower speed.

However, it has a serious drawback - the gigabit control register (MII
register 9) appears to become read only.  It seems the only way to
update the register is to re-enable the downgrade feature, reset the
PHY, changing register 9, disable the downgrade feature, and reset the
PHY again.

This series attempts to address the problem using a different approach,
similar to the approach taken with Marvell PHYs.  The AR8031, AR8033
and AR8035 have a PHY-Specific Status register which reports the
actual operating mode of the PHY - both speed and duplex.  This
register correctly reports the operating mode irrespective of whether
autoneg is enabled or not.  We use this register to fill in phylib's
speed and duplex parameters.

In detail:

Patch 1 fixes a bug where writing to register 9 does not update
phylib's advertisement mask in the same way that writing register 4
does; this looks like an omission from when gigabit PHY support came
into being.

Patch 2 seperates the generic phylib code which reads the link partners
advertisement from the PHY, so that we can re-use this in the Atheros
PHY driver.

Patch 3 seperates the generic phylib pause mode; phylib provides no
help for MAC drivers to ascertain the negotiated pause mode, it merely
copies the link partner's pause mode bits into its own variables.

Commentary: Both the aforementioned Atheros PHYs and Marvell PHYs
            provide the resolved pause modes in terms of whether 
	    we should transmit pause frames, or whether we should
	    allow reception of pause frames.  Surely the resolution
	    of this should be in phylib?

Patch 4 provides the Atheros PHY driver with a private "read_status"
implementation that fills in phylib's speed and duplex settings
depending on the PHY-Specific status register.  This ensures that
phylib and the MAC driver match the operating mode that the PHY has
decided to use.  Since the register also gives us MDIX status, we
can trivially fill that status in as well.

Note that, although the bits mentioned in this patch for this register
match those in th Marvell PHY driver, and it is located at the same
address, the meaning of other register bits varies between the PHYs.
Therefore, I do not feel that it would be appropriate to make this some
kind of generic function.

 drivers/net/phy/at803x.c     | 69 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy-core.c   | 20 ++++++++-----
 drivers/net/phy/phy.c        |  5 ++++
 drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++----------------
 include/linux/mii.h          |  9 ++++++
 include/linux/phy.h          |  2 ++
 6 files changed, 138 insertions(+), 32 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 1/4] net: phy: fix write to mii-ctrl1000 register

[Russell King]

When userspace writes to the MII_ADVERTISE register, we update phylib's
advertising mask and trigger a renegotiation.  However, writing to the
MII_CTRL1000 register, which contains the gigabit advertisement, does
neither.  This can lead to phylib's copy of the advertisement becoming
de-synced with the values in the PHY register set, which can result in
incorrect negotiation resolution.

Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 5 +++++
 include/linux/mii.h   | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7af390d8bbdb..068a08a50064 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -458,6 +458,11 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 							   val);
 				change_autoneg = true;
 				break;
+			case MII_CTRL1000:
+				mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
+							        val);
+				change_autoneg = true;
+				break;
 			default:
 				/* do nothing */
 				break;
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 5cd824c1c0ca..4ce8901a1af6 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -455,6 +455,15 @@ static inline void mii_lpa_mod_linkmode_lpa_t(unsigned long *lp_advertising,
 			 lp_advertising, lpa & LPA_LPACK);
 }
 
+static inline void mii_ctrl1000_mod_linkmode_adv_t(unsigned long *advertising,
+						   u32 ctrl1000)
+{
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, advertising,
+			 ctrl1000 & ADVERTISE_1000HALF);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, advertising,
+			 ctrl1000 & ADVERTISE_1000FULL);
+}
+
 /**
  * linkmode_adv_to_lcl_adv_t
  * @advertising:pointer to linkmode advertising
-- 
2.7.4

[PATCH 2/4] net: phy: extract link partner advertisement reading

[Russell King]

Move reading the link partner advertisement out of genphy_read_status()
into its own separate function.  This will allow re-use of this code by
PHY drivers that are able to read the resolved status from the PHY.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++++-----------------
 include/linux/phy.h          |  1 +
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2c506c3c6c7b..d17aafcde5a1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1782,32 +1782,9 @@ int genphy_update_link(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_update_link);
 
-/**
- * genphy_read_status - check the link status and update current link state
- * @phydev: target phy_device struct
- *
- * Description: Check the link, then figure out the current state
- *   by comparing what we advertise with what the link partner
- *   advertises.  Start by checking the gigabit possibilities,
- *   then move on to 10/100.
- */
-int genphy_read_status(struct phy_device *phydev)
+int genphy_read_lpa(struct phy_device *phydev)
 {
-	int adv, lpa, lpagb, err, old_link = phydev->link;
-
-	/* Update the link, but return if there was an error */
-	err = genphy_update_link(phydev);
-	if (err)
-		return err;
-
-	/* why bother the PHY if nothing can have changed */
-	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
-		return 0;
-
-	phydev->speed = SPEED_UNKNOWN;
-	phydev->duplex = DUPLEX_UNKNOWN;
-	phydev->pause = 0;
-	phydev->asym_pause = 0;
+	int adv, lpa, lpagb;
 
 	linkmode_zero(phydev->lp_advertising);
 
@@ -1838,6 +1815,44 @@ int genphy_read_status(struct phy_device *phydev)
 			return lpa;
 
 		mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(genphy_read_lpa);
+
+/**
+ * genphy_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises.  Start by checking the gigabit possibilities,
+ *   then move on to 10/100.
+ */
+int genphy_read_status(struct phy_device *phydev)
+{
+	int err, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	err = genphy_read_lpa(phydev);
+	if (err < 0)
+		return err;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
 		phy_resolve_aneg_linkmode(phydev);
 	} else if (phydev->autoneg == AUTONEG_DISABLE) {
 		int bmcr = phy_read(phydev, MII_BMCR);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0600754ce5e7..bef7f30af22d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1085,6 +1085,7 @@ int genphy_config_eee_advert(struct phy_device *phydev);
 int genphy_config_aneg(struct phy_device *phydev);
 int genphy_aneg_done(struct phy_device *phydev);
 int genphy_update_link(struct phy_device *phydev);
+int genphy_read_lpa(struct phy_device *phydev);
 int genphy_read_status(struct phy_device *phydev);
 int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
-- 
2.7.4

[PATCH 3/4] net: phy: extract pause mode

[Russell King]

Extract the update of phylib's software pause mode state from
genphy_read_status(), so that we can re-use this functionality with
PHYs that have alternative ways to read the negotiation results.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy-core.c | 20 +++++++++++++-------
 include/linux/phy.h        |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 16667fbac8bf..91b856b31fd6 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -278,6 +278,18 @@ void of_set_phy_eee_broken(struct phy_device *phydev)
 	phydev->eee_broken_modes = broken;
 }
 
+void phy_resolve_aneg_pause(struct phy_device *phydev)
+{
+	if (phydev->duplex == DUPLEX_FULL) {
+		phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+						  phydev->lp_advertising);
+		phydev->asym_pause = linkmode_test_bit(
+			ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+			phydev->lp_advertising);
+	}
+}
+EXPORT_SYMBOL_GPL(phy_resolve_aneg_pause);
+
 /**
  * phy_resolve_aneg_linkmode - resolve the advertisements into phy settings
  * @phydev: The phy_device struct
@@ -300,13 +312,7 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
 			break;
 		}
 
-	if (phydev->duplex == DUPLEX_FULL) {
-		phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-						  phydev->lp_advertising);
-		phydev->asym_pause = linkmode_test_bit(
-			ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-			phydev->lp_advertising);
-	}
+	phy_resolve_aneg_pause(phydev);
 }
 EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bef7f30af22d..224e81740963 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -686,6 +686,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
 	return phydev->state >= PHY_UP;
 }
 
+void phy_resolve_aneg_pause(struct phy_device *phydev);
 void phy_resolve_aneg_linkmode(struct phy_device *phydev);
 
 /**
-- 
2.7.4

[PATCH 4/4] net: phy: at803x: use operating parameters from PHY-specific status

[Russell King]

Read the PHY-specific status register for the current operating mode
(speed and duplex) of the PHY.  This register reflects the actual
mode that the PHY has resolved depending on either the advertisements
of autoneg is enabled, or the forced mode if autoneg is disabled.

This ensures that phylib's software state always tracks the hardware
state.

It seems both AR8033 (which uses the AR8031 ID) and AR8035 support
this status register.  AR8030 is not known at the present time.

Reported-by: tinywrkb <tinywrkb@gmail.com>
Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/at803x.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index b3893347804d..7dca170fdd80 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -16,6 +16,15 @@
 #include <linux/of_gpio.h>
 #include <linux/gpio/consumer.h>
 
+#define AT803X_SPECIFIC_STATUS			0x11
+#define AT803X_SS_SPEED_MASK			(3 << 14)
+#define AT803X_SS_SPEED_1000			(2 << 14)
+#define AT803X_SS_SPEED_100			(1 << 14)
+#define AT803X_SS_SPEED_10			(0 << 14)
+#define AT803X_SS_DUPLEX			BIT(13)
+#define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
+#define AT803X_SS_MDIX				BIT(6)
+
 #define AT803X_INTR_ENABLE			0x12
 #define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
 #define AT803X_INTR_ENABLE_SPEED_CHANGED	BIT(14)
@@ -404,6 +413,64 @@ static int at803x_aneg_done(struct phy_device *phydev)
 	return aneg_done;
 }
 
+static int at803x_read_status(struct phy_device *phydev)
+{
+	int ss, err, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	err = genphy_read_lpa(phydev);
+	if (err < 0)
+		return err;
+
+	/* Read the AT8035 PHY-Specific Status register, which indicates the
+	 * speed and duplex that the PHY is actually using, irrespective of
+	 * whether we are in autoneg mode or not.
+	 */
+	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
+	if (ss < 0)
+		return ss;
+
+	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
+		switch (ss & AT803X_SS_SPEED_MASK) {
+		case AT803X_SS_SPEED_10:
+			phydev->speed = SPEED_10;
+			break;
+		case AT803X_SS_SPEED_100:
+			phydev->speed = SPEED_100;
+			break;
+		case AT803X_SS_SPEED_1000:
+			phydev->speed = SPEED_1000;
+			break;
+		}
+		if (ss & AT803X_SS_DUPLEX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+		if (ss & AT803X_SS_MDIX)
+			phydev->mdix = ETH_TP_MDI_X;
+		else
+			phydev->mdix = ETH_TP_MDI;
+	}
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
+		phy_resolve_aneg_pause(phydev);
+
+	return 0;
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* ATHEROS 8035 */
@@ -417,6 +484,7 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_GBIT_FEATURES */
+	.read_status		= at803x_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 	.get_eee		= at8035_get_eee,
@@ -447,6 +515,7 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_GBIT_FEATURES */
+	.read_status		= at803x_read_status,
 	.aneg_done		= at803x_aneg_done,
 	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
-- 
2.7.4

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[Russell King - ARM Linux admin]

On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> Commentary: what is not yet known is whether the AR8035 restores the
>             advertisement register when the link goes down to the
> 	    previous state.

I've just found my 2-pair cable, and it appears that the AR8035 does
indeed restore the advertisement register.

Using the 2-pair cable (which takes a while for the link to come up
at 100mbit) results in register 9 becoming zero:

   1140 796d 004d d072 15e1 c1e1 000d 2001
   0000 0000 0800 0000 0000 4007 b29a a000
   0862 7c30 0000 dc20 082c 0000 04e8 0000
   3200 3000 0000 060d 0000 0005 2d47 8100.

Disconnecting the 2-pair, and connecting a 4-pair cable results in a
gigabit link with the advertisement restored:

   1140 796d 004d d072 15e1 c1e1 000d 2001
   0000 0200 2800 0000 0000 4007 b29a a000
   0862 bc50 0000 dc00 082c 0000 04e8 0000
   3200 3000 0000 060e 0000 0005 2d47 8100.

Note that register 0x11 bit 5 reports when downshift is active.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[tinywrkb]

On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> Hi,
> 
> tinywrkb, please can you test this series to ensure that it fixes
> your problem - the previous version has turned out to be a non-starter
> as it introduces more problems, thanks!

Yes, this solves my issue.
Tested against v5.3.
Thanks Russell and everyone else who helped.
> 
> The following series attempts to address an issue spotted by tinywrkb
> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> the negotiated link.
> 
> Before commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in
> genphy_read_status"), we would read not only the link partner's
> advertisement, but also our own advertisement from the PHY registers,
> and use both to derive the PHYs current link mode.  This works when the
> AR8035 downgrades the speed, because it appears that the AR8035 clears
> link mode bits in the advertisement registers as part of the downgrade.
> 
> Commentary: what is not yet known is whether the AR8035 restores the
>             advertisement register when the link goes down to the
> 	    previous state.
> 
> However, since the above referenced commit, we no longer use the PHYs
> advertisement registers, instead converting the link partner's
> advertisement to the ethtool link mode array, and combine that with
> phylib's cached version of our advertisement - which is not updated on
> speed downgrade.
> 
> This results in phylib disagreeing with the actual operating mode of
> the PHY.
> 
> Commentary: I wonder how many more PHY drivers are broken by this
> 	    commit, but have yet to be discovered.
> 
> The obvious way to address this would be to disable the downgrade
> feature, and indeed this does fix the problem in tinywrkb's case - his
> link partner instead downgrades the speed by reducing its
> advertisement, resulting in phylib correctly evaluating a slower speed.
> 
> However, it has a serious drawback - the gigabit control register (MII
> register 9) appears to become read only.  It seems the only way to
> update the register is to re-enable the downgrade feature, reset the
> PHY, changing register 9, disable the downgrade feature, and reset the
> PHY again.
> 
> This series attempts to address the problem using a different approach,
> similar to the approach taken with Marvell PHYs.  The AR8031, AR8033
> and AR8035 have a PHY-Specific Status register which reports the
> actual operating mode of the PHY - both speed and duplex.  This
> register correctly reports the operating mode irrespective of whether
> autoneg is enabled or not.  We use this register to fill in phylib's
> speed and duplex parameters.
> 
> In detail:
> 
> Patch 1 fixes a bug where writing to register 9 does not update
> phylib's advertisement mask in the same way that writing register 4
> does; this looks like an omission from when gigabit PHY support came
> into being.
> 
> Patch 2 seperates the generic phylib code which reads the link partners
> advertisement from the PHY, so that we can re-use this in the Atheros
> PHY driver.
> 
> Patch 3 seperates the generic phylib pause mode; phylib provides no
> help for MAC drivers to ascertain the negotiated pause mode, it merely
> copies the link partner's pause mode bits into its own variables.
> 
> Commentary: Both the aforementioned Atheros PHYs and Marvell PHYs
>             provide the resolved pause modes in terms of whether 
> 	    we should transmit pause frames, or whether we should
> 	    allow reception of pause frames.  Surely the resolution
> 	    of this should be in phylib?
> 
> Patch 4 provides the Atheros PHY driver with a private "read_status"
> implementation that fills in phylib's speed and duplex settings
> depending on the PHY-Specific status register.  This ensures that
> phylib and the MAC driver match the operating mode that the PHY has
> decided to use.  Since the register also gives us MDIX status, we
> can trivially fill that status in as well.
> 
> Note that, although the bits mentioned in this patch for this register
> match those in th Marvell PHY driver, and it is located at the same
> address, the meaning of other register bits varies between the PHYs.
> Therefore, I do not feel that it would be appropriate to make this some
> kind of generic function.
> 
>  drivers/net/phy/at803x.c     | 69 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/phy-core.c   | 20 ++++++++-----
>  drivers/net/phy/phy.c        |  5 ++++
>  drivers/net/phy/phy_device.c | 65 +++++++++++++++++++++++++----------------
>  include/linux/mii.h          |  9 ++++++
>  include/linux/phy.h          |  2 ++
>  6 files changed, 138 insertions(+), 32 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 1/4] net: phy: fix write to mii-ctrl1000 register

[Andrew Lunn]

On Sun, Sep 22, 2019 at 12:00:10PM +0100, Russell King wrote:
> When userspace writes to the MII_ADVERTISE register, we update phylib's
> advertising mask and trigger a renegotiation.  However, writing to the
> MII_CTRL1000 register, which contains the gigabit advertisement, does
> neither.  This can lead to phylib's copy of the advertisement becoming
> de-synced with the values in the PHY register set, which can result in
> incorrect negotiation resolution.
> 
> Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 2/4] net: phy: extract link partner advertisement reading

[Andrew Lunn]

On Sun, Sep 22, 2019 at 12:00:15PM +0100, Russell King wrote:
> Move reading the link partner advertisement out of genphy_read_status()
> into its own separate function.  This will allow re-use of this code by
> PHY drivers that are able to read the resolved status from the PHY.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 3/4] net: phy: extract pause mode

[Andrew Lunn]

On Sun, Sep 22, 2019 at 12:00:20PM +0100, Russell King wrote:
> Extract the update of phylib's software pause mode state from
> genphy_read_status(), so that we can re-use this functionality with
> PHYs that have alternative ways to read the negotiation results.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

 

Re: [PATCH 4/4] net: phy: at803x: use operating parameters from PHY-specific status

[Andrew Lunn]

On Sun, Sep 22, 2019 at 12:00:25PM +0100, Russell King wrote:
> Read the PHY-specific status register for the current operating mode
> (speed and duplex) of the PHY.  This register reflects the actual
> mode that the PHY has resolved depending on either the advertisements
> of autoneg is enabled, or the forced mode if autoneg is disabled.
> 
> This ensures that phylib's software state always tracks the hardware
> state.
> 
> It seems both AR8033 (which uses the AR8031 ID) and AR8035 support
> this status register.  AR8030 is not known at the present time.
> 
> Reported-by: tinywrkb <tinywrkb@gmail.com>
> Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[Andrew Lunn]

On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> Hi,
> 
> tinywrkb, please can you test this series to ensure that it fixes
> your problem - the previous version has turned out to be a non-starter
> as it introduces more problems, thanks!
> 
> The following series attempts to address an issue spotted by tinywrkb
> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> the negotiated link.

Hi Russell

This all looks sensible.

One things we need to be careful of, is this is for net and so stable.
But only some of the patches have fixes-tags. I don't know if we
should add fixes tags to all the patches, just to give back porters a
hint that they are all needed? It won't compile without the patches,
so at least it fails safe.

   Andrew

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[Russell King - ARM Linux admin]

On Sun, Sep 22, 2019 at 06:53:35PM +0200, Andrew Lunn wrote:
> On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> > Hi,
> > 
> > tinywrkb, please can you test this series to ensure that it fixes
> > your problem - the previous version has turned out to be a non-starter
> > as it introduces more problems, thanks!
> > 
> > The following series attempts to address an issue spotted by tinywrkb
> > with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> > the negotiated link.
> 
> Hi Russell
> 
> This all looks sensible.
> 
> One things we need to be careful of, is this is for net and so stable.

Since the regression was introduced in 5.1, it should be backported
to stable trees.

> But only some of the patches have fixes-tags. I don't know if we
> should add fixes tags to all the patches, just to give back porters a
> hint that they are all needed? It won't compile without the patches,
> so at least it fails safe.

I only put Fixes: tags on patches that are actually fixing something.
Quoting submitting-patches.rst:

  A Fixes: tag indicates that the patch fixes an issue in a previous
  commit.

Since the preceding two patches are just preparing for the fix, and
not actually fixing an issue in themselves, it seems wrong to add a
Fixes: tag for them.  However, mentioning it in the commit message
for the patch that does fix the issue is probably worth it.  Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[Florian Fainelli]

On 9/22/2019 10:52 AM, Russell King - ARM Linux admin wrote:
> On Sun, Sep 22, 2019 at 06:53:35PM +0200, Andrew Lunn wrote:
>> On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
>>> Hi,
>>>
>>> tinywrkb, please can you test this series to ensure that it fixes
>>> your problem - the previous version has turned out to be a non-starter
>>> as it introduces more problems, thanks!
>>>
>>> The following series attempts to address an issue spotted by tinywrkb
>>> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
>>> the negotiated link.
>>
>> Hi Russell
>>
>> This all looks sensible.
>>
>> One things we need to be careful of, is this is for net and so stable.
> 
> Since the regression was introduced in 5.1, it should be backported
> to stable trees.
> 
>> But only some of the patches have fixes-tags. I don't know if we
>> should add fixes tags to all the patches, just to give back porters a
>> hint that they are all needed? It won't compile without the patches,
>> so at least it fails safe.
> 
> I only put Fixes: tags on patches that are actually fixing something.
> Quoting submitting-patches.rst:
> 
>   A Fixes: tag indicates that the patch fixes an issue in a previous
>   commit.
> 
> Since the preceding two patches are just preparing for the fix, and
> not actually fixing an issue in themselves, it seems wrong to add a
> Fixes: tag for them.  However, mentioning it in the commit message
> for the patch that does fix the issue is probably worth it.  Thanks.
> 

This is not a criticism of your patch series, which is fine.

I believe Andrew's angle is that if you have fixes that rely on
non-functional changes, then the fixes cannot be back ported as a
standalone patch set towards specific stable trees. This means that
people who do care about such fixes may have to come up with a slightly
different fix for earlier kernels affected by those bugs, such fixes
would not rely on patch #2 and #3 in this series and open code
phy_resolve_aneg() and genphy_read_lpa() within the at803x.c PHY driver.
-- 
Florian

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[Russell King - ARM Linux admin]

On Sun, Sep 22, 2019 at 11:02:13AM -0700, Florian Fainelli wrote:
> On 9/22/2019 10:52 AM, Russell King - ARM Linux admin wrote:
> > On Sun, Sep 22, 2019 at 06:53:35PM +0200, Andrew Lunn wrote:
> >> On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> >>> Hi,
> >>>
> >>> tinywrkb, please can you test this series to ensure that it fixes
> >>> your problem - the previous version has turned out to be a non-starter
> >>> as it introduces more problems, thanks!
> >>>
> >>> The following series attempts to address an issue spotted by tinywrkb
> >>> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> >>> the negotiated link.
> >>
> >> Hi Russell
> >>
> >> This all looks sensible.
> >>
> >> One things we need to be careful of, is this is for net and so stable.
> > 
> > Since the regression was introduced in 5.1, it should be backported
> > to stable trees.
> > 
> >> But only some of the patches have fixes-tags. I don't know if we
> >> should add fixes tags to all the patches, just to give back porters a
> >> hint that they are all needed? It won't compile without the patches,
> >> so at least it fails safe.
> > 
> > I only put Fixes: tags on patches that are actually fixing something.
> > Quoting submitting-patches.rst:
> > 
> >   A Fixes: tag indicates that the patch fixes an issue in a previous
> >   commit.
> > 
> > Since the preceding two patches are just preparing for the fix, and
> > not actually fixing an issue in themselves, it seems wrong to add a
> > Fixes: tag for them.  However, mentioning it in the commit message
> > for the patch that does fix the issue is probably worth it.  Thanks.
> > 
> 
> This is not a criticism of your patch series, which is fine.
> 
> I believe Andrew's angle is that if you have fixes that rely on
> non-functional changes, then the fixes cannot be back ported as a
> standalone patch set towards specific stable trees.

Right, which makes it pointless adding a Fixes: tag to those changes.
The amount of change in phylib is quite high at the moment, but
thankfully the bug was introduced after the ethtool linkmode mask
conversion.  It looks like v5.2.x should be able to cope with all
three patches simply applied there.

However, v5.1.x will require a different pre-requisit patch, which
is probably easier to do as you suggest.

> This means that
> people who do care about such fixes may have to come up with a slightly
> different fix for earlier kernels affected by those bugs, such fixes
> would not rely on patch #2 and #3 in this series and open code
> phy_resolve_aneg() and genphy_read_lpa() within the at803x.c PHY driver.
> -- 
> Florian
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Sun, 22 Sep 2019 11:59:32 +0100

> tinywrkb, please can you test this series to ensure that it fixes
> your problem - the previous version has turned out to be a non-starter
> as it introduces more problems, thanks!
> 
> The following series attempts to address an issue spotted by tinywrkb
> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> the negotiated link.
 ...

This doesn't apply cleanly to the current 'net' tree, please respin.