Re: [PATCH net 2/6] igc: Do not enable taprio offload for invalid arguments

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jul 10, 2023 at 09:34:59AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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>
> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index fae534ef1c4f..fb8e55c7c402 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6097,6 +6097,7 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter)
>  
>  	adapter->base_time = 0;
>  	adapter->cycle_time = NSEC_PER_SEC;
> +	adapter->taprio_offload_enable = false;
>  	adapter->qbv_config_change_errors = 0;
>  	adapter->qbv_transition = false;
>  	adapter->qbv_count = 0;
> @@ -6124,20 +6125,12 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  	size_t n;
>  	int i;
>  
> -	switch (qopt->cmd) {
> -	case TAPRIO_CMD_REPLACE:
> -		adapter->taprio_offload_enable = true;
> -		break;
> -	case TAPRIO_CMD_DESTROY:
> -		adapter->taprio_offload_enable = false;
> -		break;
> -	default:
> -		return -EOPNOTSUPP;
> -	}

All this deleted code was changed in the previous patch. It raises the
question again if previous patch was really needed to be sent to net.

Thanks

> -
> -	if (!adapter->taprio_offload_enable)
> +	if (qopt->cmd == TAPRIO_CMD_DESTROY)
>  		return igc_tsn_clear_schedule(adapter);
>  
> +	if (qopt->cmd != TAPRIO_CMD_REPLACE)
> +		return -EOPNOTSUPP;
> +
>  	if (qopt->base_time < 0)
>  		return -ERANGE;
>  
> @@ -6149,6 +6142,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  
>  	adapter->cycle_time = qopt->cycle_time;
>  	adapter->base_time = qopt->base_time;
> +	adapter->taprio_offload_enable = true;
>  
>  	igc_ptp_read(adapter, &now);
>  
> -- 
> 2.38.1
> 
>