netdev - RE: [Intel-wired-lan][PATCH iwl-net 2/2] idpf: trigger SW interrupt when exiting wb_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM4PR11MB6502760769BC51BBBF7D7D75D4362@DM4PR11MB6502.namprd11.prod.outlook.com>
Date: Tue, 3 Dec 2024 17:52:30 +0000
From: "Hay, Joshua A" <joshua.a.hay@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>, "Kubiak, Michal"
	<michal.kubiak@...el.com>, "Chittim, Madhu" <madhu.chittim@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.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@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>; Kubiak, Michal <michal.kubiak@...el.com>;
> Chittim, Madhu <madhu.chittim@...el.com>; netdev@...r.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@...el.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@...el.com>
> > Signed-off-by: Joshua Hay <joshua.a.hay@...el.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