On Thu, Jan 17, 2019 at 03:29:35AM +0100, Andrew Lunn wrote:
> Hi Samu
>
> Please could you try this patch. I've not had chance to properly test
> it, and i'm about to go away for a long weekend.
>
> Thanks
> Andrew
>
> From 02438712824d3c7a9dd1aab8bd5dfb4383e55bfd Mon Sep 17 00:00:00 2001
> From: Andrew Lunn <and...@lunn.ch>
> Date: Wed, 16 Jan 2019 20:22:04 -0600
> Subject: [PATCH] net: phy: phylink: Only change mac when fixed link changes
> state
>
> phylink polls the fixed-link once per second to see if the GPIO has
> changed state, or if the callback indicates if there has been a change
> in state. It then calls the MAC to reconfigure itself to the current
> state.
>
> For some MACs, reconfiguration can result in packets being dropped.
> Hence keep track of the link state between polls and only reconfigure
> the MAC if there has been a change of state.
>
> Reported-by: Samu Nuutamo <samu.nuut...@vincit.fi>
> Fixes: 9cd00a8aa42e ("net: phy: phylink: Poll link GPIOs")
> Signed-off-by: Andrew Lunn <and...@lunn.ch>
> ---
> drivers/net/phy/phylink.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e7becc7379d7..6473af228842 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -407,11 +407,14 @@ static void phylink_resolve(struct work_struct *w)
> phylink_mac_config(pl, &link_state);
> break;
>
> - case MLO_AN_FIXED:
> - phylink_get_fixed_state(pl, &link_state);
> - phylink_mac_config(pl, &link_state);
> - break;
> + case MLO_AN_FIXED: {
> + bool old_link = pl->phy_state.link;
>
> + phylink_get_fixed_state(pl, &pl->phy_state);
> + if (pl->phy_state.link != old_link)
> + phylink_mac_config(pl, &pl->phy_state);
> + break;
> + }
> case MLO_AN_INBAND:
> phylink_get_mac_state(pl, &link_state);
> if (pl->phydev) {
> --
> 2.20.1
>
Hi Andrew,
I've debugged the issue further by dumping all the values inside
phylink_resolve to get a better understand how the link state behaves.
As this is a fixed link the link_state structure is populated by
phylink_get_fixed_state(), but in our case neither get_fixed_state callback
or GPIO is used. This means the link_state comes straight from the
link_config, meaning link_state.link will always be 1. On the other hand the
pl->phy_state seems to never change and the phy_state.link stays 0 even
when the link is up and functional.
This makes it impossible to use these variables for deciding if
phylink_mac_config needs to be run, and this got me thinking: do we even need
to reconfigure the link? The phylink_mac_config() is already called from
phylink_start and fixed links shouldn't change(?).
I created the following patch that simply removes the phylink_mac_config() call
and everything seems to be working. The patch was only tested with the board
we have on hand.
- Samu
>From 6786cc3b9fef40adb3f68c72236220fafaa26eee Mon Sep 17 00:00:00 2001
From: Samu Nuutamo <samu.nuut...@vincit.fi>
Date: Thu, 17 Jan 2019 16:42:29 +0200
Subject: [PATCH] net: phy: phylink: Configure fixed links only once
This prevents unnecessary link restarts from causing packet loss.
Fixes: 9cd00a8aa42e ("net: phy: phylink: Poll link GPIOs")
Signed-off-by: Samu Nuutamo <samu.nuut...@vincit.fi>
---
drivers/net/phy/phylink.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 70f3f90c2ed6..1ef76382c593 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -437,7 +437,6 @@ static void phylink_resolve(struct work_struct *w)
case MLO_AN_FIXED:
phylink_get_fixed_state(pl, &link_state);
- phylink_mac_config(pl, &link_state);
break;
case MLO_AN_INBAND:
--
2.17.1
