Re: [PATCH net v5 2/2] net: stmmac: restore RX descriptor buffer addresses on resume

[Thomas BOURGOIN]

Hello,

I reproduced this issue on the STM32MP215-DK. I’m using kernel v6.18.
I fixed it similarly to your v5 patch set (cf. 
https://lore.kernel.org/all/20261027150556.51209-2-thomas.bourgoin@xxxxxxxxxxx/).

I am limited to testing the low-power sequence on the STM32MP215-DK 
because we are currently upstreaming.
I backported your series to the STM32 fork based on v6.18, but I had to 
modify it because of some compilation issues, so I cannot say I tested 
your series as-is.

Overall, I’m okay with your patch, I have a few suggestions:

> +			stmmac_legacy_serdes_power_down(priv);
> +			mutex_unlock(&priv->lock);
> +			rtnl_unlock();
> +			return ret;
replace these 4 lines with: `goto error_unlock;`

> @@ -8503,6 +8577,18 @@ int stmmac_resume(struct device *dev)
>   	stmmac_free_tx_skbufs(priv);
>   	stmmac_clear_descriptors(priv, &priv->dma_conf);
>
> +	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
> +		ret = stmmac_reinit_rx_descriptors(priv, &priv->dma_conf, queue);
> +		if (ret) {
> +			netdev_err(ndev, "Failed to restore RX queue %u: %d\n",
> +				   queue, ret);
> +			stmmac_legacy_serdes_power_down(priv);
> +			mutex_unlock(&priv->lock);
> +			rtnl_unlock();
> +			return ret;
> +		}
> +	}
> +
>   	ret = stmmac_hw_setup(ndev);
>   	if (ret < 0) {
>   		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);

You could move the calls to `stmmac_free_tx_skbufs()`, 
`stmmac_clear_descriptors()`, and your loop into a dedicated function, 
perhaps something like `stmmac_resume_descriptors()`.

In my PR, I also factorized the loops so there is one for TX and one for 
RX, instead of one loop for TX and RX in `stmmac_clear_descriptors()`, 
one loop for TX in `stmmac_free_tx_skbufs()`, and one loop for RX to 
rebuild them.
By doing this, I managed to remove the `stmmac_free_tx_skbufs()` 
function. It changes the PM sequence a bit, but I think it is okay. I’ll 
let you decide whether you want to do it or not.

> +	/* GMAC4/XGMAC secondary address writes overwrite the OWN bit. */
> +	dma_wmb();
> +	for (i = 0; i < dma_conf->dma_rx_size; i++) {
> +		buf = &rx_q->buf_pool[i];
> +		if (rx_q->xsk_pool && !buf->xdp)
> +			continue;
> +
> +		p = stmmac_get_rx_desc(priv, rx_q, i);
> +		stmmac_set_rx_owner(priv, p, false);
> +	}

Since on GMAC4/XGMAC we need to set the owner bit after writing the 
secondary address, why do we need to call `stmmac_clear_descriptors()` 
before reinitializing the RX descriptors?
Since `stmmac_clear_descriptors()` is a loop that calls 
`stmmac_init_rx_desc()`, and I verified that all `.init_rx_desc` ops set 
the OWN bit to 1, could we call `stmmac_clear_descriptors()` after 
rebuilding the RX descriptors instead and remove this loop ?

Best regards,

Thomas