1. Patchew
  2. linux
  3. View series

[PATCH net-next v0] net: phy: aquantia: poll status register

Aryan Srivastava posted 1 patch 1 year, 2 months ago
There is a newer version of this series
drivers/net/phy/aquantia/aquantia_main.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
[PATCH net-next v0] net: phy: aquantia: poll status register
Posted by Aryan Srivastava 1 year, 2 months ago 
The system interface connection status register is not immediately
correct upon line side link up. This results in the status being read as
OFF and then transitioning to the correct host side link mode with a
short delay. This results in the phylink framework passing the OFF
status down to all MAC config drivers, resulting in the host side link
being misconfigured, which in turn can lead to link flapping or complete
packet loss in some cases.

Mitigate this by periodically polling the register until it not showing
the OFF state. This will be done every 1ms for 10ms, using the same
poll/timeout as the processor intensive operation reads.

Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
---
 drivers/net/phy/aquantia/aquantia_main.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e982e9ce44a5..d2573982b1e5 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -41,6 +41,7 @@
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI	4
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII	6
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI	7
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF	9
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII	10
 
 #define MDIO_AN_VEND_PROV			0xc400
@@ -342,9 +343,18 @@ static int aqr107_read_status(struct phy_device *phydev)
 	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
 		return 0;
 
-	val = phy_read_mmd(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS);
-	if (val < 0)
-		return val;
+	/**
+	 * The status register is not immediately correct on line side link up.
+	 * Poll periodically until it reflects the correct ON state.
+	 */
+	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
+					MDIO_PHYXS_VEND_IF_STATUS, val,
+					(FIELD_GET(MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK, val) !=
+					MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF),
+					AQR107_OP_IN_PROG_SLEEP,
+					AQR107_OP_IN_PROG_TIMEOUT, false);
+	if (ret)
+		return ret;
 
 	switch (FIELD_GET(MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK, val)) {
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR:
-- 
2.46.0
Re: [PATCH net-next v0] net: phy: aquantia: poll status register
Posted by Andrew Lunn 1 year, 2 months ago 
On Mon, Oct 07, 2024 at 10:35:36AM +1300, Aryan Srivastava wrote:
> The system interface connection status register is not immediately
> correct upon line side link up. This results in the status being read as
> OFF and then transitioning to the correct host side link mode with a
> short delay. This results in the phylink framework passing the OFF
> status down to all MAC config drivers, resulting in the host side link
> being misconfigured, which in turn can lead to link flapping or complete
> packet loss in some cases.
> 
> Mitigate this by periodically polling the register until it not showing
> the OFF state. This will be done every 1ms for 10ms, using the same
> poll/timeout as the processor intensive operation reads.

Does the datasheet say anything about when MDIO_PHYXS_VEND_IF_STATUS
is valid?

>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI	4
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII	6
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI	7
> +#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF	9
>  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII	10
>  
>  #define MDIO_AN_VEND_PROV			0xc400
> @@ -342,9 +343,18 @@ static int aqr107_read_status(struct phy_device *phydev)
>  	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
>  		return 0;
>  
> -	val = phy_read_mmd(phydev, MDIO_MMD_PHYXS, MDIO_PHYXS_VEND_IF_STATUS);
> -	if (val < 0)
> -		return val;
> +	/**
> +	 * The status register is not immediately correct on line side link up.
> +	 * Poll periodically until it reflects the correct ON state.
> +	 */
> +	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> +					MDIO_PHYXS_VEND_IF_STATUS, val,
> +					(FIELD_GET(MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK, val) !=
> +					MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF),
> +					AQR107_OP_IN_PROG_SLEEP,
> +					AQR107_OP_IN_PROG_TIMEOUT, false);
> +	if (ret)
> +		return ret;

I don't know if returning ETIMEDOUT is the correct thing to do
here. It might be better to set phydev->link to false, since there is
no end to end link yet.

	Andrew
Re: [PATCH net-next v0] net: phy: aquantia: poll status register
Posted by Aryan Srivastava 1 year, 2 months ago 
On Tue, 2024-10-08 at 23:40 +0200, Andrew Lunn wrote:
> On Mon, Oct 07, 2024 at 10:35:36AM +1300, Aryan Srivastava wrote:
> > The system interface connection status register is not immediately
> > correct upon line side link up. This results in the status being
> > read as
> > OFF and then transitioning to the correct host side link mode with
> > a
> > short delay. This results in the phylink framework passing the OFF
> > status down to all MAC config drivers, resulting in the host side
> > link
> > being misconfigured, which in turn can lead to link flapping or
> > complete
> > packet loss in some cases.
> > 
> > Mitigate this by periodically polling the register until it not
> > showing
> > the OFF state. This will be done every 1ms for 10ms, using the same
> > poll/timeout as the processor intensive operation reads.
> 
> Does the datasheet say anything about when MDIO_PHYXS_VEND_IF_STATUS
> is valid?
> 
The datasheet is quite brief about the registers. There is basic
description, but not much towards any nuances they might have,
unfortunately.
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI    4
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII   6
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI   7
> > +#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OFF     9
> >  #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII 10
> >  
> >  #define MDIO_AN_VEND_PROV                      0xc400
> > @@ -342,9 +343,18 @@ static int aqr107_read_status(struct
> > phy_device *phydev)
> >         if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
> >                 return 0;
> >  
> > -       val = phy_read_mmd(phydev, MDIO_MMD_PHYXS,
> > MDIO_PHYXS_VEND_IF_STATUS);
> > -       if (val < 0)
> > -               return val;
> > +       /**
> > +        * The status register is not immediately correct on line
> > side link up.
> > +        * Poll periodically until it reflects the correct ON
> > state.
> > +        */
> > +       ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PHYXS,
> > +                                       MDIO_PHYXS_VEND_IF_STATUS,
> > val,
> > +                                       (FIELD_GET(MDIO_PHYXS_VEND_
> > IF_STATUS_TYPE_MASK, val) !=
> > +                                       MDIO_PHYXS_VEND_IF_STATUS_T
> > YPE_OFF),
> > +                                       AQR107_OP_IN_PROG_SLEEP,
> > +                                       AQR107_OP_IN_PROG_TIMEOUT,
> > false);
> > +       if (ret)
> > +               return ret;
> 
> I don't know if returning ETIMEDOUT is the correct thing to do
> here. It might be better to set phydev->link to false, since there is
> no end to end link yet.
Yes I agree, will change this to use 'val' regardless of the return,
and let the switch/case deal with the OFF status as required.
> 
>         Andrew
Cheers,
Aryan

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.