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

[Gui-Dong Han]

On Wed, Jun 3, 2026 at 8:06 PM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> On Thu, May 28, 2026 at 05:28:38PM +0800, Gui-Dong Han wrote:
> > irq_soft_enabled is a lockless gate for interrupt handlers. When clear,
> > handlers must only acknowledge interrupts and must not touch channel state.
> >
> > Channel reallocation disables this gate, swaps and initializes channel
> > pointers, frees old channels, and then enables the gate again. READ_ONCE()
> > does not provide an acquire barrier, so an IRQ handler can observe the gate
> > as enabled while still seeing stale channel pointers or channel state.
> >
> > 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.
> >
> > Add small helpers to document the ordering and avoid repeating barrier
> > comments at every caller. Without the comments, checkpatch warns about
> > memory barriers without comments.
> >
> > Fixes: d829118705f8 ("sfc: Rework IRQ enable/disable")
> > Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
> > Fixes: 5a6681e22c14 ("sfc: separate out SFC4000 ("Falcon") support into new sfc-falcon driver")
> > Fixes: 51b35a454efd ("sfc: skeleton EF100 PF driver")
> > Fixes: 6e173d3b4af9 ("sfc: Copy shared files needed for Siena (part 1)")
> > Signed-off-by: Gui-Dong Han <hanguidong02@xxxxxxxxx>
> > ---
> >  drivers/net/ethernet/sfc/ef10.c               |  4 ++--
> >  drivers/net/ethernet/sfc/ef100_nic.c          |  2 +-
> >  drivers/net/ethernet/sfc/efx_channels.c       |  4 ++--
> >  drivers/net/ethernet/sfc/falcon/efx.c         |  4 ++--
> >  drivers/net/ethernet/sfc/falcon/falcon.c      |  2 +-
> >  drivers/net/ethernet/sfc/falcon/farch.c       |  4 ++--
> >  drivers/net/ethernet/sfc/falcon/net_driver.h  | 12 ++++++++++++
> >  drivers/net/ethernet/sfc/net_driver.h         | 12 ++++++++++++
> >  drivers/net/ethernet/sfc/siena/efx_channels.c |  4 ++--
> >  drivers/net/ethernet/sfc/siena/farch.c        |  4 ++--
> >  drivers/net/ethernet/sfc/siena/net_driver.h   | 12 ++++++++++++
> >  11 files changed, 50 insertions(+), 14 deletions(-)
>
> I'm not sure if it would be worth breaking this up, say on a per-driver
> basis. My main thinking here is that it might assist in backporting.
>
> But, OTOH, I wonder if this should be targeted at net-next without
> the Fixes tag - can this manifest in an actual bug that effects users?

I found this by code inspection rather than with a dynamic reproducer.
This kind of reordering is realistic on weakly ordered architectures such
as ARM.  So I think it can affect users, even though reproducing and
diagnosing it dynamically would be difficult.

I was not specifically aiming for stable backports.  I also think this
does not clearly meet the stable rules, which say that a patch "must
either fix a real bug that bothers people or just add a device ID".  This
was found by audit and I do not have a user-visible reproducer, so I did
not add Cc: stable.  The Fixes tags are mainly there to document where
the lockless pattern was introduced and to make the patch complete.

If you prefer, I can split this on a per-driver basis to make backporting
easier, or respin it for net-next without the Fixes tags.

>
> FTR, there is an Ai-generated review of this patch available on sashiko.dev.
> However, I believe that all the issues flagged there are pre-existing and
> can be examined in the context of possible follow-up. I do not believe they
> should block progress of this patch.
>
> Overall, this looks good to me.
>
> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>

Thanks for the careful review.