From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3522BC43387 for ; Thu, 17 Jan 2019 15:46:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0118020851 for ; Thu, 17 Jan 2019 15:46:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=vincit.fi header.i=@vincit.fi header.b="Z2Ofy4Xh" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727872AbfAQPqK (ORCPT ); Thu, 17 Jan 2019 10:46:10 -0500 Received: from mail-lf1-f67.google.com ([209.85.167.67]:35374 "EHLO mail-lf1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725926AbfAQPqJ (ORCPT ); Thu, 17 Jan 2019 10:46:09 -0500 Received: by mail-lf1-f67.google.com with SMTP id e26so8164909lfc.2 for ; Thu, 17 Jan 2019 07:46:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vincit.fi; s=ticniv; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=lGnwDPEMBQDEBEk4H7svq1ve65j0kJeWy0ncrl1/YX8=; b=Z2Ofy4XhpLii91R6iUW75vc05TyZli96EPLgJ2EFxjrKsxJc8s0KUVu0Q2inPA1g6r 07fbutfaNaaosPg1vsJHFtYv391xqYCYYh13ckr+adb1bZ3hPLq4Jq4t99yuBhxRjWEZ qfbmYbKlGtNoFgX9PGnCm2BTszdB4tUBH/P4s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=lGnwDPEMBQDEBEk4H7svq1ve65j0kJeWy0ncrl1/YX8=; b=mIkicVwuRl6kZf0kSUru+aG2XQrhv/bYG4Yrr6uGEWXWMJUQ+k8groyJATjNLw31mB leN0WjyDNCeET4lM3BYRMkHlkcGvNhSq057Udl228FDA37V0WZTVXsx4M4NiGD4N9luX s5JpqTcOsT+Agv43ZdfovkJ53P/5N9NX0x8/ZW9ozEDFbqHF7oI9cyEg7XxhlM1JF3ir 7b56stwmkuS1eGTBpxwVsodDnjib/ZZ3bmmd5mz3Clw2xc+8LilmUFBDf2aFdoLjWfnK KpGE4jUWprSiBaZpJKeNH95tT9qEW7bC9dUJG1/7rOvQe96vW6Z0BPdIMzU/c4nDXzyj E4UA== X-Gm-Message-State: AJcUukdikXlPeHWA6z6Xpqi0PPP7uxMfkl1YwaM3aBiyFT8oIw2m6INA /p+sBbhvI0NJqJqyshF3VbiG4Q== X-Google-Smtp-Source: ALg8bN7+NeXoim9JKb6OgsTtnkm5Ay5A5ggyxXxIGB26PPE+jBOB+47MZ0RngCKliFHi1nlqTDXCQg== X-Received: by 2002:a19:4ace:: with SMTP id x197mr10279897lfa.39.1547739967255; Thu, 17 Jan 2019 07:46:07 -0800 (PST) Received: from samu-ThinkPad-T480s (gw.vincit.cloud. [195.60.252.249]) by smtp.gmail.com with ESMTPSA id g15sm347283lfb.1.2019.01.17.07.46.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 17 Jan 2019 07:46:06 -0800 (PST) Date: Thu, 17 Jan 2019 17:46:04 +0200 From: Samu Nuutamo To: Andrew Lunn Cc: netdev@vger.kernel.org, Vivien Didelot , Florian Fainelli , "David S. Miller" Subject: Re: Regression: mv88e6xxx packet loss after 4.18's PHYLINK merge Message-ID: <20190117154604.GA15435@samu-ThinkPad-T480s> References: <20190114113444.GB16632@samu-ThinkPad-T480s> <20190117022935.GJ24870@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190117022935.GJ24870@lunn.ch> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > 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 > Fixes: 9cd00a8aa42e ("net: phy: phylink: Poll link GPIOs") > Signed-off-by: Andrew Lunn > --- > 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 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 --- 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