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=-11.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 5FAE9C433DF for ; Mon, 20 Jul 2020 12:44:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 356122070A for ; Mon, 20 Jul 2020 12:44:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=armlinux.org.uk header.i=@armlinux.org.uk header.b="bXkrKoEn" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728726AbgGTMoW (ORCPT ); Mon, 20 Jul 2020 08:44:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39908 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728703AbgGTMoV (ORCPT ); Mon, 20 Jul 2020 08:44:21 -0400 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:32c8:5054:ff:fe00:142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6E934C061794 for ; Mon, 20 Jul 2020 05:44:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=nFm6ycez44+yf6dAAUQGj5hstg993ZMRPufu+SSTxKA=; b=bXkrKoEnatIOHu2CdT7ia/FXB iYgEl/+0wzSvC9z+ZUmMhySe/b4sARgVPFXdOgSXvRxf9rx1ls/4DlqA/vCLU7Oa8lcyHhsy/hCbe xPdkQXzIlUE8Aps2cdEi58CzA5A6Zi5c8zHx/0Lirhfe2xXFGiVOzGvV0TRZ/B89m/CnJFdbkuyZF 0LhRlf8cDzkCoZzwlRDdZOWKqiBzyIeuyGcDiLUHPYSsC82Hdj/cSM5K7qjIAWdB0M7QV9akOi+0+ OmdezDjrY/RF6f5/9npuaAiE2LSZCjjDHSi6cfj5y8vfdCDQW89W6KONVP6hc1xTYaz9FsOcTSvwR 4YNdaK8MQ==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:41894) by pandora.armlinux.org.uk with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jxV9N-00031k-4r; Mon, 20 Jul 2020 13:44:09 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.92) (envelope-from ) id 1jxV9J-0004zK-F0; Mon, 20 Jul 2020 13:44:05 +0100 Date: Mon, 20 Jul 2020 13:44:05 +0100 From: Russell King - ARM Linux admin To: Ioana Ciornei Cc: Andrew Lunn , Florian Fainelli , Heiner Kallweit , Vladimir Oltean , Claudiu Manoil , Alexandru Marginean , "michael@walle.cc" , "olteanv@gmail.com" , "David S. Miller" , Jakub Kicinski , "netdev@vger.kernel.org" Subject: Re: [PATCH RFC net-next 11/13] net: phylink: re-implement interface configuration with PCS Message-ID: <20200720124405.GT1551@shell.armlinux.org.uk> References: <20200630142754.GC1551@shell.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Jul 20, 2020 at 11:40:44AM +0000, Ioana Ciornei wrote: > On 6/30/20 5:29 PM, Russell King wrote: > > With PCS support, how we implement interface reconfiguration is not up > > to the job; we end up reconfiguring the PCS for an interface change > > while the link could potentially be up. In order to solve this, add > > two additional MAC methods for interface configuration, one to prepare > > for the change, and one to finish the change. > > > > This allows mvneta and mvpp2 to shutdown what they require prior to the > > MAC and PCS configuration calls, and then restart as appropriate. > > > > This impacts ksettings_set(), which now needs to identify whether the > > change is a minor tweak to the advertisement masks or whether the > > interface mode has changed, and call the appropriate function for that > > update. > > > > Signed-off-by: Russell King > > --- > > drivers/net/phy/phylink.c | 80 ++++++++++++++++++++++++++------------- > > include/linux/phylink.h | 48 +++++++++++++++++++++++ > > 2 files changed, 102 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index 09f4aeef15c7..a31a00fb4974 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -433,23 +433,47 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl) > > } > > } > > > > -static void phylink_pcs_config(struct phylink *pl, bool force_restart, > > - const struct phylink_link_state *state) > > +static void phylink_change_interface(struct phylink *pl, bool restart, > > + const struct phylink_link_state *state) > > { > > - bool restart = force_restart; > > + int err; > > + > > + phylink_dbg(pl, "change interface %s\n", phy_modes(state->interface)); > > > > - if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config, > > - pl->cur_link_an_mode, > > - state->interface, > > - state->advertising, > > - !!(pl->link_config.pause & > > - MLO_PAUSE_AN))) > > - restart = true; > > + if (pl->mac_ops->mac_prepare) { > > + err = pl->mac_ops->mac_prepare(pl->config, pl->cur_link_an_mode, > > + state->interface); > > + if (err < 0) { > > + phylink_err(pl, "mac_prepare failed: %pe\n", > > + ERR_PTR(err)); > > + return; > > + } > > + } > > > > phylink_mac_config(pl, state); > > > > + if (pl->pcs_ops) { > > + err = pl->pcs_ops->pcs_config(pl->config, pl->cur_link_an_mode, > > + state->interface, > > + state->advertising, > > + !!(pl->link_config.pause & > > + MLO_PAUSE_AN)); > > + if (err < 0) > > + phylink_err(pl, "pcs_config failed: %pe\n", > > + ERR_PTR(err)); > > + if (err > 0) > > + restart = true; > > + } > > if (restart) > > phylink_mac_pcs_an_restart(pl); > > + > > + if (pl->mac_ops->mac_finish) { > > + err = pl->mac_ops->mac_finish(pl->config, pl->cur_link_an_mode, > > + state->interface); > > + if (err < 0) > > + phylink_err(pl, "mac_prepare failed: %pe\n", > > + ERR_PTR(err)); > > + } > > } > > > > /* > > @@ -555,7 +579,7 @@ static void phylink_mac_initial_config(struct phylink *pl, bool force_restart) > > link_state.link = false; > > > > phylink_apply_manual_flow(pl, &link_state); > > - phylink_pcs_config(pl, force_restart, &link_state); > > + phylink_change_interface(pl, force_restart, &link_state); > > } > > > > static const char *phylink_pause_to_str(int pause) > > @@ -674,7 +698,7 @@ static void phylink_resolve(struct work_struct *w) > > phylink_link_down(pl); > > cur_link_state = false; > > } > > - phylink_pcs_config(pl, false, &link_state); > > + phylink_change_interface(pl, false, &link_state); > > pl->link_config.interface = link_state.interface; > > } else if (!pl->pcs_ops) { > > /* The interface remains unchanged, only the speed, > > @@ -1450,22 +1474,26 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, > > return -EINVAL; > > > > mutex_lock(&pl->state_mutex); > > - linkmode_copy(pl->link_config.advertising, config.advertising); > > - pl->link_config.interface = config.interface; > > pl->link_config.speed = config.speed; > > pl->link_config.duplex = config.duplex; > > - pl->link_config.an_enabled = kset->base.autoneg != > > - AUTONEG_DISABLE; > > - > > - if (pl->cur_link_an_mode == MLO_AN_INBAND && > > - !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) { > > - /* If in 802.3z mode, this updates the advertisement. > > - * > > - * If we are in SGMII mode without a PHY, there is no > > - * advertisement; the only thing we have is the pause > > - * modes which can only come from a PHY. > > - */ > > - phylink_pcs_config(pl, true, &pl->link_config); > > + pl->link_config.an_enabled = kset->base.autoneg != AUTONEG_DISABLE; > > + > > + if (pl->link_config.interface != config.interface) { > > > I cannot seem to understand where in this function config.interface > could become different from pl->link_config.interface. > > Is there something obvious that I am missing? The validate() method is free to change the interface if required - there's a helper that MACs can use to achieve that for switching between 1000base-X and 2500base-X. Useful if you have a FC SFP plugged in capable of 2500base-X, but want to communicate with a 1000base-X link partner. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!