Re: [PATCH net-next v16 06/12] net: phylink: support late PCS provider attach
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-18 00:45:24
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] phylink: missing pl->supported update on late PCS attach - [Medium] phylink: data race on pl->supported_interfaces during PCS hotplug -- commit 09779c4042c55422798d0cff65f1f25068da26a9 Author: Christian Marangi [off-list ref] Subject: net: phylink: support late PCS provider attach This commit adds support for late PCS provider attachment to a phylink instance. It introduces a FWNODE_PCS_PROVIDER_ADD event that allows a PCS provider to be linked and supported interfaces to be refreshed if the provider is added after the instance is created. If a link was previously down, it can be re-established when the PCS returns.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 88570a59ba9e7..fb3fb522769b6 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2006,27 +2027,57 @@ static int pcs_provider_notify(struct notifier_block *self, struct fwnode_pcs_provider *pp = data; struct phylink_pcs *pcs, *tmp; bool resolve = false; + int count, i; - rtnl_lock(); + /* + * On PCS provider deletion hold rtnl lock as one of + * PCS can be currently in use by the phylink instance + * and ethtool OPs can reference it. + */ + if (val == FWNODE_PCS_PROVIDER_DEL) + rtnl_lock();
[Severity: Medium] Is it safe to conditionally skip rtnl_lock() on the FWNODE_PCS_PROVIDER_ADD path? Later in this function, pl->supported_interfaces is modified: 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); Functions like phylink_validate_mask() read this multi-word bitmap word-by-word while holding only rtnl_lock, without acquiring pl->state_mutex. Could this lead to a torn read, causing concurrent ethtool operations to see an invalid interface mask and reject valid configurations? [ ... ]
quoted hunk ↗ jump to hunk
/* Exit early if nothing has changed */ if (!resolve) { mutex_unlock(&pl->state_mutex); - rtnl_unlock(); + + if (val == FWNODE_PCS_PROVIDER_DEL) + rtnl_unlock(); + return NOTIFY_DONE; }@@ -2040,7 +2091,8 @@ static int pcs_provider_notify(struct notifier_block *self, mutex_unlock(&pl->state_mutex); - rtnl_unlock(); + if (val == FWNODE_PCS_PROVIDER_DEL) + rtnl_unlock(); phylink_run_resolve(pl);
[Severity: Medium] When a late PCS is attached, does pl->supported need to be recalculated alongside pl->supported_interfaces? If a late PCS provides new interface capabilities (such as new 10G link modes), updating pl->supported_interfaces without also recalculating pl->supported (via phylink_validate()) could leave the ethtool capabilities stale. Could this result in phylink_ethtool_ksettings_set() rejecting valid configurations? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260916235200.20712-1-ansuelsmth@gmail.com?part=6