On Thu, Jan 17, 2019 at 03:29:35AM +0100, Andrew Lunn wrote:
> On Mon, Jan 14, 2019 at 01:34:44PM +0200, Samu Nuutamo wrote:
> > Hi,
> >
> > We have an imx6q-b450v3 board that has one of the switch ports configured
> > as a
> > fixed link. After upgrading the kernel to version 4.18 the link has
> > experienced
> > a small amount of packet loss, around 0.15%.
> >
> > The issue was bisected to a commit: aab9c4067d23 net: dsa: Plug in PHYLINK
> > support
> >
> > After enabling debug we could see that the new phylink code causes the link
> > to
> > reset once every second:
> >
> > [ 309.992368] mv88e6085 gpio-0:00 enembc: phylink_mac_config:
> > mode=fixed//100Mbps/Full adv=00000,00000208 pause=10 link=1 an=1
> > [ 309.998451] mv88e6085 gpio-0:00: p5: Force link down
> > [ 309.998869] mv88e6085 gpio-0:00: p5: Speed set to 100 Mbps
> > [ 309.999280] mv88e6085 gpio-0:00: p5: Force full duplex
> > [ 309.999895] mv88e6085 gpio-0:00: p5: Force link up
> > [ 311.032400] mv88e6085 gpio-0:00 enembc: phylink_mac_config:
> > mode=fixed//100Mbps/Full adv=00000,00000208 pause=10 link=1 an=1
> > [ 311.038248] mv88e6085 gpio-0:00: p5: Force link down
> > [ 311.038660] mv88e6085 gpio-0:00: p5: Speed set to 100 Mbps
> > [ 311.039069] mv88e6085 gpio-0:00: p5: Force full duplex
> > [ 311.039678] mv88e6085 gpio-0:00: p5: Force link up
> > [ 312.072328] mv88e6085 gpio-0:00 enembc: phylink_mac_config:
> > mode=fixed//100Mbps/Full adv=00000,00000208 pause=10 link=1 an=1
> > [ 312.077603] mv88e6085 gpio-0:00: p5: Force link down
> > [ 312.078010] mv88e6085 gpio-0:00: p5: Speed set to 100 Mbps
> > [ 312.078417] mv88e6085 gpio-0:00: p5: Force full duplex
> > [ 312.079026] mv88e6085 gpio-0:00: p5: Force link up
>
> 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 tested the patch and it has an issue that prevents the carrier from turning
on. I think it's caused by the change from using link_state to phy->phy_state,
as later in the same function the link_state attributes are read when
deciding if the carrier needs to be turned on.
- Samu
