From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B927C3C4548 for ; Mon, 29 Jun 2026 07:18:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782717492; cv=none; b=cf5EyEq9udVOoO49llwgAZAtqiCaq3sW1yBZ72rPUYHS7tu0GV/G1se1w7pEnqeK0DvA8JmkZknB73silu6ZpBIjSWxCWAOJlyDrvwY/nn1RH9QWBT56bdjEhqN02yfLsTuX5MLeLhP758QopkCcZnpAjmTIIfwZOoZU7b2XUpA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782717492; c=relaxed/simple; bh=blN7kUYjZ29Lu9JrwtLJvpjU1Wy8lfqujnHxweqK+8c=; h=Message-ID:Date:From:To:Cc:Subject:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=MyagHCB/h5ZeF/AFb3agkak2z/MeNNUW/WBHFLUo0JHZJv8XPxUi28SAHAP32XQj7zi/S15a8s2QD1cawOGdVSJEUYmJr5N0jKlcnZm24RjwJlggYWb4vvIzA16fl8tf88oSCJSiD81NOFXnspjDC23kVOBXF3zuC1UMmlZHUcc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=UoKebr6L; arc=none smtp.client-ip=209.85.221.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="UoKebr6L" Received: by mail-wr1-f42.google.com with SMTP id ffacd0b85a97d-472326ca506so458360f8f.2 for ; Mon, 29 Jun 2026 00:18:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782717487; x=1783322287; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=BovyFstc80MYHmYyIZVG50kULX+1YfHlAuIHPTiI6jk=; b=UoKebr6L7g61S2ib1/fbHXwppvTCC1ASPVOG2gTWyXjTEcnJTr1ZZ1tFbyCkBNX28I ay9ZQqGMkmaKqeiUNL1Bpy+ituRDCsTu/jrTTful/YsQ2V4upAJBk1RlUzHnFE9F7iSy Js1x1pW7T8syZXAMpb57x+17cn2zG3kTfBddZIGdcHwvZHOrz7jgTy3bLZSPOWXhcwaN 8zc6BJLiedrnyhgzyP+TwCMskBECBxC/d3Dw7oYdqOCuPPd1d84yvCVWdq6Lx6En/Rgt knW2V5g7iTNkQhlBWIoSQbBRyuezOLGGLv/URcDeJtqZD8CfouiGkrVZPu4z0tPrf6Ys VK0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782717487; x=1783322287; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BovyFstc80MYHmYyIZVG50kULX+1YfHlAuIHPTiI6jk=; b=eSxNuktxNYUCCjeTecLQGYzFkD+ON/RaI2GtSg/f5k/zzLqsJo6/PtR1HNbkMaTtL8 kcCG3CEW0K68++aTCkm4sLDlDmcKewNixefRM1XsBCUN6zX3qRRX+qGSn8QB/M/B1iB2 lpw44OiOkwfqZe5Y4E9Dp5LUD/V4Ih9Rn0YJZCVVbAcvRpcaJWT9ChGJuv1bhrh3Icdf HkRqeGzBW/svJD5nRBiWttFgNg54CeK6Lpp3Mp/Gw2ce/cGwCxhj0Cbs5h3z+SyJKe0u W3lJNYSQqAxd7otUDs/gtnow9J4QkQUFYtmx5TFovcatgHuCOfJUM7PXqWfWVio+Ms2R y2NA== X-Forwarded-Encrypted: i=1; AHgh+Rpr4D0PhnDUFBWmXIQoYgE0J3JYw61PjI4bW3V/zak9rb3viwejT2ttflnNxUCeBKbb1rlfOw+DS3uA9lc=@vger.kernel.org X-Gm-Message-State: AOJu0Yw8HrjPqSxzd4XDCfHQet0yFyeh2O2gP9TZ34HT7pSIE6gkEzt5 Q+K3erDy4T2aFGDynF1Okki6ry+LKw2iC0efp59uxERhDs/3AqwpHymQ X-Gm-Gg: AfdE7ck1oKLNxnktZ4EAPIzT8fKDanQnOjIMk+HccgX+rjzWLxLpw3hG2R/CqfLpk78 606+NIJV8+8LAAc5sEWJFHZA2dtAPb3FmMBWYGJ7ny7rQLxv8GTdwuxvVOHPBwjeziad6BItnS+ ebU+qVR1iGJzG/yOaN1/oFQcrqh+0M3tJpE9ndywnDjzVCRR0KVmnF4bgdHThzEj7RwUYv/vB06 EAOHsyUVl3kT4JxkgswRCHBiKyB7T2XOCpUQtwGPMIT7KJu1jl3DSFsbucAh6L5wVg4psOv5oLN RPpyJESsPAE7+m2/rwyVFhFGz9ctD3CLTjiV+2JT9XzrCZEmsnXsReDP0B74i8uldN3EYW3cc2Z W42dkUan6CQi4uxn4672PmrCqAhVhRK5Fe91sFdm/DAJItSt2E0KLwunS0MWsb77E3lBCaW/noP 7ayL+bzbi8Z8g18HypB+6IwJFyrFL6BBPXgsCyFkxFs/u5P1y4fyX+nLu7fPbb2WxzUe0= X-Received: by 2002:a5d:6a48:0:b0:46d:32:3340 with SMTP id ffacd0b85a97d-46fb8d93892mr12284633f8f.36.1782717486707; Mon, 29 Jun 2026 00:18:06 -0700 (PDT) Received: from Ansuel-XPS. (host-80-183-219-152.pool80183.interbusiness.it. [80.183.219.152]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47290ed4377sm13297042f8f.37.2026.06.29.00.18.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Jun 2026 00:18:05 -0700 (PDT) Message-ID: <6a421c2d.1e0bfc4f.b2eb6.06fb@mx.google.com> X-Google-Original-Message-ID: Date: Mon, 29 Jun 2026 09:18:01 +0200 From: Christian Marangi To: Maxime Chevallier Cc: Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Simon Horman , Jonathan Corbet , Shuah Khan , Lorenzo Bianconi , Heiner Kallweit , Russell King , Saravana Kannan , Philipp Zabel , Nathan Chancellor , Nick Desaulniers , Bill Wendling , Justin Stitt , netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, llvm@lists.linux.dev Subject: Re: [RFC PATCH net-next v8 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS References: <20260618125752.1223-1-ansuelsmth@gmail.com> <20260618125752.1223-4-ansuelsmth@gmail.com> <6a3fc312.6161eb1e.3441bb.c0de@mx.google.com> <178defc6-8e60-4b0b-b3b0-f0f2a4003b68@bootlin.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <178defc6-8e60-4b0b-b3b0-f0f2a4003b68@bootlin.com> On Mon, Jun 29, 2026 at 09:04:49AM +0200, Maxime Chevallier wrote: > Hi Christian, > > On 6/27/26 14:33, Christian Marangi wrote: > > On Thu, Jun 25, 2026 at 04:13:14PM +0200, Maxime Chevallier wrote: > >> Hello Christian, > >> > >> On 6/18/26 14:57, Christian Marangi wrote: > >>> Add phylink_release_pcs() to externally release a PCS from a phylink > >>> instance. This can be used to handle case when a single PCS needs to be > >>> removed and the phylink instance needs to be refreshed. > >>> > >>> On calling phylink_release_pcs(), the PCS will be removed from the > >>> phylink internal PCS list and the phylink supported_interfaces value is > >>> reparsed with the remaining PCS interfaces. > >>> > >>> Also a phylink resolve is triggered to handle the PCS removal. > >>> > >>> The flag force_major_config is set to make phylink resolve reconfigure > >>> the interface (even if it didn't change). > >>> This is needed to handle the special case when the current PCS used > >>> by phylink is removed and a major_config is needed to propagae the > >>> configuration change. With this option enabled we also force mac_config > >>> even if the PHY link is not up for the in-band case. > >>> > >>> Signed-off-by: Christian Marangi > >>> --- > >>> drivers/net/phy/phylink.c | 56 +++++++++++++++++++++++++++++++++++++++ > >>> include/linux/phylink.h | 2 ++ > >>> 2 files changed, 58 insertions(+) > >>> > >>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > >>> index c38bcd43b8c8..064d6f5a06da 100644 > >>> --- a/drivers/net/phy/phylink.c > >>> +++ b/drivers/net/phy/phylink.c > >>> @@ -158,6 +158,8 @@ static const phy_interface_t phylink_sfp_interface_preference[] = { > >>> static DECLARE_PHY_INTERFACE_MASK(phylink_sfp_interfaces); > >>> > >>> static void phylink_run_resolve(struct phylink *pl); > >>> +static void phylink_link_down(struct phylink *pl); > >>> +static void phylink_pcs_disable(struct phylink_pcs *pcs); > >>> > >>> /** > >>> * phylink_set_port_modes() - set the port type modes in the ethtool mask > >>> @@ -918,6 +920,60 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state) > >>> } > >>> } > >>> > >>> +/** > >>> + * phylink_release_pcs - Removes a PCS from the phylink PCS available list > >>> + * @pcs: a pointer to the phylink_pcs struct to be released > >>> + * > >>> + * This function release a PCS from the phylink PCS available list if > >>> + * actually in use. It also refreshes the supported interfaces of the > >>> + * phylink instance by copying the supported interfaces from the phylink > >>> + * conf and merging the supported interfaces of the remaining available PCS > >>> + * in the list and trigger a resolve. > >>> + */ > >>> +void phylink_release_pcs(struct phylink_pcs *pcs) > >>> +{ > >>> + struct phylink *pl; > >>> + > >>> + ASSERT_RTNL(); > >>> + > >>> + pl = pcs->phylink; > >>> + if (!pl) > >>> + return; > >>> + > >>> + mutex_lock(&pl->state_mutex); > >>> + > >>> + list_del(&pcs->list); > >>> + pcs->phylink = NULL; > >>> + > >>> + /* > >>> + * Check if we are removing the PCS currently > >>> + * in use by phylink. If this is the case, tear down > >>> + * the link, force phylink resolve to reconfigure the > >>> + * interface mode, disable the current PCS and set the > >>> + * phylink PCS to NULL. > >>> + */ > >>> + if (pl->pcs == pcs) { > >>> + phylink_link_down(pl); > >>> + phylink_pcs_disable(pl->pcs); > >>> + > >>> + pl->force_major_config = true; > >>> + pl->pcs = NULL; > >>> + } > >>> + > >>> + mutex_unlock(&pl->state_mutex); > >>> + > >>> + /* Refresh supported interfaces */ > >>> + phy_interface_copy(pl->supported_interfaces, > >>> + pl->config->supported_interfaces); > >>> + list_for_each_entry(pcs, &pl->pcs_list, list) > >>> + phy_interface_or(pl->supported_interfaces, > >>> + pl->supported_interfaces, > >>> + pcs->supported_interfaces); > >> > >> I've given more thought to that 'supported_interfaces' thing. This > >> patchset redefines the meaning of > >> > >> pl->config->supported_interfaces > >> > >> Currently, it's filled by the MAC driver and means "Every interface > >> we can support, including the ones provided by PCSs that we can use > >> with this MAC". > >> > >> It now becomes "Every interface we support without needing a PCS", at > >> least the way I understand that. > >> > > > > Wait but with the current code using the OR logic, it still follows > > "Every interface we can support...". The modes that needs a PCS are > > specificed with the pcs_interfaces mask in phylink_config. > > you current code is correct, I was mostly concerned about the doc > that goes along with it :) Oh yep thanks, I will update also that entry. Also maybe check the .rst introduced and tell me if something is confusing or badly described. > > So in the end, we'd have something like (simplified): > > pl->config.supported_interfaces = RGMII_xx | SGMII | 1000BaseX > pl->config.pcs_interfaces = SGMII | 1000BaseX > > pcs->supported_interface = SGMII| 1000BaseX > > correct ? > Correct. phylink_config describing that for SGMII and 1000BaseX a PCS is required and the related PCS declaring support for those modes. Code will skip searching for a PCS for RGMII. > > > > The late add and release operates on the phylink supported_interfaces ONLY > > when the MAC didn't specify support for it (by removing it as only the PCS > > will declare support for it) > > > > The confusion is present because everything is validated later on > > major_config so those supported_interfaces are just an HINT that are later > > verified with get_caps and with the pcs_validate OPs. > > > > Adding the supported_interfaces to phylink is really to keep an original > > reference of the value. This is to address a pattern I have notice where > > the MAC driver always OR the interfaces with the one supported by the PCS. > > (I remember it was pointed out by Russell) > > > > But I'm more than open to discussion as this is something marginal to the > > whole implementation, I'm also questioning if this OR is actually useful to > > anything on the nth tought on this. > > > > One thing that I notice is that parsing this early with AND might be > > problematic at phylink_create, but I still have to evaluate that. > > > > My take is that would be good to have some review also on the other logic > > as I think I reached a point where Sashiko starts to comments on more or > > less unreal problem. > > True, TBH all the fwnode part is something I'm a bit less familiar with though > so maybe someone else can browse through that. > > FWIW, I've tested that whole series on a board that has "legacy" PCS board > that has mvpp2 and 2 possible PCSs, and it seems to work fine so no regressions > there :) > > A side note with the "legacy" naming, I'd rather have it called "built-in" or > something like that, I don't see a clear path to porting the existing code to > fwnode without breaking DT compat, as it's likely we'll have to remove the PCS > register ranges out of the MAC's range. I think also built-in might be confusing as a SoC might have a PCS built-in but just as a separate device on a different register map. A better description might be "externally-managed" but that is very long... Also the use of fwnode and DT is just to reference them and use the normal helper but the implementation is liberal on custom implementation for the current code. Everything is around the pcs_interface mask, num_possible_pcs and the fill_available_pcs. Nobody stops from implementing custom parser in fill_available_pcs and return a PCS pointer created directly by the MAC. (In such case late and release won't be needed as everything is present when phylink_create is called) On OpenWrt we migrated every SoC that used ""exotic"" pcs reference (Qcom, Realtek, MTK, Airoha) but I don't have a board that used something like the 'pcs' property. > > Thanks for this work anyway, this is great ! > > Maxime -- Ansuel