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

[Thomas BOURGOIN <thomas.bourgoin@foss.st.com>]

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@foss.st.com/).

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