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

[Ding Hui]

Hi Thomas,
 
Thank you for reproducing the issue on the STM32MP215-DK and for
backporting and testing the series. It is helpful to have confirmation
on another platform. I understand that the backport required changes,
so I will not describe this as testing the 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;`
>
Regarding the error path, agreed. I have replaced the duplicated cleanup
with "goto error_unlock" in my local revision.

> > @@ -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.
>
Extracting a stmmac_resume_descriptors() helper also makes sense. I will
look at organizing the work per TX and RX queue.

> > +	/* 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 ?
>
Regarding moving stmmac_clear_descriptors() after restoring the buffer
addresses, you are right that the GMAC4/XGMAC init_rx_desc callbacks do
not overwrite those addresses. For a fully populated RX ring, doing the
initialization afterwards could replace the separate re-arm pass.
 
The complication is the partially populated AF_XDP ring. In v5,
stmmac_clear_descriptors() unconditionally sets OWN on every RX
descriptor. Moving that call after buffer restoration would therefore
also give descriptors without an XSK buffer to DMA.
 
The v5 ordering was intended to leave those slots CPU-owned afterwards,
but further review exposed problems with that approach: the legacy
clear callbacks do not clear OWN, and the unfilled slots also need to
be represented correctly in the software refill state.
 
I am addressing those issues in the next revision. I agree that the
better ordering is to restore addresses and chain pointers first, then
initialize/re-arm only the valid descriptors, while preserving the
legacy descriptor control fields and the existing RX watchdog policy.
Unfilled XSK descriptors must remain CPU-owned and available for a
later refill.

Thanks again for the suggestions and the testing feedback.
 
Best regards,
Ding Hui