[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250903083601.aghtwpeoh7krh7ao@skbuf>
Date: Wed, 3 Sep 2025 11:36:01 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net] net: phy: transfer phy_config_inband() locking
responsibility to phylink
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);
Powered by blists - more mailing lists