[Intel-wired-lan] [PATCH iwl-net 0/2] idpf: trigger SW interrupt when exiting wb_on_itr mode

[Intel-wired-lan] [PATCH iwl-net 0/2] idpf: trigger SW interrupt when exiting wb_on_itr mode

[Joshua Hay]

This patch series introduces SW triggered interrupt support for idpf,
then uses said interrupt to fix a race condition between completion
writebacks and re-enabling interrupts.

Joshua Hay (2):
  idpf: add support for SW triggered interrupts
  idpf: trigger SW interrupt when exiting wb_on_itr mode

 drivers/net/ethernet/intel/idpf/idpf_dev.c    |  3 ++
 drivers/net/ethernet/intel/idpf/idpf_txrx.c   | 30 ++++++++++++-------
 drivers/net/ethernet/intel/idpf/idpf_txrx.h   |  8 ++++-
 drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |  3 ++
 4 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.39.2

[Intel-wired-lan] [PATCH iwl-net 1/2] idpf: add support for SW triggered interrupts

[Joshua Hay]

SW triggered interrupts are guaranteed to fire after their timer
expires, unlike Tx and Rx interrupts which will only fire after the
timer expires _and_ a descriptor write back is available to be processed
by the driver.

Add the necessary fields, defines, and initializations to enable a SW
triggered interrupt in the vector's dyn_ctl register.

Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_dev.c    | 3 +++
 drivers/net/ethernet/intel/idpf/idpf_txrx.h   | 8 +++++++-
 drivers/net/ethernet/intel/idpf/idpf_vf_dev.c | 3 +++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_dev.c
index 6c913a703df6..41e4bd49402a 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c
@@ -101,6 +101,9 @@ static int idpf_intr_reg_init(struct idpf_vport *vport)
 		intr->dyn_ctl_itridx_s = PF_GLINT_DYN_CTL_ITR_INDX_S;
 		intr->dyn_ctl_intrvl_s = PF_GLINT_DYN_CTL_INTERVAL_S;
 		intr->dyn_ctl_wb_on_itr_m = PF_GLINT_DYN_CTL_WB_ON_ITR_M;
+		intr->dyn_ctl_swint_trig_m = PF_GLINT_DYN_CTL_SWINT_TRIG_M;
+		intr->dyn_ctl_sw_itridx_ena_m =
+			PF_GLINT_DYN_CTL_SW_ITR_INDX_ENA_M;
 
 		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
 					       IDPF_PF_ITR_IDX_SPACING);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
index b59aa7d8de2c..cd9a20c9cc7f 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
@@ -335,6 +335,8 @@ struct idpf_vec_regs {
  * @dyn_ctl_itridx_m: Mask for ITR index
  * @dyn_ctl_intrvl_s: Register bit offset for ITR interval
  * @dyn_ctl_wb_on_itr_m: Mask for WB on ITR feature
+ * @dyn_ctl_sw_itridx_ena_m: Mask for SW ITR index
+ * @dyn_ctl_swint_trig_m: Mask for dyn_ctl SW triggered interrupt enable
  * @rx_itr: RX ITR register
  * @tx_itr: TX ITR register
  * @icr_ena: Interrupt cause register offset
@@ -348,6 +350,8 @@ struct idpf_intr_reg {
 	u32 dyn_ctl_itridx_m;
 	u32 dyn_ctl_intrvl_s;
 	u32 dyn_ctl_wb_on_itr_m;
+	u32 dyn_ctl_sw_itridx_ena_m;
+	u32 dyn_ctl_swint_trig_m;
 	void __iomem *rx_itr;
 	void __iomem *tx_itr;
 	void __iomem *icr_ena;
@@ -418,7 +422,7 @@ struct idpf_q_vector {
 	cpumask_var_t affinity_mask;
 	__cacheline_group_end_aligned(cold);
 };
-libeth_cacheline_set_assert(struct idpf_q_vector, 112,
+libeth_cacheline_set_assert(struct idpf_q_vector, 120,
 			    24 + sizeof(struct napi_struct) +
 			    2 * sizeof(struct dim),
 			    8 + sizeof(cpumask_var_t));
@@ -452,6 +456,8 @@ struct idpf_tx_queue_stats {
 #define IDPF_ITR_IS_DYNAMIC(itr_mode) (itr_mode)
 #define IDPF_ITR_TX_DEF		IDPF_ITR_20K
 #define IDPF_ITR_RX_DEF		IDPF_ITR_20K
+/* Index used for 'SW ITR' update in DYN_CTL register */
+#define IDPF_SW_ITR_UPDATE_IDX	2
 /* Index used for 'No ITR' update in DYN_CTL register */
 #define IDPF_NO_ITR_UPDATE_IDX	3
 #define IDPF_ITR_IDX_SPACING(spacing, dflt)	(spacing ? spacing : dflt)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
index aad62e270ae4..aba828abcb17 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_vf_dev.c
@@ -101,6 +101,9 @@ static int idpf_vf_intr_reg_init(struct idpf_vport *vport)
 		intr->dyn_ctl_itridx_s = VF_INT_DYN_CTLN_ITR_INDX_S;
 		intr->dyn_ctl_intrvl_s = VF_INT_DYN_CTLN_INTERVAL_S;
 		intr->dyn_ctl_wb_on_itr_m = VF_INT_DYN_CTLN_WB_ON_ITR_M;
+		intr->dyn_ctl_swint_trig_m = VF_INT_DYN_CTLN_SWINT_TRIG_M;
+		intr->dyn_ctl_sw_itridx_ena_m =
+			VF_INT_DYN_CTLN_SW_ITR_INDX_ENA_M;
 
 		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
 					       IDPF_VF_ITR_IDX_SPACING);
-- 
2.39.2

[Intel-wired-lan] [PATCH iwl-net 2/2] idpf: trigger SW interrupt when exiting wb_on_itr mode

[Joshua Hay]

There is a race condition between exiting wb_on_itr and completion write
backs. For example, we are in wb_on_itr mode and a Tx completion is
generated by HW, ready to be written back, as we are re-enabling
interrupts:

	HW                      SW
	|                       |
	|			| idpf_tx_splitq_clean_all
	|                       | napi_complete_done
	|			|
	| tx_completion_wb 	| idpf_vport_intr_update_itr_ena_irq

That tx_completion_wb happens before the vector is fully re-enabled.
Continuing with this example, it is a UDP stream and the
tx_completion_wb is the last one in the flow (there are no rx packets).
Because the HW generated the completion before the interrupt is fully
enabled, the HW will not fire the interrupt once the timer expires and
the write back will not happen. NAPI poll won't be called.  We have
indicated we're back in interrupt mode but nothing else will trigger the
interrupt. Therefore, the completion goes unprocessed, triggering a Tx
timeout.

To mitigate this, fire a SW triggered interrupt upon exiting wb_on_itr.
This interrupt will catch the rogue completion and avoid the timeout.
Add logic to set the appropriate bits in the vector's dyn_ctl register.

Fixes: 9c4a27da0ecc ("idpf: enable WB_ON_ITR")
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 30 ++++++++++++++-------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index a8989dd98272..9558b90469c8 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3604,21 +3604,32 @@ static void idpf_vport_intr_dis_irq_all(struct idpf_vport *vport)
 /**
  * idpf_vport_intr_buildreg_itr - Enable default interrupt generation settings
  * @q_vector: pointer to q_vector
- * @type: itr index
- * @itr: itr value
  */
-static u32 idpf_vport_intr_buildreg_itr(struct idpf_q_vector *q_vector,
-					const int type, u16 itr)
+static u32 idpf_vport_intr_buildreg_itr(struct idpf_q_vector *q_vector)
 {
-	u32 itr_val;
+	u32 itr_val = q_vector->intr_reg.dyn_ctl_intena_m;
+	int type = IDPF_NO_ITR_UPDATE_IDX;
+	u16 itr = 0;
+
+	if (q_vector->wb_on_itr) {
+		/*
+		 * Trigger a software interrupt when exiting wb_on_itr, to make
+		 * sure we catch any pending write backs that might have been
+		 * missed due to interrupt state transition.
+		 */
+
+		itr_val |= q_vector->intr_reg.dyn_ctl_swint_trig_m |
+			   q_vector->intr_reg.dyn_ctl_sw_itridx_ena_m;
+		type = IDPF_SW_ITR_UPDATE_IDX;
+		itr = IDPF_ITR_20K;
+	}
 
 	itr &= IDPF_ITR_MASK;
 	/* Don't clear PBA because that can cause lost interrupts that
 	 * came in while we were cleaning/polling
 	 */
-	itr_val = q_vector->intr_reg.dyn_ctl_intena_m |
-		  (type << q_vector->intr_reg.dyn_ctl_itridx_s) |
-		  (itr << (q_vector->intr_reg.dyn_ctl_intrvl_s - 1));
+	itr_val |= (type << q_vector->intr_reg.dyn_ctl_itridx_s) |
+		   (itr << (q_vector->intr_reg.dyn_ctl_intrvl_s - 1));
 
 	return itr_val;
 }
@@ -3716,9 +3727,8 @@ void idpf_vport_intr_update_itr_ena_irq(struct idpf_q_vector *q_vector)
 	/* net_dim() updates ITR out-of-band using a work item */
 	idpf_net_dim(q_vector);
 
+	intval = idpf_vport_intr_buildreg_itr(q_vector);
 	q_vector->wb_on_itr = false;
-	intval = idpf_vport_intr_buildreg_itr(q_vector,
-					      IDPF_NO_ITR_UPDATE_IDX, 0);
 
 	writel(intval, q_vector->intr_reg.dyn_ctl);
 }
-- 
2.39.2

Re: [Intel-wired-lan] [PATCH iwl-net 2/2] idpf: trigger SW interrupt when exiting wb_on_itr mode

[Alexander Lobakin]

From: Joshua Hay <joshua.a.hay@intel.com>
Date: Mon, 25 Nov 2024 15:58:55 -0800

> There is a race condition between exiting wb_on_itr and completion write
> backs. For example, we are in wb_on_itr mode and a Tx completion is
> generated by HW, ready to be written back, as we are re-enabling
> interrupts:
> 
> 	HW                      SW
> 	|                       |
> 	|			| idpf_tx_splitq_clean_all
> 	|                       | napi_complete_done
> 	|			|
> 	| tx_completion_wb 	| idpf_vport_intr_update_itr_ena_irq
> 
> That tx_completion_wb happens before the vector is fully re-enabled.
> Continuing with this example, it is a UDP stream and the
> tx_completion_wb is the last one in the flow (there are no rx packets).
> Because the HW generated the completion before the interrupt is fully
> enabled, the HW will not fire the interrupt once the timer expires and
> the write back will not happen. NAPI poll won't be called.  We have
> indicated we're back in interrupt mode but nothing else will trigger the
> interrupt. Therefore, the completion goes unprocessed, triggering a Tx
> timeout.
> 
> To mitigate this, fire a SW triggered interrupt upon exiting wb_on_itr.
> This interrupt will catch the rogue completion and avoid the timeout.
> Add logic to set the appropriate bits in the vector's dyn_ctl register.
> 
> Fixes: 9c4a27da0ecc ("idpf: enable WB_ON_ITR")
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_txrx.c | 30 ++++++++++++++-------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index a8989dd98272..9558b90469c8 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -3604,21 +3604,32 @@ static void idpf_vport_intr_dis_irq_all(struct idpf_vport *vport)
>  /**
>   * idpf_vport_intr_buildreg_itr - Enable default interrupt generation settings
>   * @q_vector: pointer to q_vector
> - * @type: itr index
> - * @itr: itr value
>   */
> -static u32 idpf_vport_intr_buildreg_itr(struct idpf_q_vector *q_vector,
> -					const int type, u16 itr)
> +static u32 idpf_vport_intr_buildreg_itr(struct idpf_q_vector *q_vector)
>  {
> -	u32 itr_val;
> +	u32 itr_val = q_vector->intr_reg.dyn_ctl_intena_m;
> +	int type = IDPF_NO_ITR_UPDATE_IDX;
> +	u16 itr = 0;
> +
> +	if (q_vector->wb_on_itr) {
> +		/*
> +		 * Trigger a software interrupt when exiting wb_on_itr, to make
> +		 * sure we catch any pending write backs that might have been
> +		 * missed due to interrupt state transition.
> +		 */
> +

This empty newline is not needed I'd say.

> +		itr_val |= q_vector->intr_reg.dyn_ctl_swint_trig_m |
> +			   q_vector->intr_reg.dyn_ctl_sw_itridx_ena_m;
> +		type = IDPF_SW_ITR_UPDATE_IDX;
> +		itr = IDPF_ITR_20K;
> +	}
>  
>  	itr &= IDPF_ITR_MASK;
>  	/* Don't clear PBA because that can cause lost interrupts that
>  	 * came in while we were cleaning/polling
>  	 */
> -	itr_val = q_vector->intr_reg.dyn_ctl_intena_m |
> -		  (type << q_vector->intr_reg.dyn_ctl_itridx_s) |
> -		  (itr << (q_vector->intr_reg.dyn_ctl_intrvl_s - 1));
> +	itr_val |= (type << q_vector->intr_reg.dyn_ctl_itridx_s) |
> +		   (itr << (q_vector->intr_reg.dyn_ctl_intrvl_s - 1));
>  
>  	return itr_val;
>  }
> @@ -3716,9 +3727,8 @@ void idpf_vport_intr_update_itr_ena_irq(struct idpf_q_vector *q_vector)
>  	/* net_dim() updates ITR out-of-band using a work item */
>  	idpf_net_dim(q_vector);
>  
> +	intval = idpf_vport_intr_buildreg_itr(q_vector);
>  	q_vector->wb_on_itr = false;
> -	intval = idpf_vport_intr_buildreg_itr(q_vector,
> -					      IDPF_NO_ITR_UPDATE_IDX, 0);

Is there a reason for changing the order of these two?

>  
>  	writel(intval, q_vector->intr_reg.dyn_ctl);
>  }

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH iwl-net 2/2] idpf: trigger SW interrupt when exiting wb_on_itr mode

[Hay, Joshua A]

> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Kubiak, Michal <michal.kubiak@intel.com>;
> Chittim, Madhu <madhu.chittim@intel.com>; netdev@vger.kernel.org
> Subject: Re: [Intel-wired-lan][PATCH iwl-net 2/2] idpf: trigger SW interrupt
> when exiting wb_on_itr mode
> 
> From: Joshua Hay <joshua.a.hay@intel.com>
> Date: Mon, 25 Nov 2024 15:58:55 -0800
> 
> > There is a race condition between exiting wb_on_itr and completion write
> > backs. For example, we are in wb_on_itr mode and a Tx completion is
> > generated by HW, ready to be written back, as we are re-enabling
> > interrupts:
> >
> > 	HW                      SW
> > 	|                       |
> > 	|			| idpf_tx_splitq_clean_all
> > 	|                       | napi_complete_done
> > 	|			|
> > 	| tx_completion_wb 	| idpf_vport_intr_update_itr_ena_irq
> >
> > That tx_completion_wb happens before the vector is fully re-enabled.
> > Continuing with this example, it is a UDP stream and the
> > tx_completion_wb is the last one in the flow (there are no rx packets).
> > Because the HW generated the completion before the interrupt is fully
> > enabled, the HW will not fire the interrupt once the timer expires and
> > the write back will not happen. NAPI poll won't be called.  We have
> > indicated we're back in interrupt mode but nothing else will trigger the
> > interrupt. Therefore, the completion goes unprocessed, triggering a Tx
> > timeout.
> >
> > To mitigate this, fire a SW triggered interrupt upon exiting wb_on_itr.
> > This interrupt will catch the rogue completion and avoid the timeout.
> > Add logic to set the appropriate bits in the vector's dyn_ctl register.
> >
> > Fixes: 9c4a27da0ecc ("idpf: enable WB_ON_ITR")
> > Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> > ---
> >  drivers/net/ethernet/intel/idpf/idpf_txrx.c | 30 ++++++++++++++-------
> >  1 file changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > index a8989dd98272..9558b90469c8 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > @@ -3604,21 +3604,32 @@ static void idpf_vport_intr_dis_irq_all(struct
> idpf_vport *vport)
> >  /**
> >   * idpf_vport_intr_buildreg_itr - Enable default interrupt generation settings
> >   * @q_vector: pointer to q_vector
> > - * @type: itr index
> > - * @itr: itr value
> >   */
> > -static u32 idpf_vport_intr_buildreg_itr(struct idpf_q_vector *q_vector,
> > -					const int type, u16 itr)
> > +static u32 idpf_vport_intr_buildreg_itr(struct idpf_q_vector *q_vector)
> >  {
> > -	u32 itr_val;
> > +	u32 itr_val = q_vector->intr_reg.dyn_ctl_intena_m;
> > +	int type = IDPF_NO_ITR_UPDATE_IDX;
> > +	u16 itr = 0;
> > +
> > +	if (q_vector->wb_on_itr) {
> > +		/*
> > +		 * Trigger a software interrupt when exiting wb_on_itr, to make
> > +		 * sure we catch any pending write backs that might have been
> > +		 * missed due to interrupt state transition.
> > +		 */
> > +
> 
> This empty newline is not needed I'd say.

Will fix.

> 
> > +		itr_val |= q_vector->intr_reg.dyn_ctl_swint_trig_m |
> > +			   q_vector->intr_reg.dyn_ctl_sw_itridx_ena_m;
> > +		type = IDPF_SW_ITR_UPDATE_IDX;
> > +		itr = IDPF_ITR_20K;
> > +	}
> >
> >  	itr &= IDPF_ITR_MASK;
> >  	/* Don't clear PBA because that can cause lost interrupts that
> >  	 * came in while we were cleaning/polling
> >  	 */
> > -	itr_val = q_vector->intr_reg.dyn_ctl_intena_m |
> > -		  (type << q_vector->intr_reg.dyn_ctl_itridx_s) |
> > -		  (itr << (q_vector->intr_reg.dyn_ctl_intrvl_s - 1));
> > +	itr_val |= (type << q_vector->intr_reg.dyn_ctl_itridx_s) |
> > +		   (itr << (q_vector->intr_reg.dyn_ctl_intrvl_s - 1));
> >
> >  	return itr_val;
> >  }
> > @@ -3716,9 +3727,8 @@ void idpf_vport_intr_update_itr_ena_irq(struct
> idpf_q_vector *q_vector)
> >  	/* net_dim() updates ITR out-of-band using a work item */
> >  	idpf_net_dim(q_vector);
> >
> > +	intval = idpf_vport_intr_buildreg_itr(q_vector);
> >  	q_vector->wb_on_itr = false;
> > -	intval = idpf_vport_intr_buildreg_itr(q_vector,
> > -					      IDPF_NO_ITR_UPDATE_IDX, 0);
> 
> Is there a reason for changing the order of these two?

Yes. The SW triggered interrupt is only fired if we were in wb_on_itr mode. We cannot clear it until after buildreg sets the dyn_ctl value accordingly. Otherwise, the SW interrupt will never be used

> 
> >
> >  	writel(intval, q_vector->intr_reg.dyn_ctl);
> >  }
> 
> Thanks,
> Olek

Thanks,
Josh

Re: [Intel-wired-lan] [PATCH iwl-net 1/2] idpf: add support for SW triggered interrupts

[Singh, Krishneil K]

> -----Original Message-----
> From: Joshua Hay <joshua.a.hay@intel.com>
> Sent: Monday, November 25, 2024 3:59 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Kubiak, Michal
> <michal.kubiak@intel.com>; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; Chittim, Madhu
> <madhu.chittim@intel.com>; netdev@vger.kernel.org; Hay, Joshua A
> <joshua.a.hay@intel.com>
> Subject: [Intel-wired-lan][PATCH iwl-net 1/2] idpf: add support for SW
> triggered interrupts
> 
> SW triggered interrupts are guaranteed to fire after their timer
> expires, unlike Tx and Rx interrupts which will only fire after the
> timer expires _and_ a descriptor write back is available to be processed
> by the driver.
> 
> Add the necessary fields, defines, and initializations to enable a SW
> triggered interrupt in the vector's dyn_ctl register.
> 
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_dev.c    | 3 +++
>  drivers/net/ethernet/intel/idpf/idpf_txrx.h   | 8 +++++++-
>  drivers/net/ethernet/intel/idpf/idpf_vf_dev.c | 3 +++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c
> b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> index 6c913a703df6..41e4bd49402a 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-net 2/2] idpf: trigger SW interrupt when exiting wb_on_itr mode

[Singh, Krishneil K]

> -----Original Message-----
> From: Hay, Joshua A <joshua.a.hay@intel.com>
> Sent: Tuesday, December 3, 2024 9:53 AM
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Kubiak, Michal <michal.kubiak@intel.com>;
> Chittim, Madhu <madhu.chittim@intel.com>; netdev@vger.kernel.org
> Subject: RE: [Intel-wired-lan][PATCH iwl-net 2/2] idpf: trigger SW interrupt
> when exiting wb_on_itr mode
> 
> > From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@intel.com>; Kubiak, Michal
> <michal.kubiak@intel.com>;
> > Chittim, Madhu <madhu.chittim@intel.com>; netdev@vger.kernel.org
> > Subject: Re: [Intel-wired-lan][PATCH iwl-net 2/2] idpf: trigger SW interrupt
> > when exiting wb_on_itr mode
> >
> > From: Joshua Hay <joshua.a.hay@intel.com>
> > Date: Mon, 25 Nov 2024 15:58:55 -0800
> >
> > > There is a race condition between exiting wb_on_itr and completion write
> > > backs. For example, we are in wb_on_itr mode and a Tx completion is
> > > generated by HW, ready to be written back, as we are re-enabling
> > > interrupts:
> > >
> > > 	HW                      SW
> > > 	|                       |
> > > 	|			| idpf_tx_splitq_clean_all
> > > 	|                       | napi_complete_done
> > > 	|			|
> > > 	| tx_completion_wb 	| idpf_vport_intr_update_itr_ena_irq
> > >
> > > That tx_completion_wb happens before the vector is fully re-enabled.
> > > Continuing with this example, it is a UDP stream and the
> > > tx_completion_wb is the last one in the flow (there are no rx packets).
> > > Because the HW generated the completion before the interrupt is fully
> > > enabled, the HW will not fire the interrupt once the timer expires and
> > > the write back will not happen. NAPI poll won't be called.  We have
> > > indicated we're back in interrupt mode but nothing else will trigger the
> > > interrupt. Therefore, the completion goes unprocessed, triggering a Tx
> > > timeout.
> > >
> > > To mitigate this, fire a SW triggered interrupt upon exiting wb_on_itr.
> > > This interrupt will catch the rogue completion and avoid the timeout.
> > > Add logic to set the appropriate bits in the vector's dyn_ctl register.
> > >
> > > Fixes: 9c4a27da0ecc ("idpf: enable WB_ON_ITR")
> > > Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> > > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/idpf/idpf_txrx.c | 30 ++++++++++++++-------
> > >  1 file changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > > index a8989dd98272..9558b90469c8 100644
> > > --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > > +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> > > @@ -3604,21 +3604,32 @@ static void idpf_vport_intr_dis_irq_all(struct


Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>