Re: [PATCH net v3 3/3] net: stmmac: document oversized AF_XDP frame handling

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

Pre-existing issues:
- [High] The driver uses a bit flag and a polling loop to simulate a lock instead of using standard synchronization primitives.
--

--- Patch [3]: [PATCH net v3 3/3] net: stmmac: document oversized AF_XDP frame handling ---
commit 71d40d9a1bd119643235e87e55c9834a8a5c6c83
Author: Stanislav Fomichev <[email protected]>

net: stmmac: document oversized AF_XDP frame handling

This commit updates the driver to document its intentional ring imbalance
when dropping AF_XDP zero-copy frames that exceed taprio's queueMaxSDU. It
adds comments explaining that completing a rejected descriptor is unsafe
due to ordered completions, and instead wedges the ring to expose the error.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 62de03e65a90c..6a532747c0394 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2713,6 +2713,10 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
>  		if (priv->est && priv->est->enable &&
>  		    priv->est->max_sdu[queue] &&
>  		    xdp_desc.len > priv->est->max_sdu[queue]) {
> +			/* Completions are ordered, so this descriptor cannot
> +			 * be completed safely. Wedge the ring to expose the
> +			 * application error instead.
> +			 */
>  			priv->xstats.max_sdu_txq_drop[queue]++;
>  			continue;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but I noticed an ad-hoc locking
pattern using a bit flag in stmmac_reset_subtask() in this file:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_reset_subtask() {
    ...
	rtnl_lock();
	netif_trans_update(priv->dev);
	while (test_and_set_bit(STMMAC_RESETING, &priv->state))
		usleep_range(1000, 2000);

	set_bit(STMMAC_DOWN, &priv->state);
	dev_close(priv->dev);
	dev_open(priv->dev, NULL);
    ...
}

Are there any standard locking primitives that could be used here instead?

Simulating a lock with a bit flag and a sleep loop avoids lockdep tracking,
deadlock detection, and standard kernel memory ordering guarantees.

Could this pattern lead to subtle concurrency issues or hidden deadlocks
during the driver's reset task?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3