Re: [PATCH] sfc: Use acquire/release for irq_soft_enabled

[Jakub Kicinski]

On Wed, 3 Jun 2026 21:28:02 +0100 Edward Cree wrote:
> On 03/06/2026 19:20, Jakub Kicinski wrote:
> > On Thu, 28 May 2026 17:28:38 +0800 Gui-Dong Han wrote:  
> >> Use release stores when updating the gate and acquire loads in interrupt
> >> handlers before touching channels. Keep the existing smp_wmb() after gate
> >> updates, preserving the current ordering with event queue start and stop.  
> > 
> > Why are you keeping the smp_wmb() tho? You don't have to repeat what
> > the patch does. The goal of the commit message is to explain the why.
> > 
> > Ed, WDYT?  
> 
> I think... that my head hurts from trying to understand this.
> What I particularly don't understand is why it wouldn't suffice to just
>  put smp_rmb() after every read of irq_soft_enabled, pairing with the
>  existing smp_wmb().

Exactly..

> And, conversely, why any of this works at all — what stops an interrupt
>  handler passing the gate, because the write to irq_soft_enabled has
>  propagated to its CPU, but the writes to channel pointers haven't?
>  Naïvely I'd expect the smp_wmb() in efx_soft_enable_interrupts() to
>  come _before_ the write to irq_soft_enabled.
> (The smp_wmb() in efx_soft_disable_interrupts(), I'm not sure exactly
>  what that pairs with, since the ordering that matters there is between
>  the irq_soft_enabled=false write and the synchronize_irq().)
> 
> Anyway, this code predates my involvement with sfc and I've never had
>  to touch it, so frankly I don't know any more here than anyone else,
>  and if a memory-models expert reviews it and says 'this is needed and
>  correct', that would mean more than a review tag from me.
> That's not a cop-out "I won't review this", rather I'm saying that if
>  Gui-Dong can't convince me because I'm too dumb, but can convince you,
>  then you don't need to wait for me to show up and add my tag.

I haven't dug into the code too much but FWIW having a store_release
barrier which flips something _both_ to true and false strikes me as
highly suspicious. So the patch needs a much better commit description
at the very least.

Thanks for the prompt reply!