[PATCH] sfc: Use acquire/release for irq_soft_enabled

[PATCH] sfc: Use acquire/release for irq_soft_enabled

[Gui-Dong Han]

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@gmail.com>
---
 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(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 7e04f115bbaa..a907303497f9 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -2143,7 +2143,7 @@ static irqreturn_t efx_ef10_msi_interrupt(int irq, void *dev_id)
 	netif_vdbg(efx, intr, efx->net_dev,
 		   "IRQ %d on CPU %d\n", irq, raw_smp_processor_id());
 
-	if (likely(READ_ONCE(efx->irq_soft_enabled))) {
+	if (likely(efx_irq_soft_enabled(efx))) {
 		/* Note test interrupts */
 		if (context->index == efx->irq_level)
 			efx->last_irq_cpu = raw_smp_processor_id();
@@ -2158,7 +2158,7 @@ static irqreturn_t efx_ef10_msi_interrupt(int irq, void *dev_id)
 static irqreturn_t efx_ef10_legacy_interrupt(int irq, void *dev_id)
 {
 	struct efx_nic *efx = dev_id;
-	bool soft_enabled = READ_ONCE(efx->irq_soft_enabled);
+	bool soft_enabled = efx_irq_soft_enabled(efx);
 	struct efx_channel *channel;
 	efx_dword_t reg;
 	u32 queues;
diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
index 00050f786cae..7885b3a5a398 100644
--- a/drivers/net/ethernet/sfc/ef100_nic.c
+++ b/drivers/net/ethernet/sfc/ef100_nic.c
@@ -333,7 +333,7 @@ static irqreturn_t ef100_msi_interrupt(int irq, void *dev_id)
 	netif_vdbg(efx, intr, efx->net_dev,
 		   "IRQ %d on CPU %d\n", irq, raw_smp_processor_id());
 
-	if (likely(READ_ONCE(efx->irq_soft_enabled))) {
+	if (likely(efx_irq_soft_enabled(efx))) {
 		/* Note test interrupts */
 		if (context->index == efx->irq_level)
 			efx->last_irq_cpu = raw_smp_processor_id();
diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index f4dc3f3f4416..d1dc8667c645 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -972,7 +972,7 @@ int efx_soft_enable_interrupts(struct efx_nic *efx)
 
 	BUG_ON(efx->state == STATE_DISABLED);
 
-	efx->irq_soft_enabled = true;
+	efx_set_irq_soft_enabled(efx, true);
 	smp_wmb();
 
 	efx_for_each_channel(channel, efx) {
@@ -1009,7 +1009,7 @@ void efx_soft_disable_interrupts(struct efx_nic *efx)
 
 	efx_mcdi_mode_poll(efx);
 
-	efx->irq_soft_enabled = false;
+	efx_set_irq_soft_enabled(efx, false);
 	smp_wmb();
 
 	if (efx->legacy_irq)
diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index 0c197b448645..e1a60e5035db 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.c
+++ b/drivers/net/ethernet/sfc/falcon/efx.c
@@ -1460,7 +1460,7 @@ static int ef4_soft_enable_interrupts(struct ef4_nic *efx)
 
 	BUG_ON(efx->state == STATE_DISABLED);
 
-	efx->irq_soft_enabled = true;
+	ef4_set_irq_soft_enabled(efx, true);
 	smp_wmb();
 
 	ef4_for_each_channel(channel, efx) {
@@ -1493,7 +1493,7 @@ static void ef4_soft_disable_interrupts(struct ef4_nic *efx)
 	if (efx->state == STATE_DISABLED)
 		return;
 
-	efx->irq_soft_enabled = false;
+	ef4_set_irq_soft_enabled(efx, false);
 	smp_wmb();
 
 	if (efx->legacy_irq)
diff --git a/drivers/net/ethernet/sfc/falcon/falcon.c b/drivers/net/ethernet/sfc/falcon/falcon.c
index fb1d19b7c419..0c0e00412689 100644
--- a/drivers/net/ethernet/sfc/falcon/falcon.c
+++ b/drivers/net/ethernet/sfc/falcon/falcon.c
@@ -449,7 +449,7 @@ static irqreturn_t falcon_legacy_interrupt_a1(int irq, void *dev_id)
 		   "IRQ %d on CPU %d status " EF4_OWORD_FMT "\n",
 		   irq, raw_smp_processor_id(), EF4_OWORD_VAL(*int_ker));
 
-	if (!likely(READ_ONCE(efx->irq_soft_enabled)))
+	if (!likely(ef4_irq_soft_enabled(efx)))
 		return IRQ_HANDLED;
 
 	/* Check to see if we have a serious error condition */
diff --git a/drivers/net/ethernet/sfc/falcon/farch.c b/drivers/net/ethernet/sfc/falcon/farch.c
index 23d507a3820d..291165db7933 100644
--- a/drivers/net/ethernet/sfc/falcon/farch.c
+++ b/drivers/net/ethernet/sfc/falcon/farch.c
@@ -1500,7 +1500,7 @@ irqreturn_t ef4_farch_fatal_interrupt(struct ef4_nic *efx)
 irqreturn_t ef4_farch_legacy_interrupt(int irq, void *dev_id)
 {
 	struct ef4_nic *efx = dev_id;
-	bool soft_enabled = READ_ONCE(efx->irq_soft_enabled);
+	bool soft_enabled = ef4_irq_soft_enabled(efx);
 	ef4_oword_t *int_ker = efx->irq_status.addr;
 	irqreturn_t result = IRQ_NONE;
 	struct ef4_channel *channel;
@@ -1592,7 +1592,7 @@ irqreturn_t ef4_farch_msi_interrupt(int irq, void *dev_id)
 		   "IRQ %d on CPU %d status " EF4_OWORD_FMT "\n",
 		   irq, raw_smp_processor_id(), EF4_OWORD_VAL(*int_ker));
 
-	if (!likely(READ_ONCE(efx->irq_soft_enabled)))
+	if (!likely(ef4_irq_soft_enabled(efx)))
 		return IRQ_HANDLED;
 
 	/* Handle non-event-queue sources */
diff --git a/drivers/net/ethernet/sfc/falcon/net_driver.h b/drivers/net/ethernet/sfc/falcon/net_driver.h
index 7ab0db44720d..c8c00bdc17f0 100644
--- a/drivers/net/ethernet/sfc/falcon/net_driver.h
+++ b/drivers/net/ethernet/sfc/falcon/net_driver.h
@@ -1305,6 +1305,18 @@ static inline netdev_features_t ef4_supported_features(const struct ef4_nic *efx
 	return net_dev->features | net_dev->hw_features;
 }
 
+static inline void ef4_set_irq_soft_enabled(struct ef4_nic *efx, bool enabled)
+{
+	/* Publish channel state before changing the IRQ handler gate. */
+	smp_store_release(&efx->irq_soft_enabled, enabled);
+}
+
+static inline bool ef4_irq_soft_enabled(struct ef4_nic *efx)
+{
+	/* Pair with ef4_set_irq_soft_enabled() before touching channels. */
+	return smp_load_acquire(&efx->irq_soft_enabled);
+}
+
 /* Get the current TX queue insert index. */
 static inline unsigned int
 ef4_tx_queue_get_insert_index(const struct ef4_tx_queue *tx_queue)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index b98c259f672d..a4a3ec657a71 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1731,6 +1731,18 @@ static inline void efx_xmit_hwtstamp_pending(struct sk_buff *skb)
 	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 }
 
+static inline void efx_set_irq_soft_enabled(struct efx_nic *efx, bool enabled)
+{
+	/* Publish channel state before changing the IRQ handler gate. */
+	smp_store_release(&efx->irq_soft_enabled, enabled);
+}
+
+static inline bool efx_irq_soft_enabled(struct efx_nic *efx)
+{
+	/* Pair with efx_set_irq_soft_enabled() before touching channels. */
+	return smp_load_acquire(&efx->irq_soft_enabled);
+}
+
 /* Get the max fill level of the TX queues on this channel */
 static inline unsigned int
 efx_channel_tx_fill_level(struct efx_channel *channel)
diff --git a/drivers/net/ethernet/sfc/siena/efx_channels.c b/drivers/net/ethernet/sfc/siena/efx_channels.c
index 1fc343598771..ace27f6ff6ae 100644
--- a/drivers/net/ethernet/sfc/siena/efx_channels.c
+++ b/drivers/net/ethernet/sfc/siena/efx_channels.c
@@ -1004,7 +1004,7 @@ static int efx_soft_enable_interrupts(struct efx_nic *efx)
 
 	BUG_ON(efx->state == STATE_DISABLED);
 
-	efx->irq_soft_enabled = true;
+	efx_set_irq_soft_enabled(efx, true);
 	smp_wmb();
 
 	efx_for_each_channel(channel, efx) {
@@ -1041,7 +1041,7 @@ static void efx_soft_disable_interrupts(struct efx_nic *efx)
 
 	efx_siena_mcdi_mode_poll(efx);
 
-	efx->irq_soft_enabled = false;
+	efx_set_irq_soft_enabled(efx, false);
 	smp_wmb();
 
 	if (efx->legacy_irq)
diff --git a/drivers/net/ethernet/sfc/siena/farch.c b/drivers/net/ethernet/sfc/siena/farch.c
index 7613d7988894..208cc499c747 100644
--- a/drivers/net/ethernet/sfc/siena/farch.c
+++ b/drivers/net/ethernet/sfc/siena/farch.c
@@ -1514,7 +1514,7 @@ irqreturn_t efx_farch_fatal_interrupt(struct efx_nic *efx)
 irqreturn_t efx_farch_legacy_interrupt(int irq, void *dev_id)
 {
 	struct efx_nic *efx = dev_id;
-	bool soft_enabled = READ_ONCE(efx->irq_soft_enabled);
+	bool soft_enabled = efx_irq_soft_enabled(efx);
 	efx_oword_t *int_ker = efx->irq_status.addr;
 	irqreturn_t result = IRQ_NONE;
 	struct efx_channel *channel;
@@ -1606,7 +1606,7 @@ irqreturn_t efx_farch_msi_interrupt(int irq, void *dev_id)
 		   "IRQ %d on CPU %d status " EFX_OWORD_FMT "\n",
 		   irq, raw_smp_processor_id(), EFX_OWORD_VAL(*int_ker));
 
-	if (!likely(READ_ONCE(efx->irq_soft_enabled)))
+	if (!likely(efx_irq_soft_enabled(efx)))
 		return IRQ_HANDLED;
 
 	/* Handle non-event-queue sources */
diff --git a/drivers/net/ethernet/sfc/siena/net_driver.h b/drivers/net/ethernet/sfc/siena/net_driver.h
index 4cf556782133..a84aca72f311 100644
--- a/drivers/net/ethernet/sfc/siena/net_driver.h
+++ b/drivers/net/ethernet/sfc/siena/net_driver.h
@@ -1624,6 +1624,18 @@ static inline void efx_xmit_hwtstamp_pending(struct sk_buff *skb)
 	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 }
 
+static inline void efx_set_irq_soft_enabled(struct efx_nic *efx, bool enabled)
+{
+	/* Publish channel state before changing the IRQ handler gate. */
+	smp_store_release(&efx->irq_soft_enabled, enabled);
+}
+
+static inline bool efx_irq_soft_enabled(struct efx_nic *efx)
+{
+	/* Pair with efx_set_irq_soft_enabled() before touching channels. */
+	return smp_load_acquire(&efx->irq_soft_enabled);
+}
+
 /* Get the max fill level of the TX queues on this channel */
 static inline unsigned int
 efx_channel_tx_fill_level(struct efx_channel *channel)
-- 
2.34.1

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

[Simon Horman]

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@gmail.com>
> ---
>  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?

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@kernel.org>

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@kernel.org> 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@gmail.com>
> > ---
> >  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@kernel.org>

Thanks for the careful review.