Re: [Intel-wired-lan] [PATCH iwl-net v1 1/4] igc: Fix qbv_config_change_errors logics

[Simon Horman <horms@kernel.org>]

On Tue, Jul 02, 2024 at 12:09:23AM -0400, Faizal Rahim wrote:
> When user issues these cmds:
> 1. Either a) or b)
>    a) mqprio with hardware offload disabled
>    b) taprio with txtime-assist feature enabled
> 2. etf
> 3. tc qdisc delete
> 4. taprio with base time in the past
> 
> At step 4, qbv_config_change_errors wrongly increased by 1.
> 
> Excerpt from IEEE 802.1Q-2018 8.6.9.3.1:
> "If AdminBaseTime specifies a time in the past, and the current schedule
> is running, then: Increment ConfigChangeError counter"
> 
> qbv_config_change_errors should only increase if base time is in the past
> and no taprio is active. In user perspective, taprio was not active when
> first triggered at step 4. However, i225/6 reuses qbv for etf, so qbv is
> enabled with a dummy schedule at step 2 where it enters
> igc_tsn_enable_offload() and qbv_count got incremented to 1. At step 4, it
> enters igc_tsn_enable_offload() again, qbv_count is incremented to 2.
> Because taprio is running, tc_setup_type is TC_SETUP_QDISC_ETF and
> qbv_count > 1, qbv_config_change_errors value got incremented.
> 
> This issue happens due to reliance on qbv_count field where a non-zero
> value indicates that taprio is running. But qbv_count increases
> regardless if taprio is triggered by user or by other tsn feature. It does
> not align with qbv_config_change_errors expectation where it is only
> concerned with taprio triggered by user.
> 
> Fixing this by relocating the qbv_config_change_errors logic to
> igc_save_qbv_schedule(), eliminating reliance on qbv_count and its
> inaccuracies from i225/6's multiple uses of qbv feature for other TSN
> features.
> 
> The new function created: igc_tsn_is_taprio_activated_by_user() uses
> taprio_offload_enable field to indicate that the current running taprio
> was triggered by user, instead of triggered by non-qbv feature like etf.
> 
> Fixes: ae4fe4698300 ("igc: Add qbv_config_change_errors counter")
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>

Thanks Faizal,

My nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
> index 22cefb1eeedf..02dd41aff634 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
> @@ -78,6 +78,17 @@ void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter)
>  	wr32(IGC_GTXOFFSET, txoffset);
>  }
>  
> +bool igc_tsn_is_taprio_activated_by_user(struct igc_adapter *adapter)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +
> +	if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) &&
> +	    adapter->taprio_offload_enable)
> +		return true;
> +	else
> +		return false;

As per my response to patch 2/4, I think something like this is a bit
nicer:

(Completely untested!)

	return (rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) &&
		adapter->taprio_offload_enable;


> +}
> +
>  /* Returns the TSN specific registers to their default values after
>   * the adapter is reset.
>   */

...