[PATCH] net/ixgbe: don't create a delayed interrupt handler if one already exists

[PATCH] net/ixgbe: don't create a delayed interrupt handler if one already exists

[edwin.brossette]

From: Edwin Brossette <edwin.brossette@6wind.com>

Since link state may need some time to stabilize after a link state
change, we cannot update the link state right after one occurs. So link
state change interrupts (lsc) are handled after a delay. To do this, an
alarm to call a delayed handler is programmed. This delayed handler is
tasked with updating the link after a variable delay of one to four
seconds which should be enough time for the link state to become stable
again.

However, a problem can occur with some models of network cards. For
example, ixgbe_mac_X550EM_x may trigger this interrupt twice because
another interrupt signal is received on the General Purpose Interrupt
pin SPD0, which has the same interrupt handler. In such a case, the
delayed interrupt handler would be programmed to be executed twice.

Since we save the original interrupt mask value to restore it after the
delayed handler is done with its work, we end up overwritting its value
after the second alarm is programmed. Even worse: when restoring it the
first time, the saved original mask variable is reset to 0, so we end up
completely disabling all interrupts when trying to restore this mask
after the second time the delayed handler is executed.

Add a check on the interrupt mask value when programming the alarm for
the delayed handler. If the bit for lsc interrupts is unset, it means an
alarm was already programmed for the delayed handler. In this case, skip
the alarm creation.

Fixes: 9b667210700e ("net/ixgbe: fix blocked interrupts")
Cc: stable@dpdk.org

Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index c61c52b2966b..52cafcbc965f 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4667,14 +4667,20 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev)
 			timeout = IXGBE_LINK_DOWN_CHECK_TIMEOUT;
 
 		ixgbe_dev_link_status_print(dev);
-		if (rte_eal_alarm_set(timeout * 1000,
-				      ixgbe_dev_interrupt_delayed_handler, (void *)dev) < 0)
-			PMD_DRV_LOG(ERR, "Error setting alarm");
-		else {
-			/* remember original mask */
-			intr->mask_original = intr->mask;
-			/* only disable lsc interrupt */
-			intr->mask &= ~IXGBE_EIMS_LSC;
+
+		/* Don't program delayed handler if LSC interrupt is disabled.
+		 * It means one is already programmed.
+		 */
+		if (intr->mask & IXGBE_EIMS_LSC){
+			if (rte_eal_alarm_set(timeout * 1000,
+					      ixgbe_dev_interrupt_delayed_handler, (void *)dev) < 0)
+				PMD_DRV_LOG(ERR, "Error setting alarm");
+			else {
+				/* remember original mask */
+				intr->mask_original = intr->mask;
+				/* only disable lsc interrupt */
+				intr->mask &= ~IXGBE_EIMS_LSC;
+			}
 		}
 	}
 
-- 
2.35.0.4.g44a5d4affccf

Re: [PATCH] net/ixgbe: don't create a delayed interrupt handler if one already exists

[Burakov, Anatoly]

On 4/18/2024 3:53 PM, edwin.brossette@6wind.com wrote:
> From: Edwin Brossette <edwin.brossette@6wind.com>
> 
> Since link state may need some time to stabilize after a link state
> change, we cannot update the link state right after one occurs. So link
> state change interrupts (lsc) are handled after a delay. To do this, an
> alarm to call a delayed handler is programmed. This delayed handler is
> tasked with updating the link after a variable delay of one to four
> seconds which should be enough time for the link state to become stable
> again.
> 
> However, a problem can occur with some models of network cards. For
> example, ixgbe_mac_X550EM_x may trigger this interrupt twice because
> another interrupt signal is received on the General Purpose Interrupt
> pin SPD0, which has the same interrupt handler. In such a case, the
> delayed interrupt handler would be programmed to be executed twice.
> 
> Since we save the original interrupt mask value to restore it after the
> delayed handler is done with its work, we end up overwritting its value
> after the second alarm is programmed. Even worse: when restoring it the
> first time, the saved original mask variable is reset to 0, so we end up
> completely disabling all interrupts when trying to restore this mask
> after the second time the delayed handler is executed.
> 
> Add a check on the interrupt mask value when programming the alarm for
> the delayed handler. If the bit for lsc interrupts is unset, it means an
> alarm was already programmed for the delayed handler. In this case, skip
> the alarm creation.
> 
> Fixes: 9b667210700e ("net/ixgbe: fix blocked interrupts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
> ---
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH] net/ixgbe: don't create a delayed interrupt handler if one already exists

[Bruce Richardson]

On Thu, May 16, 2024 at 01:41:22PM +0200, Burakov, Anatoly wrote:
> On 4/18/2024 3:53 PM, edwin.brossette@6wind.com wrote:
> > From: Edwin Brossette <edwin.brossette@6wind.com>
> > 
> > Since link state may need some time to stabilize after a link state
> > change, we cannot update the link state right after one occurs. So link
> > state change interrupts (lsc) are handled after a delay. To do this, an
> > alarm to call a delayed handler is programmed. This delayed handler is
> > tasked with updating the link after a variable delay of one to four
> > seconds which should be enough time for the link state to become stable
> > again.
> > 
> > However, a problem can occur with some models of network cards. For
> > example, ixgbe_mac_X550EM_x may trigger this interrupt twice because
> > another interrupt signal is received on the General Purpose Interrupt
> > pin SPD0, which has the same interrupt handler. In such a case, the
> > delayed interrupt handler would be programmed to be executed twice.
> > 
> > Since we save the original interrupt mask value to restore it after the
> > delayed handler is done with its work, we end up overwritting its value
> > after the second alarm is programmed. Even worse: when restoring it the
> > first time, the saved original mask variable is reset to 0, so we end up
> > completely disabling all interrupts when trying to restore this mask
> > after the second time the delayed handler is executed.
> > 
> > Add a check on the interrupt mask value when programming the alarm for
> > the delayed handler. If the bit for lsc interrupts is unset, it means an
> > alarm was already programmed for the delayed handler. In this case, skip
> > the alarm creation.
> > 
> > Fixes: 9b667210700e ("net/ixgbe: fix blocked interrupts")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
> > ---
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
Applied to dpdk-next-net-intel

Thanks,
/Bruce