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

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.

[ ... ]

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.

[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;

[ ... ]

[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