Re: [PATCH net-next v5 3/6] net: xilinx: axienet: Propagate device reset failure from ndo_open

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: xilinx: axienet: u32 stats underflow on
  __axienet_device_reset() failure
- [Medium] net: xilinx: axienet: leaked legacy DMA rings and active DMA
  on open failure
--

commit 13e0ec7cb747f36a50832556f5abcdb18558f153
Author: Suraj Gupta <suraj.gupta2@amd.com>

net: xilinx: axienet: Propagate device reset failure from ndo_open

This commit checks and propagates the return value of
axienet_device_reset() in axienet_open(), tracks whether ndo_open
completed via lp->datapath_up so axienet_stop() is a no-op otherwise,
handles reopen failures in axienet_resume(), and clears
lp->reset_in_progress on __axienet_device_reset() failure.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 125ad3dbf6aa9..a553150faa89b 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -703,6 +703,17 @@ static int __axienet_device_reset(struct axienet_local *lp)
>  	}
>  
>  out:
> +	/* On failure the block above is skipped, so clear the flag here.
> +	 * axienet_open() now returns on a reset error without scheduling the
> +	 * statistics work that used to clear it, which would otherwise leave
> +	 * axienet_stat() returning the frozen base until a reset succeeds.
> +	 */
> +	if (ret && (lp->features & XAE_FEATURE_STATS)) {
> +		write_seqcount_begin(&lp->hw_stats_seqcount);
> +		lp->reset_in_progress = false;
> +		write_seqcount_end(&lp->hw_stats_seqcount);
> +	}
> +
>  	mutex_unlock(&lp->stats_lock);
>  	return ret;
>  }

[Severity: Medium]
Can clearing lp->reset_in_progress here without updating
lp->hw_last_counter[] cause a 32-bit unsigned underflow in
axienet_stat() and axienet_stats_update()?

At the start of __axienet_device_reset(), axienet_stats_update(lp, true)
saves the pre-reset counter values in lp->hw_last_counter[], and writing
XAXIDMA_CR_RESET_MASK resets the Axi Ethernet core and zeroes the
hardware statistics registers.  If the subsequent PhyRstCmplt poll times
out and jumps to out, the loop that resynchronizes lp->hw_last_counter[]
is skipped.  With lp->reset_in_progress cleared, axienet_stat() reads
zero from the hardware counter and subtracts the non-zero pre-reset
lp->hw_last_counter[stat] in 32-bit unsigned arithmetic, inflating the
reported 64-bit statistics by ~2^32 (and permanently folding that
underflow into lp->hw_stat_base[] on the next axienet_stats_update()):

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_stat() {
    ...
	if (lp->reset_in_progress)
		return lp->hw_stat_base[stat];

	counter = axienet_ior(lp, XAE_STATS_OFFSET + stat * 8);
	return lp->hw_stat_base[stat] + (counter - lp->hw_last_counter[stat]);
}

> @@ -1776,6 +1787,10 @@ static int axienet_open(struct net_device *ndev)
>  	axienet_lock_mii(lp);
>  	ret = axienet_device_reset(ndev);
>  	axienet_unlock_mii(lp);
> +	if (ret) {
> +		dev_err(lp->dev, "axienet_device_reset() failed: %d\n", ret);
> +		return ret;
> +	}
>  
>  	ret = phylink_fwnode_phy_connect(lp->phylink, dev_fwnode(lp->dev), 0);
>  	if (ret) {
> @@ -1806,6 +1821,8 @@ static int axienet_open(struct net_device *ndev)
>  			goto err_phy;
>  	}
>  
> +	lp->datapath_up = true;
> +
>  	return 0;
>  
>  err_free_eth_irq:
> @@ -1834,6 +1851,16 @@ static int axienet_stop(struct net_device *ndev)
>  	struct axienet_local *lp = netdev_priv(ndev);
>  	int i;
>  
> +	/* axienet_resume() calls ndo_open directly, so unlike the netdev core
> +	 * it can leave the device running after a failed open. Tearing down
> +	 * below would then stop a phylink that open already stopped and
> +	 * disconnected, and free interrupts it never requested.
> +	 */
> +	if (!lp->datapath_up)
> +		return 0;
> +
> +	lp->datapath_up = false;

[Severity: Medium]
When axienet_open() fails on the non-dmaengine path after
axienet_dma_bd_init() succeeds, does returning with lp->datapath_up set
to false leak the DMA descriptor rings and leave DMA running?

In the !lp->use_dmaengine path, axienet_device_reset() calls
axienet_dma_bd_init(), which allocates lp->tx_bd_v, lp->rx_bd_v and RX
skbs and starts DMA, and then enables XAE_OPTION_TXEN | XAE_OPTION_RXEN.
If phylink_fwnode_phy_connect() or axienet_init_legacy_dma() then fails
(or if lp->axienet_config->mac_init() fails inside
axienet_device_reset()), axienet_open() returns an error without calling
axienet_dma_stop() or axienet_dma_bd_release().  Because
lp->datapath_up remains false, axienet_stop() (including via
netif_close() in axienet_resume()) now returns early as well, leaving
hardware DMA active and leaking the coherent rings and mapped RX skbs,
which are overwritten on the next axienet_open().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114182947.2908628-1-suraj.gupta2@amd.com?part=3