Re: [Intel-wired-lan] [PATCH net v2 2/6] igc: Do not enable taprio offload for invalid arguments

From: "naamax.meir" <naamax.meir@linux.intel.com>
To: Florian Kauer <florian.kauer@linutronix.de>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	Vinicius Costa Gomes <vinicius.gomes@intel.com>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Tan Tee Min <tee.min.tan@linux.intel.com>,
	Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>,
	Aravindhan Gunasekaran <aravindhan.gunasekaran@intel.com>,
	Malli C <mallikarjuna.chilakala@intel.com>
Cc: netdev@vger.kernel.org, kurt@linutronix.de,
	intel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH net v2 2/6] igc: Do not enable taprio offload for invalid arguments
Date: Mon, 3 Jul 2023 11:34:44 +0300	[thread overview]
Message-ID: <f14f0d50-2569-0d47-91d6-23c1f99712ad@linux.intel.com> (raw)
In-Reply-To: <20230619100858.116286-3-florian.kauer@linutronix.de>

On 6/19/2023 13:08, Florian Kauer wrote:
> Only set adapter->taprio_offload_enable after validating the arguments.
> Otherwise, it stays set even if the offload was not enabled.
> Since the subsequent code does not get executed in case of invalid
> arguments, it will not be read at first.
> However, by activating and then deactivating another offload
> (e.g. ETF/TX launchtime offload), taprio_offload_enable is read
> and erroneously keeps the offload feature of the NIC enabled.
> 
> This can be reproduced as follows:
> 
>      # TAPRIO offload (flags == 0x2) and negative base-time leading to expected -ERANGE
>      sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
> 	    num_tc 1 \
> 	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
> 	    queues 1@0 \
> 	    base-time -1000 \
> 	    sched-entry S 01 300000 \
> 	    flags 0x2
> 
>      # IGC_TQAVCTRL is 0x0 as expected (iomem=relaxed for reading register)
>      sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1
> 
>      # Activate ETF offload
>      sudo tc qdisc replace dev enp1s0 parent root handle 6666 mqprio \
> 	    num_tc 3 \
> 	    map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
> 	    queues 1@0 1@1 2@2 \
> 	    hw 0
>      sudo tc qdisc add dev enp1s0 parent 6666:1 etf \
> 	    clockid CLOCK_TAI \
> 	    delta 500000 \
> 	    offload
> 
>      # IGC_TQAVCTRL is 0x9 as expected
>      sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1
> 
>      # Deactivate ETF offload again
>      sudo tc qdisc delete dev enp1s0 parent 6666:1
> 
>      # IGC_TQAVCTRL should now be 0x0 again, but is observed as 0x9
>      sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1
> 
> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)


Tested-by: Naama Meir <naamax.meir@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan