> From: Lobakin, Aleksander <[email protected]> > Cc: [email protected]; Kitszel, Przemyslaw > <[email protected]>; Kubiak, Michal <[email protected]>; > Chittim, Madhu <[email protected]>; [email protected] > Subject: Re: [Intel-wired-lan][PATCH iwl-net 2/2] idpf: trigger SW interrupt > when exiting wb_on_itr mode > > From: Joshua Hay <[email protected]> > 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 <[email protected]> > > Signed-off-by: Joshua Hay <[email protected]> > > --- > > 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
