[PATCH net-next 0/2] Minor fixes for pcs_get_state() implementations

[PATCH net-next 0/2] Minor fixes for pcs_get_state() implementations

[Russell King (Oracle)]

Hi,

This series contains a number fixes for minor issues with some
pcs_get_state() implementations, particualrly for the phylink
state->an_enabled member. As they are minor, I'm suggesting we
queue them in net-next as there is follow-on work for these, and
there is no urgency for them to be in -rc.

Just like phylib, state->advertising's Autoneg bit is a copy of
state->an_enabled, and thus it is my intention to remove
state->an_enabled from phylink to simplify things.

This series gets rid of state->an_enabled assignments or
reporting that should never have been there.

 drivers/net/pcs/pcs-lynx.c |  4 ++--
 drivers/net/pcs/pcs-xpcs.c | 13 ++-----------
 2 files changed, 4 insertions(+), 13 deletions(-)

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

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

[Russell King (Oracle)]

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;
 
 	/* ... and then we check the faults. */
 	ret = xpcs_read_fault_c73(xpcs, state);
-- 
2.30.2

[PATCH net-next 2/2] net: pcs: lynx: don't print an_enabled in pcs_get_state()

[Russell King (Oracle)]

an_enabled will be going away, and in any case, pcs_get_state() should
not be updating this member. Remove the print.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-lynx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 3903f3baba2b..622c3de3f3a8 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -112,11 +112,11 @@ static void lynx_pcs_get_state(struct phylink_pcs *pcs,
 	}
 
 	dev_dbg(&lynx->mdio->dev,
-		"mode=%s/%s/%s link=%u an_enabled=%u an_complete=%u\n",
+		"mode=%s/%s/%s link=%u an_complete=%u\n",
 		phy_modes(state->interface),
 		phy_speed_to_str(state->speed),
 		phy_duplex_to_str(state->duplex),
-		state->link, state->an_enabled, state->an_complete);
+		state->link, state->an_complete);
 }
 
 static int lynx_pcs_config_giga(struct mdio_device *pcs, unsigned int mode,
-- 
2.30.2

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

[Steen Hegelund]

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?

> 
>         /* ... and then we check the faults. */
>         ret = xpcs_read_fault_c73(xpcs, state);
> --
> 2.30.2
> 

BR
Steen

Re: [PATCH net-next 2/2] net: pcs: lynx: don't print an_enabled in pcs_get_state()

[Steen Hegelund]

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
> 
> an_enabled will be going away, and in any case, pcs_get_state() should
> not be updating this member. Remove the print.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-lynx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
> index 3903f3baba2b..622c3de3f3a8 100644
> --- a/drivers/net/pcs/pcs-lynx.c
> +++ b/drivers/net/pcs/pcs-lynx.c
> @@ -112,11 +112,11 @@ static void lynx_pcs_get_state(struct phylink_pcs *pcs,
>         }
> 
>         dev_dbg(&lynx->mdio->dev,
> -               "mode=%s/%s/%s link=%u an_enabled=%u an_complete=%u\n",
> +               "mode=%s/%s/%s link=%u an_complete=%u\n",
>                 phy_modes(state->interface),
>                 phy_speed_to_str(state->speed),
>                 phy_duplex_to_str(state->duplex),
> -               state->link, state->an_enabled, state->an_complete);
> +               state->link, state->an_complete);
>  }
> 
>  static int lynx_pcs_config_giga(struct mdio_device *pcs, unsigned int mode,
> --
> 2.30.2
> 

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>

BR
Steen

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

[Russell King (Oracle)]

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!

Re: [PATCH net-next 0/2] Minor fixes for pcs_get_state() implementations

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 15 Mar 2023 14:46:24 +0000 you wrote:
> Hi,
> 
> This series contains a number fixes for minor issues with some
> pcs_get_state() implementations, particualrly for the phylink
> state->an_enabled member. As they are minor, I'm suggesting we
> queue them in net-next as there is follow-on work for these, and
> there is no urgency for them to be in -rc.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: pcs: xpcs: remove double-read of link state when using AN
    https://git.kernel.org/netdev/net-next/c/ef63461caf42
  - [net-next,2/2] net: pcs: lynx: don't print an_enabled in pcs_get_state()
    https://git.kernel.org/netdev/net-next/c/ecec0ebbc638

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Steen Hegelund]

Hi Russell,

On Thu, 2023-03-16 at 16:12 +0000, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> 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.

OK - got that!


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

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>

BR
Steen