From: Joshua Hay <joshua.a....@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.chit...@intel.com>
> Signed-off-by: Joshua Hay <joshua.a....@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
