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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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