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

[Aryan Srivastava]

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