Attempt to fix regression with AR8035 speed downgrade

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

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(-)

Signature · 3 lines

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

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Fixes 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")

Signed-off-by Russell King <[email protected]>

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.

Quoted 9 lines · 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.

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.

Quoted 11 lines · Andrew Lunn

Quoted 9 lines · 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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 29 lines · Russell King - ARM Linux admin

Quoted 11 lines · Andrew Lunn

Quoted 9 lines · 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.

Quoted 5 lines

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.

Quoted 4 lines

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:

Quoted 8 lines

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.

Signature · 1 line

Florian

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.