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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A9505C47422 for ; Mon, 29 Jan 2024 13:19:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=btSATsz/dSXLcgTxmq1NQFJQJkbGwuxiJgLrSBc2MhU=; b=odtEANb7YvGu1S bxYptA+S/fIMUjxDRgaeFcrLt5sE8srPcwrEA7z9kANY9LGKcKMHrx5IXCiROLNq4VhDGOXD24TmH 89ndZLmapqdVf9vcsNwh2DTrv4+dxTRK2B7yLKZjWMtXaDKTi89v2E7G7q1sUXa+XaGg0qlvhxIdj 7/qv3GGbJWQG2ErlS2SkjDKE3WhvTgTkK8x5HH9eBB1pSVQYJEgwIu7m0359f11ijBU1ZfVofJspT NoXXY/FMgIkmJZuIn5BPE8bJeV0ZXmNi3OpOs1NA6Rs5Hw4rRmxdKC/fc/8F41eXEmy/cPPYYoHam iSrABddgS0QBUE7UFqKg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rURXH-0000000CpGT-3AqD; Mon, 29 Jan 2024 13:18:51 +0000 Received: from mgamail.intel.com ([192.55.52.93]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rURXD-0000000CpFe-3VXR for linux-arm-kernel@lists.infradead.org; Mon, 29 Jan 2024 13:18:49 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1706534327; x=1738070327; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=JpFZYsNzV1BEiQaBqprQnIqEqg0c3XqDOiJt3LcQ2KU=; b=aKmA4peOILY/X5XC9dL826/Fu8lo61brUGqZxBiOBAWzN7nAqoObHntU pziwvZtWF2JWfa5fDIZI2esECZYTy5/NJShPO0GMN7TcZ53T+Bo7Xytkg axefPyEGDA4VcACwLapuFzLSxCidn/4S/rMM62lPdz0ZArDSWX+iF7Rj3 VFZ2syf2DMr1I8gCqa+yu90p5fMb46TVwRIUC2UNzru93KR3iJi32fRxm 9QKM+CHptmlo7gY1LqUe/Z7xivzoGO3OEdmOy5s5t1qJHXjfxiXYp2Y90 VQM0FSrddiJb9oc4Q5xH83TtjOG/Q6L97i+X46/hdtqHsiAgYShFvO/hz g==; X-IronPort-AV: E=McAfee;i="6600,9927,10967"; a="400092657" X-IronPort-AV: E=Sophos;i="6.05,227,1701158400"; d="scan'208";a="400092657" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jan 2024 05:18:45 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10967"; a="787828961" X-IronPort-AV: E=Sophos;i="6.05,227,1701158400"; d="scan'208";a="787828961" Received: from choongyo-mobl.gar.corp.intel.com (HELO [10.247.122.111]) ([10.247.122.111]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jan 2024 05:18:33 -0800 Message-ID: Date: Mon, 29 Jan 2024 21:18:31 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v3 3/5] net: phy: update in-band AN mode when changing interface by PHY driver Content-Language: en-US To: "Russell King (Oracle)" Cc: Rajneesh Bhardwaj , David E Box , Hans de Goede , Mark Gross , Jose Abreu , Andrew Lunn , Heiner Kallweit , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , =?UTF-8?Q?Marek_Beh=C3=BAn?= , Jean Delvare , Guenter Roeck , Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , Maxime Coquelin , Richard Cochran , Philipp Zabel , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , Wong Vee Khee , Jon Hunter , Jesse Brandeburg , Revanth Kumar Uppala , Shenwei Wang , Andrey Konovalov , Jochen Henneberg , David E Box , Andrew Halaney , Simon Horman , Bartosz Golaszewski , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, platform-driver-x86@vger.kernel.org, linux-hwmon@vger.kernel.org, bpf@vger.kernel.org, Voon Wei Feng , Tan Tee Min , Michael Sit Wei Hong , Lai Peter Jun Ann References: <20230921121946.3025771-1-yong.liang.choong@linux.intel.com> <20230921121946.3025771-4-yong.liang.choong@linux.intel.com> From: Choong Yong Liang In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240129_051847_954333_5833D860 X-CRM114-Status: GOOD ( 34.79 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 21/9/2023 10:04 pm, Russell King (Oracle) wrote: > On Thu, Sep 21, 2023 at 08:19:44PM +0800, Choong Yong Liang wrote: >> As there is a mechanism in PHY drivers to switch the PHY interface >> between SGMII and 2500BaseX according to link speed. In this case, >> the in-band AN mode should be switching based on the PHY interface >> as well, if the PHY interface has been changed/updated by PHY driver. >> >> For e.g., disable in-band AN in 2500BaseX mode, or enable in-band AN >> back for SGMII mode (10/100/1000Mbps). >> >> Signed-off-by: Choong Yong Liang > > This approach is *going* to break existing setups, sorry. > >> +/** >> + * phylink_interface_change() - update both cfg_link_an_mode and >> + * cur_link_an_mode when there is a change in the interface. >> + * @phydev: pointer to &struct phy_device >> + * >> + * When the PHY interface switches between SGMII and 2500BaseX in >> + * accordance with the link speed, the in-band AN mode should also switch >> + * based on the PHY interface >> + */ >> +static void phylink_interface_change(struct phy_device *phydev) >> +{ >> + struct phylink *pl = phydev->phylink; >> + >> + if (pl->phy_state.interface != phydev->interface) { >> + /* Fallback to the correct AN mode. */ >> + if (phy_interface_mode_is_8023z(phydev->interface) && >> + pl->cfg_link_an_mode == MLO_AN_INBAND) { >> + pl->cfg_link_an_mode = MLO_AN_PHY; >> + pl->cur_link_an_mode = MLO_AN_PHY; > > 1. Why are you changing both cfg_link_an_mode (configured link AN mode) > and cur_link_an_mode (current link AN mode) ? > > The "configured" link AN mode is supposed to be whatever was configured > at phylink creation time, and it's never supposed to change. The > "current" link AN mode can change, but changing that must be followed > by a major reconfiguration to ensure everything is correctly setup. > That will happen only because the change to the current link AN mode > can only happen when pl->phy_state.interface has changed, and the > change of pl->phy_state.interface triggers the reconfiguration. > During the phylink_create, the cfg_link_an_mode will be set to MLO_AN_INBAND. Then we switch from the SGMII interface to 2500BASEX interface. When we perform 'ifconfig eth0 down' and 'ifconfig eth0 up' then we will not able to bring up the PHY due to the phylink_attach_phy function. It is not expect to have MLO_AN_INBAND with PHY_INTERFACE_MODE_2500BASEX interface. static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy, phy_interface_t interface) { if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED || (pl->cfg_link_an_mode == MLO_AN_INBAND && phy_interface_mode_is_8023z(interface) && !pl->sfp_bus))) return -EINVAL; if (pl->phydev) return -EBUSY; return phy_attach_direct(pl->netdev, phy, 0, interface); } I did change different ways to handle it in the new patch series. So it should not impact much on the existing phylink framework. > 2. You force this behaviour on everyone, so now everyone with a SFP > module that operates in 802.3z mode will be switched out of inband mode > whether they want that or not. This is likely to cause some breakage. > >> + } else if (pl->config->ovr_an_inband) { >> + pl->cfg_link_an_mode = MLO_AN_INBAND; >> + pl->cur_link_an_mode = MLO_AN_INBAND; > > Here you force inband when not 802.3z mode and ovr_an_inband is set. > There are SFP modules that do *not* support in-band at all, and this > will break these modules when combined with a driver that sets > ovr_an_inband. So more breakage. > > Please enumerate the PHY interface modes that you are trying to support > with this patch set, and indicate whether you want in-band for that > mode or not, and where the restriction for whether in-band can be used > comes from (PHY, PCS or MAC) so that it's possible to better understand > what you're trying to achieve. > > Thanks. > Thank you for pointing out the bug that will impact everyone. Actually cur_link_an_mode is just required for PCS, I did handle it that only intel platforms will get the PCS negotiation mode for the PCS. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel