Re: [RFC PATCH net] net: phy: transfer phy_config_inband() locking responsibility to phylink

[Vladimir Oltean]

On Tue, Sep 02, 2025 at 06:02:49PM +0300, Vladimir Oltean wrote:
> On Tue, Sep 02, 2025 at 05:42:41PM +0300, Vladimir Oltean wrote:
> > Can we disable the resolver from phylink_sfp_disconnect_phy(), to offer
> > a similar guarantee that phylink_disconnect_phy() never runs with a
> > concurrent resolver?
> 
> Hmm, I now noticed phylink_sfp_link_down() which does disable the
> resolver already. I need to test/understand whether the SFP state
> machine ever calls sfp_remove_phy() without a prior sfp_link_down(), if
> the link was up.

This is ugly but is also the only functional idea I have (patch is on
top of the submitted one):

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9609dc445a0a..16644d5dfa5b 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -67,6 +67,8 @@ struct phylink {
 	struct timer_list link_poll;
 
 	struct mutex state_mutex;
+	/* Serialize updates to pl->phydev with phylink_resolve() */
+	struct mutex phy_lock;
 	struct phylink_link_state phy_state;
 	unsigned int phy_ib_mode;
 	struct work_struct resolve;
@@ -1594,11 +1596,13 @@ static void phylink_resolve(struct work_struct *w)
 {
 	struct phylink *pl = container_of(w, struct phylink, resolve);
 	struct phylink_link_state link_state;
-	struct phy_device *phy = pl->phydev;
 	bool mac_config = false;
 	bool retrigger = false;
+	struct phy_device *phy;
 	bool cur_link_state;
 
+	mutex_lock(&pl->phy_lock);
+	phy = pl->phydev;
 	if (phy)
 		mutex_lock(&phy->lock);
 	mutex_lock(&pl->state_mutex);
@@ -1704,6 +1708,7 @@ static void phylink_resolve(struct work_struct *w)
 	mutex_unlock(&pl->state_mutex);
 	if (phy)
 		mutex_unlock(&phy->lock);
+	mutex_unlock(&pl->phy_lock);
 }
 
 static void phylink_run_resolve(struct phylink *pl)
@@ -1839,6 +1844,7 @@ struct phylink *phylink_create(struct phylink_config *config,
 	if (!pl)
 		return ERR_PTR(-ENOMEM);
 
+	mutex_init(&pl->phy_lock);
 	mutex_init(&pl->state_mutex);
 	INIT_WORK(&pl->resolve, phylink_resolve);
 
@@ -2099,6 +2105,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 		     dev_name(&phy->mdio.dev), phy->drv->name, irq_str);
 	kfree(irq_str);
 
+	mutex_lock(&pl->phy_lock);
 	mutex_lock(&phy->lock);
 	mutex_lock(&pl->state_mutex);
 	pl->phydev = phy;
@@ -2144,6 +2151,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 
 	mutex_unlock(&pl->state_mutex);
 	mutex_unlock(&phy->lock);
+	mutex_unlock(&pl->phy_lock);
 
 	phylink_dbg(pl,
 		    "phy: %s setting supported %*pb advertising %*pb\n",
@@ -2324,6 +2332,7 @@ void phylink_disconnect_phy(struct phylink *pl)
 
 	phy = pl->phydev;
 	if (phy) {
+		mutex_lock(&pl->phy_lock);
 		mutex_lock(&phy->lock);
 		mutex_lock(&pl->state_mutex);
 		pl->phydev = NULL;
@@ -2331,6 +2340,7 @@ void phylink_disconnect_phy(struct phylink *pl)
 		pl->mac_tx_clk_stop = false;
 		mutex_unlock(&pl->state_mutex);
 		mutex_unlock(&phy->lock);
+		mutex_unlock(&pl->phy_lock);
 		flush_work(&pl->resolve);
 
 		phy_disconnect(phy);