Re: [PATCH] net: stmmac: Avoid freeing and re-requesting IRQ during XDP set prog

[Daniel Thompson]

On Tue, Jul 07, 2026 at 02:30:44PM +0200, Andrew Lunn wrote:
> On Tue, Jul 07, 2026 at 10:48:50AM +0100, Daniel Thompson wrote:
> > On Mon, Jul 06, 2026 at 05:17:58PM +0100, Daniel Thompson wrote:
> > > Currently stmmac will run a full cycle of IRQ tear down and setup when
> > > setting up a new XDP program. This makes tuning TSN systems difficult
> > > because whenever a new XDP program is installed then the irq threads will
> > > be stopped and restarted which will undo any thread tuning.
> > >
> > > The problem is avoided by removing stmmac_free_irq()/stmmac_request_irq()
> > > from stmmac_xdp_release()/stmmac_xdp_open().
> > >
> > > stmmac_free_irq() implicitly synchronizes interrupts and, with that
> > > removed, I was unable to prove that later actions in
> > > stmmac_xdp_release() are safe when there are concurrent interrupts. To
> > > avoid problems let's also move the code to disable DMA interrupts earlier
> > > in the sequence and explicitly sync the interrupts handler(s).
> > >
> > > Signed-off-by: Daniel Thompson <daniel@xxxxxxxxxxxx>
> > > <snip>
> > > @@ -7156,10 +7201,8 @@ int stmmac_xdp_open(struct net_device *dev)
> > >  	stmmac_reset_queues_param(priv);
> > >
> > >  	/* DMA CSR Channel configuration */
> > > -	for (chan = 0; chan < dma_csr_ch; chan++) {
> > > +	for (chan = 0; chan < dma_csr_ch; chan++)
> > >  		stmmac_init_chan(priv, priv->ioaddr, priv->plat->dma_cfg, chan);
> > > -		stmmac_disable_dma_irq(priv, priv->ioaddr, chan, 1, 1);
> > > -	}
> >
> > Sashiko picked up that stmmac_init_chan() has the effect of enabling DMA
> > irqs, making this code unsafe.
> >
> > Given stmmac_xdp_open() can only be called on a running interface (and that
> > stmmac_xdp_open() explicitly sets sph) then re-initializing with stmmac_init_chan()
> > should have no effect and we can drop that as well.
>
> Something i like to see is symmetry. In theory, stmmac_xdp_open() and
> stmmac_xdp_release() should be mirrors of each other. But i don't see
> anything like the opposite of stmmac_init_chan() in
> stmmac_xdp_release(). So i can well understand that something is
> wrong.
>
> Maybe rather than just fixing this IRQ issue, you can look at both
> functions and see about making them symmetric? I've no idea what that
> actually entails, it could be a can of worms, which is not so uncommon
> with stmmmac.

After review I found that moving the disable to stmmac_xdp_release()
does and removing the call the stmmac_init_chan() gets things most of
the way there. After these changes every action in stmmac_xdp_release()
can now be traced by to corresponding restore code in stmmac_xdp_open().

It's not fully symmetric because activity in open does not appear in
reverse order compared to release. However it definitely moves things
in the right direction because the new stmmac_disable_all_dma_irq()
function pairs nicely with stmmac_enable_all_dma_irq().


Daniel.