On Fri, May 15, 2026 at 11:24:01AM +0200, Paolo Abeni wrote:
> > You put this after whole section, but I assume you are talking about
> > synchronize_rcu()? Because currently there are 0 checks and it is pure race
> > condition. synchronize_rcu() does secure it in some way, but you are correct,
> > proper ensuring that napi has finished is napi_synchronize().
> > Will fix in v2.
> >>
> >>> @@ -5267,6 +5279,9 @@ static int stmmac_xdp_xmit_back(struct stmmac_priv *priv,
> >>>  	if (unlikely(!xdpf))
> >>>  		return STMMAC_XDP_CONSUMED;
> >>>  
> >>> +	if (unlikely(test_bit(STMMAC_DOWN, &priv->state)))
> >>> +		return -ENETDOWN;
> >>
> >> Sashiko noted here you should return STMMAC_XDP_CONSUMED
> >>
> >> /P
> > Seems good, will fix in v2.
> 
> If you use napi_synchronize(), I think you can avoid setting STMMAC_DOWN
> and testing it in the fast path: the run-to-completion after irq
> disabling should ensure that no tx could happen after that
> napi_synchronize() completes.
> 
> Side note: it's not clear to me where/when irq disabling take place?!?
> 
> /P
>
STMMAC_DOWN is currently in strange place where it is only used in XDP and is
only ever set/cleared in stmmac_reset_subtask(), and is only applied in
one path. So for starters we wanted to unify it accross XDP paths.
But it could probably be replaced by detecting netlink state,
but that is talk for different patch.

But that aside, in stmmac_disable_all_queues() there is already measure for
XDP to drain which is after phylink stop but before stopping IRQ's in
stmmac_free_irq() (or rather freeing them, which should stop them,
unless shared), but it is not enough it seems, as we managed to crash in
test environment.

So there is a moment between phylink_stop() and stop_all_dma() where
packets are processed, even though there was synchronize_rcu() inbetween.
Using napi_synchronize() or synchronize_rcu() AFTER stop_all_dma() should be
sufficient, but we will need to retest as you said.

Question is whether we actually care about scraps of data when
killing interface.

But that generally still makes STMMAC_DOWN flag almost completely useless.
I mean, it should still be added to other paths, but doesn't do much, other
than during abnormal behavior.

Meanwhile stmmac_xdp_release() handles above in different order and should
be no issue there.

So we will retest.

BR
Jakub Raczynski