[PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

[PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

[Qingtao Cao]

On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
activated, the device assumes a link-up status with existing configuration
in BMCR, avoid bringing down the fibre link in this case

Test case:
1. Two 88E151x connected with SFP, both enable autoneg, link is up with
   speed 1000M
2. Disable autoneg on one device and explicitly set its speed to 1000M
3. The fibre link can still up with this change, otherwise not.

Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
---
 drivers/net/phy/marvell.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 9964bf3dea2f..efc4b2317466 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -195,6 +195,10 @@
 
 #define MII_88E1510_MSCR_2		0x15
 
+#define MII_88E1510_FSCR2		0x1a
+#define MII_88E1510_FSCR2_BYPASS_ENABLE	BIT(6)
+#define MII_88E1510_FSCR2_BYPASS_STATUS	BIT(5)
+
 #define MII_VCT5_TX_RX_MDI0_COUPLING	0x10
 #define MII_VCT5_TX_RX_MDI1_COUPLING	0x11
 #define MII_VCT5_TX_RX_MDI2_COUPLING	0x12
@@ -1623,11 +1627,21 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
 static int marvell_read_status_page_an(struct phy_device *phydev,
 				       int fiber, int status)
 {
+	int fscr2;
 	int lpa;
 	int err;
 
 	if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
 		phydev->link = 0;
+		if (fiber) {
+			fscr2 = phy_read(phydev, MII_88E1510_FSCR2);
+			if (fscr2 < 0)
+				return fscr2;
+			if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) &&
+			    (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS) &&
+			    (genphy_read_status_fixed(phydev) == 0))
+				phydev->link = 1;
+		}
 		return 0;
 	}
 
-- 
2.34.1

Re: [PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

[Mateusz Polchlopek]

On 10/3/2024 9:10 AM, Qingtao Cao wrote:
> On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
> activated, the device assumes a link-up status with existing configuration
> in BMCR, avoid bringing down the fibre link in this case
> 
> Test case:
> 1. Two 88E151x connected with SFP, both enable autoneg, link is up with
>     speed 1000M
> 2. Disable autoneg on one device and explicitly set its speed to 1000M
> 3. The fibre link can still up with this change, otherwise not.
> 
> Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
> ---
>   drivers/net/phy/marvell.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 9964bf3dea2f..efc4b2317466 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -195,6 +195,10 @@
>   
>   #define MII_88E1510_MSCR_2		0x15
>   
> +#define MII_88E1510_FSCR2		0x1a
> +#define MII_88E1510_FSCR2_BYPASS_ENABLE	BIT(6)
> +#define MII_88E1510_FSCR2_BYPASS_STATUS	BIT(5)
> +
>   #define MII_VCT5_TX_RX_MDI0_COUPLING	0x10
>   #define MII_VCT5_TX_RX_MDI1_COUPLING	0x11
>   #define MII_VCT5_TX_RX_MDI2_COUPLING	0x12
> @@ -1623,11 +1627,21 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
>   static int marvell_read_status_page_an(struct phy_device *phydev,
>   				       int fiber, int status)
>   {
> +	int fscr2;
>   	int lpa;
>   	int err;
>   
>   	if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
>   		phydev->link = 0;
> +		if (fiber) {
> +			fscr2 = phy_read(phydev, MII_88E1510_FSCR2);
> +			if (fscr2 < 0)
> +				return fscr2;
> +			if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) &&
> +			    (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS) &&
> +			    (genphy_read_status_fixed(phydev) == 0))
> +				phydev->link = 1;
> +		}
>   		return 0;
>   	}
>   

LGTM.

Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>

Re: [PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

[Russell King (Oracle)]

On Thu, Oct 03, 2024 at 05:10:50PM +1000, Qingtao Cao wrote:
> On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
> activated, the device assumes a link-up status with existing configuration
> in BMCR, avoid bringing down the fibre link in this case
> 
> Test case:
> 1. Two 88E151x connected with SFP, both enable autoneg, link is up with
>    speed 1000M
> 2. Disable autoneg on one device and explicitly set its speed to 1000M
> 3. The fibre link can still up with this change, otherwise not.

As you're clearly using fibre, there's this chunk of code in the same
function that is (IMHO) wrong:

                if (phydev->duplex == DUPLEX_FULL) {
                        if (!(lpa & LPA_PAUSE_FIBER)) {
                                phydev->pause = 0;
                                phydev->asym_pause = 0;
                        } else if ((lpa & LPA_PAUSE_ASYM_FIBER)) {
                                phydev->pause = 1;
                                phydev->asym_pause = 1;
                        } else {
                                phydev->pause = 1;
                                phydev->asym_pause = 0;
                        }
                }

as ->pause and ->asym_pause are supposed to be the _resolved_
state, and this is only looking at the link partner's state.

fiber_lpa_mod_linkmode_lpa_t() also uses the baseT linkmodes for
Fibre.

IMHO, this should be:

		mii_lpa_mod_linkmode_x(phydev->lp_advertising, lpa,
				ETHTOOL_LINK_MODE_1000baseX_Full_BIT);
		phy_resolve_aneg_pause(phydev);

Please can you test whether that works correctly in addition to
your other fix? Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

[Andrew Lunn]

On Thu, Oct 03, 2024 at 05:10:50PM +1000, Qingtao Cao wrote:
> On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
> activated, the device assumes a link-up status with existing configuration
> in BMCR, avoid bringing down the fibre link in this case

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Please note the bullet point:

* don’t repost your patches within one 24h period

It would be good if you read all this document.

	Andrew