Re: [PATCH net 3/3] net: stmmac: stop DMA when IRQ registration fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/stmmac: stmmac_xdp_set_prog() ignores stmmac_xdp_open() failure
- [Medium] net: stmmac: MAC Rx/Tx left enabled on open error paths
--

commit 7ffd7d4ee658f5d19fab03ed54e7532669eeea56
Author: Karl Mehltretter <kmehltretter@gmail.com>

net: stmmac: stop DMA when IRQ registration fails

Both __stmmac_open() and stmmac_xdp_open() start DMA before requesting
IRQs. If IRQ registration fails, their error paths release descriptors
and packet buffers while the DMA channels remain active.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8fd7d21730911..5c4df26a64bd6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4206,6 +4206,7 @@ static int __stmmac_open(struct net_device *dev,
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
>  
> +	stmmac_stop_all_dma(priv);

[Severity: Medium]
In this error path, phylink_stop() is called just above this snippet, but it
only invokes the driver's mac_link_down callback if the link is considered
logically UP.

If the link was never UP, the MAC hardware is left active since it was
enabled earlier during stmmac_hw_setup().

Should the MAC be explicitly disabled here so it doesn't remain permanently
active?

>  	stmmac_release_ptp(priv);
>  init_error:
>  	return ret;
> @@ -7259,6 +7260,7 @@ int stmmac_xdp_open(struct net_device *dev)
>  irq_error:
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> +	stmmac_stop_all_dma(priv);

[Severity: Medium]
Earlier in stmmac_xdp_open(), the MAC is explicitly enabled with
stmmac_mac_set(..., true).

However, this error cleanup path lacks the corresponding
stmmac_mac_set(..., false).

Will the MAC continue to process incoming packets and potentially overflow
the internal MTL FIFOs while the DMA and IRQs are stopped?

[Severity: High]
If we hit this error path, stmmac_xdp_open() cleans up its descriptors
and returns an error.

However, its caller stmmac_xdp_set_prog() ignores this return value:

drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
    ...
	if (if_running && need_update)
		stmmac_xdp_open(dev);

	if (prog)
		xdp_features_set_redirect_target(dev, false);

	return 0;
}

If an error occurs here, does this leave the interface functionally dead
while still reporting success to userspace?

>  
>  init_error:
>  	free_dma_desc_resources(priv, &priv->dma_conf);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791958735.git.kmehltretter@gmail.com?part=3