Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

From: Leon Romanovsky <[email-masked]>
Date: Thu, 6 Jul 2023 19:40:39 +0300

On Thu, Jul 06, 2023 at 12:43:33PM +0000, Zulkifli, Muhammad Husaini wrote:
> Dear Leon,
> 
> Thanks for reviewing 😊
> Replied inline.
> 
> > -----Original Message-----
> > From: Leon Romanovsky <[email-masked]>
> > Sent: Thursday, 6 July, 2023 3:56 PM
> > To: Nguyen, Anthony L <[email-masked]>
> > Cc: [email-masked]; [email-masked]; [email-masked];
> > [email-masked]; [email-masked]; Zulkifli, Muhammad
> > Husaini <[email-masked]>; Neftin, Sasha
> > <[email-masked]>; [email-masked]; Tan Tee Min
> > <[email-masked]>; Choong, Chwee Lin
> > <[email-masked]>; Naama Meir
> > <[email-masked]>
> > Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed
> > 
> > On Wed, Jul 05, 2023 at 01:19:02PM -0700, Tony Nguyen wrote:
> > > From: Muhammad Husaini Zulkifli <[email-masked]>
> > >
> > > If a user schedules a Gate Control List (GCL) to close one of the QBV
> > > gates while also transmitting a packet to that closed gate, TX Hang
> > > will be happen. HW would not drop any packet when the gate is closed
> > > and keep queuing up in HW TX FIFO until the gate is re-opened.
> > > This patch implements the solution to drop the packet for the closed
> > > gate.
> > >
> > > This patch will also reset the adapter to perform SW initialization
> > > for each 1st Gate Control List (GCL) to avoid hang.
> > > This is due to the HW design, where changing to TSN transmit mode
> > > requires SW initialization. Intel Discrete I225/6 transmit mode cannot
> > > be changed when in dynamic mode according to Software User Manual
> > > Section 7.5.2.1. Subsequent Gate Control List (GCL) operations will
> > > proceed without a reset, as they already are in TSN Mode.
> > >
> > > Step to reproduce:
> > >
> > > DUT:
> > > 1) Configure GCL List with certain gate close.
> > >
> > > BASE=$(date +%s%N)
> > > tc qdisc replace dev $IFACE parent root handle 100 taprio \
> > >     num_tc 4 \
> > >     map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> > >     queues 1@0 1@1 1@2 1@3 \
> > >     base-time $BASE \
> > >     sched-entry S 0x8 500000 \
> > >     sched-entry S 0x4 500000 \
> > >     flags 0x2
> > >
> > > 2) Transmit the packet to closed gate. You may use udp_tai application
> > > to transmit UDP packet to any of the closed gate.
> > >
> > > ./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004
> > >
> > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
> > > Co-developed-by: Tan Tee Min <[email-masked]>
> > > Signed-off-by: Tan Tee Min <[email-masked]>
> > > Tested-by: Chwee Lin Choong <[email-masked]>
> > > Signed-off-by: Muhammad Husaini Zulkifli
> > > <[email-masked]>
> > > Tested-by: Naama Meir <[email-masked]>
> > > Signed-off-by: Tony Nguyen <[email-masked]>
> > > ---
> > >  drivers/net/ethernet/intel/igc/igc.h      |  6 +++
> > >  drivers/net/ethernet/intel/igc/igc_main.c | 58
> > > +++++++++++++++++++++--  drivers/net/ethernet/intel/igc/igc_tsn.c  |
> > > 41 ++++++++++------
> > >  3 files changed, 87 insertions(+), 18 deletions(-)

<...>

> > > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer
> > > +*timer) {
> > > +	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> > > +						   hrtimer);
> > > +	unsigned int i;
> > > +
> > > +	adapter->qbv_transition = true;
> > > +	for (i = 0; i < adapter->num_tx_queues; i++) {
> > > +		struct igc_ring *tx_ring = adapter->tx_ring[i];
> > > +
> > > +		if (tx_ring->admin_gate_closed) {
> > 
> > Doesn't asynchronic access to shared variable through hrtimer require some
> > sort of locking?
> 
> Yeah I agreed with you. However, IMHO, it should be saved without the lock. 
> These variables, admin_gate_closed and oper_gate_closed, were set during the transition 
> and setup/delete of the TC only. The qbv_transition flag has been used to protect the 
> operation when it is in qbv transition.

I have no idea what last sentence means, but igc_qbv_scheduling_timer()
and tx_ring are global function/variables and TC setup/delete can run in
parallel to them.

Thanks