[PATCH net 0/2] amd-xgbe: fix two PHY/IRQ lifecycle bugs found on SolidRun Bedrock V3000

[PATCH net 0/2] amd-xgbe: fix two PHY/IRQ lifecycle bugs found on SolidRun Bedrock V3000

[Stepan Svatenko]

These two bugs were found while debugging SFP-related failures on a
SolidRun Bedrock V3000 (AMD Ryzen Embedded V3C48), on kernels 6.15.5,
6.18 and 7.0. Both are longstanding (dating back to when the affected
code was originally added) and independent of each other, but touch
related PHY/IRQ lifecycle paths in the same driver, so sending them
together:

  1/2 fixes a self-deadlock: xgbe_phy_sfp_detect() holds
      xgbe_phy_comm_lock across a call chain that can re-enter the
      driver's own MDIO read/write callbacks, which take the same
      mutex. Triggered by removing an SFP module while an external
      PHY is attached; confirmed hung via the kernel hung-task
      detector (368s+ blocked).

  2/2 fixes an IRQ leak: xgbe_phy_start() can return an error after
      already registering its AN/PCS interrupt, without freeing it
      or clearing state, if xgbe_phy_config_aneg() fails (e.g.
      against a non-functional SFP module). Any later retry then
      hits -EBUSY requesting the same still-registered IRQ forever,
      with no recovery short of a power cycle.

Both were hardware-tested on a SolidRun Bedrock V3000 running 6.15.5
with these fixes applied: repeated SFP insert/remove and interface
up/down cycles, including with a non-functional module, no longer
trigger the hang or the -EBUSY loop. The code paths touched by both
patches are byte-for-byte unchanged between 6.15.5 and the tree this
series is generated against, so the same fix applies directly here.

Stepan Svatenko (2):
  amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal
  amd-xgbe: fix an_irq leak causing permanent -EBUSY on PHY (re)start

 drivers/net/ethernet/amd/xgbe/xgbe-mdio.c   | 14 ++++++-
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 41 ++++++++++++++++-----
 2 files changed, 45 insertions(+), 10 deletions(-)

--
2.55.0

[PATCH net 1/2] amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal

[Stepan Svatenko]

xgbe_phy_sfp_detect() acquires xgbe_phy_comm_lock (via
xgbe_phy_get_comm_ownership()) and holds it across calls that can end
up freeing the external PHY device:

  xgbe_phy_sfp_detect()
    xgbe_phy_get_comm_ownership()                  <- mutex_lock
    xgbe_phy_sfp_mod_absent() / xgbe_phy_sfp_read_eeprom() (SFP changed)
      xgbe_phy_free_phy_device()
        phy_detach()
          phy_suspend()
            genphy_suspend()
              xgbe_phy_mii_read_c22()               <- mii_bus->read
                xgbe_phy_get_comm_ownership()       <- mutex_lock again

xgbe_phy_comm_lock is a plain, non-recursive mutex. phy_detach() ends
up calling back into this driver's own MDIO bus callbacks
(xgbe_phy_mii_read_c22()/xgbe_phy_mii_write_c22(), reached via
genphy_suspend() during phy_detach()), which independently acquire the
same lock, so the second acquisition deadlocks the task tearing down
the SFP module.

This is reliably reproducible by removing an SFP module while an
external PHY is attached: the removal handler hangs forever inside
xgbe_phy_free_phy_device(), confirmed via /proc/<pid>/stack and the
kernel hung-task detector (blocked 368s+). Reproduced on a SolidRun
Bedrock V3000 (AMD Ryzen Embedded V3C48).

There were two call paths into xgbe_phy_free_phy_device() while the
mutex was held: the module-absent path, and a second one inside
xgbe_phy_sfp_read_eeprom() when the EEPROM contents change (e.g. a
module swap).

Fix this by never calling xgbe_phy_free_phy_device() (directly, or via
xgbe_phy_sfp_mod_absent()) while holding xgbe_phy_comm_lock.
xgbe_phy_sfp_read_eeprom() no longer frees the PHY device itself; it
only records that the SFP changed. xgbe_phy_sfp_detect() releases the
mutex before calling xgbe_phy_sfp_mod_absent() or
xgbe_phy_free_phy_device(), and re-acquires it only around the
remaining raw I2C access in xgbe_phy_sfp_external_phy(). Neither
xgbe_phy_sfp_mod_absent() nor xgbe_phy_sfp_parse_eeprom()/
xgbe_phy_sfp_phy_settings() touch hardware directly, so they don't
need the mutex held.

Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
Cc: stable@vger.kernel.org
Signed-off-by: Stepan Svatenko <ssvatenko@iit.org.ua>
Assisted-by: Claude Code:claude-sonnet-5 [Bash] [Read] [Edit]
---
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 41 ++++++++++++++++-----
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 59a074ed312a..a264ec5bb085 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -1218,7 +1218,13 @@ static int xgbe_phy_sfp_read_eeprom(struct xgbe_prv_data *pdata)
 		goto put;
 	}
 
-	/* Check for an added or changed SFP */
+	/* Check for an added or changed SFP. Freeing any existing external
+	 * PHY device is deferred to the caller: xgbe_phy_free_phy_device()
+	 * can end up calling back into this driver's MDIO read/write
+	 * routines (via phy_detach() -> phy_suspend()), which take the
+	 * comm ownership mutex themselves, and that mutex is held across
+	 * this call.
+	 */
 	if (memcmp(&phy_data->sfp_eeprom, &sfp_eeprom, sizeof(sfp_eeprom))) {
 		phy_data->sfp_changed = 1;
 
@@ -1226,8 +1232,6 @@ static int xgbe_phy_sfp_read_eeprom(struct xgbe_prv_data *pdata)
 			xgbe_phy_sfp_eeprom_info(pdata, &sfp_eeprom);
 
 		memcpy(&phy_data->sfp_eeprom, &sfp_eeprom, sizeof(sfp_eeprom));
-
-		xgbe_phy_free_phy_device(pdata);
 	} else {
 		phy_data->sfp_changed = 0;
 	}
@@ -1296,26 +1300,45 @@ static void xgbe_phy_sfp_detect(struct xgbe_prv_data *pdata)
 	/* Read the SFP signals and check for module presence */
 	xgbe_phy_sfp_signals(pdata);
 	if (phy_data->sfp_mod_absent) {
+		/* xgbe_phy_sfp_mod_absent() calls xgbe_phy_free_phy_device(),
+		 * which can call back into this driver's MDIO read/write
+		 * routines via phy_detach() -> phy_suspend(). Those routines
+		 * take the comm ownership mutex themselves, so it must be
+		 * released before making this call.
+		 */
+		xgbe_phy_put_comm_ownership(pdata);
 		xgbe_phy_sfp_mod_absent(pdata);
-		goto put;
+		goto settings;
 	}
 
 	ret = xgbe_phy_sfp_read_eeprom(pdata);
+	xgbe_phy_put_comm_ownership(pdata);
 	if (ret) {
 		/* Treat any error as if there isn't an SFP plugged in */
 		xgbe_phy_sfp_reset(phy_data);
 		xgbe_phy_sfp_mod_absent(pdata);
-		goto put;
+		goto settings;
 	}
 
+	/* Same reasoning as above: this must run without the comm
+	 * ownership mutex held.
+	 */
+	if (phy_data->sfp_changed)
+		xgbe_phy_free_phy_device(pdata);
+
 	xgbe_phy_sfp_parse_eeprom(pdata);
 
-	xgbe_phy_sfp_external_phy(pdata);
+	/* Re-acquire ownership for the external PHY access below; it talks
+	 * to the SFP over I2C directly and needs the mutex held again.
+	 */
+	ret = xgbe_phy_get_comm_ownership(pdata);
+	if (!ret) {
+		xgbe_phy_sfp_external_phy(pdata);
+		xgbe_phy_put_comm_ownership(pdata);
+	}
 
-put:
+settings:
 	xgbe_phy_sfp_phy_settings(pdata);
-
-	xgbe_phy_put_comm_ownership(pdata);
 }
 
 static int xgbe_phy_module_eeprom(struct xgbe_prv_data *pdata,
-- 
2.55.0

[PATCH net 2/2] amd-xgbe: fix an_irq leak causing permanent -EBUSY on PHY (re)start

[Stepan Svatenko]

xgbe_phy_start() requests the separate AN/PCS interrupt (an_irq) and,
on any failure past that point, is expected to free it again via the
err_irq/err_stop labels before returning an error. The last error path
skips that cleanup:

  pdata->phy_started = 1;
  xgbe_an_init(pdata);
  xgbe_an_enable_interrupts(pdata);
  return xgbe_phy_config_aneg(pdata);   <- returns directly on error

xgbe_phy_config_aneg() (via __xgbe_phy_config_aneg()) can genuinely
fail, e.g. when phy_impl.an_config() fails against a non-functional
SFP module. When it does, xgbe_phy_start() returns that error without
going through err_irq/err_stop, so:

 - an_irq is never freed with devm_free_irq(), and
 - pdata->phy_started is left set to 1, even though the caller
   (xgbe_start()) now treats this as a failed start and does not call
   phy_if->phy_stop() itself on that path.

Any later retry of xgbe_phy_start() (interface bring-up retried by
userspace, or the driver's own recovery logic) then calls
devm_request_irq() for the same still-registered an_irq and gets
-EBUSY every time, with no way to recover short of a reboot/power
cycle:

  genirq: Flags mismatch irq 63. 00200000 (enp8s0f3-pcs) vs. 00200000 (enp8s0f3-pcs)
  amd-xgbe 0000:08:00.3: error -EBUSY: request_irq(63) xgbe_an_isr [amd_xgbe] 0x0 enp8s0f3-pcs
  amd-xgbe 0000:08:00.3 enp8s0f3: phy irq request failed

Reproduced on a SolidRun Bedrock V3000 (AMD Ryzen Embedded V3C48) by
inserting a non-functional SFP module, then bringing the interface up.

Fix this by routing the xgbe_phy_config_aneg() failure through
xgbe_phy_stop(), which already contains the correct, symmetric
teardown (disables AN, frees an_irq if separate, cancels the bh work,
stops the PHY implementation) and is safe to call here because it is
gated on pdata->phy_started.

Fixes: 7c12aa08779c ("amd-xgbe: Move the PHY support into amd-xgbe")
Cc: stable@vger.kernel.org
Signed-off-by: Stepan Svatenko <ssvatenko@iit.org.ua>
Assisted-by: Claude Code:claude-sonnet-5 [Bash] [Read] [Edit]
---
 drivers/net/ethernet/amd/xgbe/xgbe-mdio.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
index 12770af031eb..638c24b9c83c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
@@ -1445,7 +1445,19 @@ static int xgbe_phy_start(struct xgbe_prv_data *pdata)
 	xgbe_an_init(pdata);
 	xgbe_an_enable_interrupts(pdata);
 
-	return xgbe_phy_config_aneg(pdata);
+	ret = xgbe_phy_config_aneg(pdata);
+	if (ret) {
+		/* Tear down what was just brought up above (including
+		 * freeing the an_irq) instead of returning with phy_started
+		 * left set and an_irq still registered - otherwise a retry
+		 * calls devm_request_irq() on an already-owned an_irq and
+		 * gets stuck in a permanent -EBUSY loop.
+		 */
+		xgbe_phy_stop(pdata);
+		return ret;
+	}
+
+	return 0;
 
 err_irq:
 	if (pdata->dev_irq != pdata->an_irq)
-- 
2.55.0

Re: [PATCH net 1/2] amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal

The patch stops xgbe_phy_sfp_read_eeprom() from freeing the external PHY
device and instead defers that to xgbe_phy_sfp_detect(), which now drops
xgbe_phy_comm_lock before calling xgbe_phy_sfp_mod_absent() or
xgbe_phy_free_phy_device() and re-acquires it only around
xgbe_phy_sfp_external_phy().  A couple of questions about the new
sequence below.

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 59a074ed312a5..a264ec5bb0851 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c

[ ... ]

> @@ -1296,26 +1300,45 @@ static void xgbe_phy_sfp_detect(struct xgbe_prv_data *pdata)
>  	/* Read the SFP signals and check for module presence */
>  	xgbe_phy_sfp_signals(pdata);
>  	if (phy_data->sfp_mod_absent) {
> +		/* xgbe_phy_sfp_mod_absent() calls xgbe_phy_free_phy_device(),
> +		 * which can call back into this driver's MDIO read/write
> +		 * routines via phy_detach() -> phy_suspend(). Those routines
> +		 * take the comm ownership mutex themselves, so it must be
> +		 * released before making this call.
> +		 */
> +		xgbe_phy_put_comm_ownership(pdata);
>  		xgbe_phy_sfp_mod_absent(pdata);
> -		goto put;
> +		goto settings;
>  	}
>  
>  	ret = xgbe_phy_sfp_read_eeprom(pdata);
> +	xgbe_phy_put_comm_ownership(pdata);
>  	if (ret) {
>  		/* Treat any error as if there isn't an SFP plugged in */
>  		xgbe_phy_sfp_reset(phy_data);
>  		xgbe_phy_sfp_mod_absent(pdata);
> -		goto put;
> +		goto settings;
>  	}
>  
> +	/* Same reasoning as above: this must run without the comm
> +	 * ownership mutex held.
> +	 */
> +	if (phy_data->sfp_changed)
> +		xgbe_phy_free_phy_device(pdata);

[Severity: High]
Can freeing phy_data->phydev here race with the ethtool configuration
path and turn into a use-after-free write?

Before this patch the teardown could not actually complete from the
service work, because phy_detach() -> phy_suspend() -> genphy_suspend()
-> xgbe_phy_mii_read_c22() -> xgbe_phy_get_comm_ownership() deadlocked
on the mutex that was still held.  That is the hang this commit fixes.
With the mutex released, xgbe_phy_free_phy_device() now runs to
completion from:

xgbe_service()
  xgbe_phy_status()
    xgbe_phy_link_status()
      xgbe_phy_sfp_detect()
        xgbe_phy_sfp_mod_absent() / xgbe_phy_free_phy_device()

xgbe_service() holds neither RTNL nor pdata->an_mutex, and
xgbe_phy_free_phy_device() dereferences the shared pointer before
clearing it:

	if (phy_data->phydev) {
		phy_detach(phy_data->phydev);
		phy_device_remove(phy_data->phydev);
		phy_device_free(phy_data->phydev);
		phy_data->phydev = NULL;
	}

Meanwhile ethtool -s / ethtool -A enter xgbe_set_link_ksettings() /
xgbe_set_pauseparam() under only "if (netif_running(netdev))", then
__xgbe_phy_config_aneg() (which takes pdata->an_mutex only) calls
xgbe_phy_an_config():

	if (!phy_data->phydev)
		return 0;

	phy_data->phydev->autoneg = pdata->phy.autoneg;
	linkmode_and(phy_data->phydev->advertising, ...);
	...
	ret = phy_start_aneg(phy_data->phydev);

The freeing path holds no lock the consumer path takes, so can the
phy_device be freed between that NULL check and the writes?  The same
window looks present for phy_init_hw(phy_data->phydev) and the
phy_data->phydev dereferences in xgbe_phy_phydev_flowctrl().  Neither
ethtool entry point flushes or cancels pdata->service_work; only
xgbe_stop() does that, which is why the existing free in
xgbe_phy_stop() is safe.

Also, phy_detach() uses rtnl_dereference(dev->hwprov) and calls
phy_link_topo_del_phy(), so it expects RTNL to be held.  Should the
teardown be moved into a context that holds RTNL or an_mutex, or be
serialized against the phydev consumers some other way?

>  	xgbe_phy_sfp_parse_eeprom(pdata);
>  
> -	xgbe_phy_sfp_external_phy(pdata);
> +	/* Re-acquire ownership for the external PHY access below; it talks
> +	 * to the SFP over I2C directly and needs the mutex held again.
> +	 */
> +	ret = xgbe_phy_get_comm_ownership(pdata);
> +	if (!ret) {
> +		xgbe_phy_sfp_external_phy(pdata);
> +		xgbe_phy_put_comm_ownership(pdata);
> +	}

[Severity: Medium]
What happens to phy_data->sfp_phy_avail when this second
xgbe_phy_get_comm_ownership() fails?

xgbe_phy_get_comm_ownership() can return -ETIMEDOUT after polling
XP_I2C_MUTEX / XP_MDIO_MUTEX for five seconds and printing "unable to
obtain hardware mutexes".  There is no else branch here, so the failure
is neither propagated nor reflected in driver state.

By this point the "SFP changed" edge has already been consumed:
xgbe_phy_sfp_read_eeprom() set phy_data->sfp_changed = 1 and copied the
new image into phy_data->sfp_eeprom, xgbe_phy_free_phy_device() already
cleared phy_data->phydev, and xgbe_phy_sfp_parse_eeprom() reprogrammed
sfp_base/sfp_speed.

xgbe_phy_sfp_external_phy() is the only writer of sfp_phy_avail and
self-disables on the next poll:

	if (!phy_data->sfp_changed)
		return;

	phy_data->sfp_phy_avail = 0;
	...
	ret = xgbe_phy_i2c_mii_read(pdata, MII_BMCR);
	if (ret < 0)
		return;

	phy_data->sfp_phy_avail = 1;

On the following service poll the EEPROM compares equal, sfp_changed
becomes 0, and this function returns immediately, so does sfp_phy_avail
keep the value computed for the previous module until the module is
physically re-plugged?

The consumer xgbe_phy_find_phy_device() gates on that flag:

	if ((phy_data->port_mode == XGBE_PORT_MODE_SFP) &&
	    !phy_data->sfp_phy_avail)
		return 0;

so a stale 1 after a copper-to-fiber swap would make get_phy_device()
fail and return -ENODEV up through xgbe_phy_an_config() ->
__xgbe_phy_config_aneg() -> xgbe_phy_start(), and a stale 0 for a newly
inserted 1000BASE-T module would leave its internal PHY unattached even
though xgbe_phy_sfp_phy_settings() just advertised 10/100/1000baseT.
xgbe_phy_link_status() sets *an_restart = 1 whenever sfp_changed is
set, so xgbe_phy_config_aneg() acts on the stale flag right away.

Before this patch a mutex timeout aborted xgbe_phy_sfp_detect() with all
bookkeeping untouched and the next poll re-ran the whole sequence.  Would
it make sense to handle this like the read_eeprom error path does, i.e.
xgbe_phy_sfp_reset() plus xgbe_phy_sfp_mod_absent(), or otherwise avoid
committing sfp_eeprom/sfp_changed before the probe can be retried?  The
commit message does not mention this new failure path.

>  
> -put:
> +settings:
>  	xgbe_phy_sfp_phy_settings(pdata);
> -
> -	xgbe_phy_put_comm_ownership(pdata);
>  }

Both observations are still present at the end of the series, since the
following patch touches only xgbe-mdio.c.
-- 
pw-bot: cr

[PATCH v2] amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal

[Stepan Svatenko]

xgbe_phy_sfp_detect() acquires xgbe_phy_comm_lock (via
xgbe_phy_get_comm_ownership()) and holds it across calls that can end
up freeing the external PHY device:

  xgbe_phy_sfp_detect()
    xgbe_phy_get_comm_ownership()                  <- mutex_lock
    xgbe_phy_sfp_mod_absent() / xgbe_phy_sfp_read_eeprom() (SFP changed)
      xgbe_phy_free_phy_device()
        phy_detach()
          phy_suspend()
            genphy_suspend()
              xgbe_phy_mii_read_c22()               <- mii_bus->read
                xgbe_phy_get_comm_ownership()       <- mutex_lock again

xgbe_phy_comm_lock is a plain, non-recursive mutex. phy_detach() ends
up calling back into this driver's own MDIO bus callbacks
(xgbe_phy_mii_read_c22()/xgbe_phy_mii_write_c22(), reached via
genphy_suspend() during phy_detach()), which independently acquire the
same lock, so the second acquisition deadlocks the task tearing down
the SFP module.

This is reliably reproducible by removing an SFP module while an
external PHY is attached: the removal handler hangs forever inside
xgbe_phy_free_phy_device(), confirmed via /proc/<pid>/stack and the
kernel hung-task detector (blocked 368s+). Reproduced on a SolidRun
Bedrock V3000 (AMD Ryzen Embedded V3C48).

There were two call paths into xgbe_phy_free_phy_device() while the
mutex was held: the module-absent path, and a second one inside
xgbe_phy_sfp_read_eeprom() when the EEPROM contents change (e.g. a
module swap).

Fix this by never calling xgbe_phy_free_phy_device() (directly, or via
xgbe_phy_sfp_mod_absent()) while holding xgbe_phy_comm_lock.
xgbe_phy_sfp_read_eeprom() no longer frees the PHY device itself; it
only records that the SFP changed. xgbe_phy_sfp_detect() releases the
mutex before calling xgbe_phy_sfp_mod_absent() or
xgbe_phy_free_phy_device(), and re-acquires it only around the
remaining raw I2C access in xgbe_phy_sfp_external_phy(). Neither
xgbe_phy_sfp_mod_absent() nor xgbe_phy_sfp_parse_eeprom()/
xgbe_phy_sfp_phy_settings() touch hardware directly, so they don't
need the mutex held.

phy_detach()/phy_device_remove() additionally require RTNL to be held
by the caller. The callers here run from the service workqueue with no
lock held, and a plain rtnl_lock() cannot be used: xgbe_stopdev()
(system workqueue) takes rtnl_lock() and then calls
flush_workqueue(pdata->dev_workqueue) inside xgbe_stop(), which would
block waiting for this very (dev_workqueue) work item to finish, while
that work item is blocked waiting to reacquire RTNL from
xgbe_stopdev() - an ABBA deadlock. Use a non-blocking rtnl_trylock()
instead: on contention, skip the teardown for this poll and let the
next service poll (100ms-1s later) retry it; if the interface is going
down concurrently, xgbe_phy_stop() (phy_impl.stop) frees the PHY
itself under RTNL already held by its own caller, so nothing is lost
by skipping here.

Taking RTNL around the free also closes a second, independent race:
xgbe does not opt into the newer per-netdevice instance lock
(netdev_need_ops_lock() in include/net/netdev_lock.h checks
dev->request_ops_lock / dev->queue_mgmt_ops / dev->netdev_ops->
net_shaper_ops, none of which this driver sets), so __dev_ethtool()
computes need_rtnl = true and takes rtnl_lock() before calling into
this driver's set_link_ksettings()/set_pauseparam(), which dereference
phy_data->phydev without any lock of their own. Before this change,
xgbe_phy_free_phy_device() could free phy_data->phydev out from under
those ethtool paths with nothing serializing the two. With the free
now gated on rtnl_trylock(), it only proceeds while holding RTNL, so
it is properly excluded from any ethtool_ops call on this device: a
concurrent ethtool call either already holds RTNL (the trylock loses
and the free is retried later) or is blocked waiting to acquire it
(behind the still-held lock from the free side).

Finally, if re-acquiring comm ownership for xgbe_phy_sfp_external_phy()
fails after a new SFP's EEPROM has already been parsed, fall back to
the "no module" state (same as the existing xgbe_phy_sfp_read_eeprom()
failure path) instead of leaving sfp_changed/sfp_eeprom/sfp_base
updated for a module whose sfp_phy_avail was never actually set.

Fixes: abf0a1c2b26a ("amd-xgbe: Add support for SFP+ modules")
Cc: stable@vger.kernel.org
Signed-off-by: Stepan Svatenko <ssvatenko@iit.org.ua>
Assisted-by: Claude Code:claude-sonnet-5 [Bash] [Read] [Edit]
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

---
Changes since v1:
 - Use rtnl_trylock() instead of leaving the PHY teardown unguarded,
   closing both the ABBA deadlock against xgbe_stopdev()'s
   flush_workqueue(pdata->dev_workqueue) under RTNL, and a
   use-after-free race against the ethtool_ops paths
   (set_link_ksettings()/set_pauseparam()) that dereference
   phy_data->phydev without their own locking (both reported in
   review).
 - Handle the case where re-acquiring comm_ownership after parsing a
   new SFP's EEPROM fails, instead of leaving stale/incomplete SFP
   state in place (reported in review).
---
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 94 ++++++++++++++++++---
 1 file changed, 83 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 59a074ed312a..8c625a0340c4 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -11,6 +11,7 @@
 #include <linux/mdio.h>
 #include <linux/phy.h>
 #include <linux/ethtool.h>
+#include <linux/rtnetlink.h>
 
 #include "xgbe.h"
 #include "xgbe-common.h"
@@ -1218,7 +1219,13 @@ static int xgbe_phy_sfp_read_eeprom(struct xgbe_prv_data *pdata)
 		goto put;
 	}
 
-	/* Check for an added or changed SFP */
+	/* Check for an added or changed SFP. Freeing any existing external
+	 * PHY device is deferred to the caller: xgbe_phy_free_phy_device()
+	 * can end up calling back into this driver's MDIO read/write
+	 * routines (via phy_detach() -> phy_suspend()), which take the
+	 * comm ownership mutex themselves, and that mutex is held across
+	 * this call.
+	 */
 	if (memcmp(&phy_data->sfp_eeprom, &sfp_eeprom, sizeof(sfp_eeprom))) {
 		phy_data->sfp_changed = 1;
 
@@ -1226,8 +1233,6 @@ static int xgbe_phy_sfp_read_eeprom(struct xgbe_prv_data *pdata)
 			xgbe_phy_sfp_eeprom_info(pdata, &sfp_eeprom);
 
 		memcpy(&phy_data->sfp_eeprom, &sfp_eeprom, sizeof(sfp_eeprom));
-
-		xgbe_phy_free_phy_device(pdata);
 	} else {
 		phy_data->sfp_changed = 0;
 	}
@@ -1268,9 +1273,47 @@ static void xgbe_phy_sfp_mod_absent(struct xgbe_prv_data *pdata)
 
 	phy_data->sfp_mod_absent = 1;
 	phy_data->sfp_phy_avail = 0;
+	phy_data->sfp_changed = 0;
 	memset(&phy_data->sfp_eeprom, 0, sizeof(phy_data->sfp_eeprom));
 }
 
+/* phy_detach()/phy_device_remove(), called from xgbe_phy_free_phy_device()
+ * below, require RTNL to be held by the caller (phy_detach() itself uses
+ * rtnl_dereference() and phy_link_topo_del_phy()). The callers below run
+ * from the service workqueue with no lock held, so a plain rtnl_lock()
+ * cannot be used here: xgbe_stopdev() (system workqueue) takes rtnl_lock()
+ * and then calls flush_workqueue(pdata->dev_workqueue) inside xgbe_stop(),
+ * which would block waiting for this very (dev_workqueue) work item to
+ * finish - while it is blocked waiting to reacquire RTNL from
+ * xgbe_stopdev(). That is an ABBA deadlock, the same class of bug this
+ * driver just fixed elsewhere.
+ *
+ * Use a non-blocking rtnl_trylock() instead: on contention, skip the
+ * teardown for this poll and let the next service poll (100ms-1s later)
+ * retry it. If the interface is going down concurrently, xgbe_phy_stop()
+ * (phy_impl.stop) frees the PHY itself, under RTNL already held by its
+ * own caller - so nothing is lost by skipping here.
+ */
+static void xgbe_phy_sfp_mod_absent_safe(struct xgbe_prv_data *pdata)
+{
+	if (!rtnl_trylock())
+		return;
+
+	xgbe_phy_sfp_mod_absent(pdata);
+
+	rtnl_unlock();
+}
+
+static void xgbe_phy_free_phy_device_safe(struct xgbe_prv_data *pdata)
+{
+	if (!rtnl_trylock())
+		return;
+
+	xgbe_phy_free_phy_device(pdata);
+
+	rtnl_unlock();
+}
+
 static void xgbe_phy_sfp_reset(struct xgbe_phy_data *phy_data)
 {
 	phy_data->sfp_rx_los = 0;
@@ -1296,26 +1339,55 @@ static void xgbe_phy_sfp_detect(struct xgbe_prv_data *pdata)
 	/* Read the SFP signals and check for module presence */
 	xgbe_phy_sfp_signals(pdata);
 	if (phy_data->sfp_mod_absent) {
-		xgbe_phy_sfp_mod_absent(pdata);
-		goto put;
+		/* xgbe_phy_sfp_mod_absent() calls xgbe_phy_free_phy_device(),
+		 * which can call back into this driver's MDIO read/write
+		 * routines via phy_detach() -> phy_suspend(). Those routines
+		 * take the comm ownership mutex themselves, so it must be
+		 * released before making this call.
+		 */
+		xgbe_phy_put_comm_ownership(pdata);
+		xgbe_phy_sfp_mod_absent_safe(pdata);
+		goto settings;
 	}
 
 	ret = xgbe_phy_sfp_read_eeprom(pdata);
+	xgbe_phy_put_comm_ownership(pdata);
 	if (ret) {
 		/* Treat any error as if there isn't an SFP plugged in */
 		xgbe_phy_sfp_reset(phy_data);
-		xgbe_phy_sfp_mod_absent(pdata);
-		goto put;
+		xgbe_phy_sfp_mod_absent_safe(pdata);
+		goto settings;
 	}
 
+	/* Same reasoning as above: this must run without the comm
+	 * ownership mutex held.
+	 */
+	if (phy_data->sfp_changed)
+		xgbe_phy_free_phy_device_safe(pdata);
+
 	xgbe_phy_sfp_parse_eeprom(pdata);
 
-	xgbe_phy_sfp_external_phy(pdata);
+	/* Re-acquire ownership for the external PHY access below; it talks
+	 * to the SFP over I2C directly and needs the mutex held again.
+	 */
+	ret = xgbe_phy_get_comm_ownership(pdata);
+	if (!ret) {
+		xgbe_phy_sfp_external_phy(pdata);
+		xgbe_phy_put_comm_ownership(pdata);
+	} else {
+		/* Could not finish bringing up the new module: sfp_changed,
+		 * sfp_eeprom and sfp_base/sfp_speed were already updated
+		 * above for it, but external_phy() (and thus sfp_phy_avail)
+		 * never ran. Fall back to "no module" state instead of
+		 * committing to a half-initialized one - same pattern as
+		 * the read_eeprom() failure path above.
+		 */
+		xgbe_phy_sfp_reset(phy_data);
+		xgbe_phy_sfp_mod_absent_safe(pdata);
+	}
 
-put:
+settings:
 	xgbe_phy_sfp_phy_settings(pdata);
-
-	xgbe_phy_put_comm_ownership(pdata);
 }
 
 static int xgbe_phy_module_eeprom(struct xgbe_prv_data *pdata,
-- 
2.55.0

Re: [PATCH v2] amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 3 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The deferred external-PHY teardown introduced by this patch is
  silently and permanently lost when rtnl_trylock() fails.
- [High] The documented precondition that justifies rtnl_trylock() is
  factually wrong.
- [High] xgbe_phy_sfp_mod_absent_safe() (xgbe-phy-v2.c:1297-1305) skips
  the whole module-absent bookkeeping when rtnl_trylock() fails, yet…

--

amd-xgbe: fix comm_ownership mutex deadlock on SFP module removal

This moves the external PHY teardown out from under xgbe_phy_comm_lock so
that phy_detach() can no longer re-enter the driver's MDIO callbacks and
deadlock on the same non-recursive mutex.  The teardown is now done from
two new helpers that take RTNL with rtnl_trylock() before calling
xgbe_phy_sfp_mod_absent() / xgbe_phy_free_phy_device().

A few questions about the new trylock-based teardown below.

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 59a074ed312a..8c625a0340c4 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c

[ ... ]

> @@ -1218,7 +1219,13 @@ static int xgbe_phy_sfp_read_eeprom(struct xgbe_prv_data *pdata)
>  		goto put;
>  	}
>  
> -	/* Check for an added or changed SFP */
> +	/* Check for an added or changed SFP. Freeing any existing external
> +	 * PHY device is deferred to the caller: xgbe_phy_free_phy_device()
> +	 * can end up calling back into this driver's MDIO read/write
> +	 * routines (via phy_detach() -> phy_suspend()), which take the
> +	 * comm ownership mutex themselves, and that mutex is held across
> +	 * this call.
> +	 */
>  	if (memcmp(&phy_data->sfp_eeprom, &sfp_eeprom, sizeof(sfp_eeprom))) {
>  		phy_data->sfp_changed = 1;
>  
> @@ -1226,8 +1233,6 @@ static int xgbe_phy_sfp_read_eeprom(struct xgbe_prv_data *pdata)
>  			xgbe_phy_sfp_eeprom_info(pdata, &sfp_eeprom);
>  
>  		memcpy(&phy_data->sfp_eeprom, &sfp_eeprom, sizeof(sfp_eeprom));
> -
> -		xgbe_phy_free_phy_device(pdata);
>  	} else {
>  		phy_data->sfp_changed = 0;
>  	}

Note for the comment below: this memcpy() commits the new module's EEPROM
into phy_data->sfp_eeprom in the very same pass that sets sfp_changed = 1,
so the "changed" condition is consumed here and cannot be re-derived on a
later poll.

> @@ -1268,9 +1273,47 @@ static void xgbe_phy_sfp_mod_absent(struct xgbe_prv_data *pdata)
>  
>  	phy_data->sfp_mod_absent = 1;
>  	phy_data->sfp_phy_avail = 0;
> +	phy_data->sfp_changed = 0;
>  	memset(&phy_data->sfp_eeprom, 0, sizeof(phy_data->sfp_eeprom));
>  }
>  
> +/* phy_detach()/phy_device_remove(), called from xgbe_phy_free_phy_device()
> + * below, require RTNL to be held by the caller (phy_detach() itself uses
> + * rtnl_dereference() and phy_link_topo_del_phy()). The callers below run
> + * from the service workqueue with no lock held, so a plain rtnl_lock()
> + * cannot be used here: xgbe_stopdev() (system workqueue) takes rtnl_lock()

[Severity: High]
Is the statement "The callers below run from the service workqueue with no
lock held" accurate?  xgbe_phy_sfp_detect() has a second caller,
xgbe_phy_start() (phy_impl.start), which runs with RTNL already held by the
current task:

  xgbe_open()            (ndo_open, RTNL taken by the netdev core)
  xgbe_restart()         (rtnl_lock() before xgbe_restart_dev())
  xgbe_change_mtu()      -> xgbe_restart_dev()
    xgbe_start()
      pdata->phy_if.phy_impl.start()
        xgbe_phy_start()
          xgbe_phy_sfp_detect()

rtnl_trylock() is mutex_trylock(&rtnl_mutex), and mutex_trylock() fails when
the current task already owns the mutex.  So on the ifup / restart / MTU
change path, don't both new helpers become deterministic no-ops?

That drops more than the RTNL-requiring phy_detach(): all of
xgbe_phy_sfp_mod_absent()'s pure software bookkeeping (sfp_phy_avail = 0,
sfp_changed = 0, and the memset of sfp_eeprom above) needs no lock at all,
yet it is now only reachable through the trylock wrapper.

Would it make sense to split the RTNL-requiring free from the software state
reset that must always run, and handle the caller-already-holds-RTNL case
explicitly?

> + * and then calls flush_workqueue(pdata->dev_workqueue) inside xgbe_stop(),
> + * which would block waiting for this very (dev_workqueue) work item to
> + * finish - while it is blocked waiting to reacquire RTNL from
> + * xgbe_stopdev(). That is an ABBA deadlock, the same class of bug this
> + * driver just fixed elsewhere.
> + *
> + * Use a non-blocking rtnl_trylock() instead: on contention, skip the
> + * teardown for this poll and let the next service poll (100ms-1s later)
> + * retry it. If the interface is going down concurrently, xgbe_phy_stop()
> + * (phy_impl.stop) frees the PHY itself, under RTNL already held by its
> + * own caller - so nothing is lost by skipping here.
> + */
> +static void xgbe_phy_sfp_mod_absent_safe(struct xgbe_prv_data *pdata)
> +{
> +	if (!rtnl_trylock())
> +		return;
> +
> +	xgbe_phy_sfp_mod_absent(pdata);
> +
> +	rtnl_unlock();
> +}

[Severity: High]
When the trylock loses here, the whole module-absent transition is skipped,
but xgbe_phy_sfp_detect() still falls through to xgbe_phy_sfp_phy_settings()
as though it had been committed.  Can that wedge the port?

xgbe_phy_sfp_mod_absent() is the only place that zeroes phy_data->sfp_eeprom
and clears sfp_phy_avail/sfp_changed; xgbe_phy_sfp_reset() does not touch
sfp_eeprom:

  static void xgbe_phy_sfp_reset(struct xgbe_phy_data *phy_data)
  {
	phy_data->sfp_rx_los = 0;
	phy_data->sfp_tx_fault = 0;
	phy_data->sfp_mod_absent = 1;
	phy_data->sfp_base = XGBE_SFP_BASE_UNKNOWN;
	...
  }

So after a removal where the trylock failed, the old EEPROM stays cached.
If the module is re-inserted before a later poll wins the trylock,
xgbe_phy_sfp_read_eeprom() compares against that never-zeroed cache, finds
it identical and takes the else branch:

	} else {
		phy_data->sfp_changed = 0;
	}

and from then on xgbe_phy_sfp_external_phy() early-returns:

	if (!phy_data->sfp_changed)
		return;

so sfp_phy_avail is never re-validated, xgbe_phy_find_phy_device() returns 0
on the "!sfp_phy_avail" check, and xgbe_phy_sfp_phy_settings() early-returns
too:

	if (!phy_data->sfp_mod_absent && !phy_data->sfp_changed)
		return;

leaving pdata->phy.lks stuck in the absent-module capability set published
during the removal.  Does the port then stay unlinkable until an ifdown/ifup
or a physical re-seat?

The same loss applies to the new comm-ownership fallback added further down,
which calls this helper to "fall back to no module": if the trylock fails
there, sfp_changed stays 1 and sfp_eeprom stays committed to the new module,
which is exactly the half-initialized state that branch is meant to prevent.

> +
> +static void xgbe_phy_free_phy_device_safe(struct xgbe_prv_data *pdata)
> +{
> +	if (!rtnl_trylock())
> +		return;
> +
> +	xgbe_phy_free_phy_device(pdata);
> +
> +	rtnl_unlock();
> +}
> +
>  static void xgbe_phy_sfp_reset(struct xgbe_phy_data *phy_data)
>  {
>  	phy_data->sfp_rx_los = 0;

[ ... ]

> @@ -1296,26 +1339,55 @@ static void xgbe_phy_sfp_detect(struct xgbe_prv_data *pdata)
>  	/* Read the SFP signals and check for module presence */
>  	xgbe_phy_sfp_signals(pdata);
>  	if (phy_data->sfp_mod_absent) {
> -		xgbe_phy_sfp_mod_absent(pdata);
> -		goto put;
> +		/* xgbe_phy_sfp_mod_absent() calls xgbe_phy_free_phy_device(),
> +		 * which can call back into this driver's MDIO read/write
> +		 * routines via phy_detach() -> phy_suspend(). Those routines
> +		 * take the comm ownership mutex themselves, so it must be
> +		 * released before making this call.
> +		 */
> +		xgbe_phy_put_comm_ownership(pdata);
> +		xgbe_phy_sfp_mod_absent_safe(pdata);
> +		goto settings;
>  	}
>  
>  	ret = xgbe_phy_sfp_read_eeprom(pdata);
> +	xgbe_phy_put_comm_ownership(pdata);
>  	if (ret) {
>  		/* Treat any error as if there isn't an SFP plugged in */
>  		xgbe_phy_sfp_reset(phy_data);
> -		xgbe_phy_sfp_mod_absent(pdata);
> -		goto put;
> +		xgbe_phy_sfp_mod_absent_safe(pdata);
> +		goto settings;
>  	}
>  
> +	/* Same reasoning as above: this must run without the comm
> +	 * ownership mutex held.
> +	 */
> +	if (phy_data->sfp_changed)
> +		xgbe_phy_free_phy_device_safe(pdata);

[Severity: High]
Is the deferred free lost for good when rtnl_trylock() fails here?  The
commit message says:

  "on contention, skip the teardown for this poll and let the
   next service poll (100ms-1s later) retry it"

but nothing appears to record that the teardown is still pending.  The only
condition that reaches this call is sfp_changed, and
xgbe_phy_sfp_read_eeprom() already wrote the new EEPROM into
phy_data->sfp_eeprom in the same pass that set sfp_changed = 1, so the next
poll sees an identical memcmp() and sets sfp_changed = 0.

After that:

  - xgbe_phy_free_phy_device_safe() is never called again
  - xgbe_phy_sfp_external_phy() returns early on "if (!phy_data->sfp_changed)"
    so sfp_phy_avail is never re-evaluated
  - xgbe_phy_find_phy_device() returns immediately:

	/* If we already have a PHY, just return */
	if (phy_data->phydev)
		return 0;

and xgbe_phy_link_status() keeps gating link-up on the stale phy_device:

	if (phy_data->phydev) {
		ret = phy_read_status(phy_data->phydev);
		if (ret < 0)
			return 0;

Since RTNL is global, doesn't any concurrent ip/ethtool/netlink operation on
any interface in the system make the trylock fail for the one poll in which
the change is observed?  Would it be better to commit sfp_eeprom only after
a successful teardown, or to keep a sticky "needs free / needs reprobe" flag
so the next poll can genuinely retry?

>  	xgbe_phy_sfp_parse_eeprom(pdata);
>  
> -	xgbe_phy_sfp_external_phy(pdata);
> +	/* Re-acquire ownership for the external PHY access below; it talks
> +	 * to the SFP over I2C directly and needs the mutex held again.
> +	 */
> +	ret = xgbe_phy_get_comm_ownership(pdata);
> +	if (!ret) {
> +		xgbe_phy_sfp_external_phy(pdata);
> +		xgbe_phy_put_comm_ownership(pdata);
> +	} else {
> +		/* Could not finish bringing up the new module: sfp_changed,
> +		 * sfp_eeprom and sfp_base/sfp_speed were already updated
> +		 * above for it, but external_phy() (and thus sfp_phy_avail)
> +		 * never ran. Fall back to "no module" state instead of
> +		 * committing to a half-initialized one - same pattern as
> +		 * the read_eeprom() failure path above.
> +		 */
> +		xgbe_phy_sfp_reset(phy_data);
> +		xgbe_phy_sfp_mod_absent_safe(pdata);
> +	}
>  
> -put:
> +settings:
>  	xgbe_phy_sfp_phy_settings(pdata);
> -
> -	xgbe_phy_put_comm_ownership(pdata);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918142238.191589-1-ssvatenko%40iit.org.ua