Re: [PATCH net-next 1/2] net: pcs: xpcs: remove double-read of link state when using AN

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Mar 16, 2023 at 10:44:48AM +0100, Steen Hegelund wrote:
> Hi Russell,
> 
> 
> On Wed, 2023-03-15 at 14:46 +0000, Russell King (Oracle) wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Phylink does not want the current state of the link when reading the
> > PCS link state - it wants the latched state. Don't double-read the
> > MII status register. Phylink will re-read as necessary to capture
> > transient link-down events as of dbae3388ea9c ("net: phylink: Force
> > retrigger in case of latched link-fail indicator").
> > 
> > The above referenced commit is a dependency for this change, and thus
> > this change should not be backported to any kernel that does not
> > contain the above referenced commit.
> > 
> > Fixes: fcb26bd2b6ca ("net: phy: Add Synopsys DesignWare XPCS MDIO module")
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/pcs/pcs-xpcs.c | 13 ++-----------
> >  1 file changed, 2 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index bc428a816719..04a685353041 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -321,7 +321,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs,
> >         return 0;
> >  }
> > 
> > -static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an)
> > +static int xpcs_read_link_c73(struct dw_xpcs *xpcs)
> >  {
> >         bool link = true;
> >         int ret;
> > @@ -333,15 +333,6 @@ static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an)
> >         if (!(ret & MDIO_STAT1_LSTATUS))
> >                 link = false;
> > 
> > -       if (an) {
> > -               ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1);
> > -               if (ret < 0)
> > -                       return ret;
> > -
> > -               if (!(ret & MDIO_STAT1_LSTATUS))
> > -                       link = false;
> > -       }
> > -
> >         return link;
> >  }
> > 
> > @@ -935,7 +926,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
> >         int ret;
> > 
> >         /* Link needs to be read first ... */
> > -       state->link = xpcs_read_link_c73(xpcs, state->an_enabled) > 0 ? 1 : 0;
> > +       state->link = xpcs_read_link_c73(xpcs) > 0 ? 1 : 0;
> 
> Couldn't you just say:
> 
> 	state->link = xpcs_read_link_c73(xpcs) > 0;
> 
> That should be a boolean, right?

That would be another change to the code - and consider how such a
change would fit in this series given its description and also this
patch.

IMHO such a change should be a separate patch - and there's plenty
of scope for cleanups like the one you suggest in this driver.

Thanks.

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