netdev - Re: [PATCH net 1/6] igc: Rename qbv_enable to taprio_offload

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: <51f59838-8972-73c8-e6d2-83ad56bfeab4@linutronix.de>
Date: Tue, 11 Jul 2023 09:18:31 +0200
From: Florian Kauer <florian.kauer@...utronix.de>
To: Leon Romanovsky <leon@...nel.org>,
 Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, netdev@...r.kernel.org, kurt@...utronix.de,
 vinicius.gomes@...el.com, muhammad.husaini.zulkifli@...el.com,
 tee.min.tan@...ux.intel.com, aravindhan.gunasekaran@...el.com,
 sasha.neftin@...el.com, Naama Meir <naamax.meir@...ux.intel.com>
Subject: Re: [PATCH net 1/6] igc: Rename qbv_enable to taprio_offload_enable

Hi Leon,

On 11.07.23 09:01, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote:
>> From: Florian Kauer <florian.kauer@...utronix.de>
>>
>> In the current implementation the flags adapter->qbv_enable
>> and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not
>> have the same meaning. The first one is used only to indicate
>> taprio offload (i.e. when igc_save_qbv_schedule was called),
>> while the second one corresponds to the Qbv mode of the hardware.
>> However, the second one is also used to support the TX launchtime
>> feature, i.e. ETF qdisc offload. This leads to situations where
>> adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED
>> is set. This is prone to confusion.
>>
>> The rename should reduce this confusion. Since it is a pure
>> rename, it has no impact on functionality.
> 
> And shouldn't be sent to net, but to net-next.> 
> Thanks

In principle I fully agree that sole renames are not intended for net.
But in this case the rename is tightly coupled with the other patches
of the series, not only due to overlapping code changes, but in particular
because the naming might very likely be one root cause of the regressions.

I probably should have just squashed it with the second patch,
but my initial idea was to keep them separate for clarity.

Also see:
https://lore.kernel.org/netdev/SJ1PR11MB6180B5C87699252B91FB7EE1B858A@SJ1PR11MB6180.namprd11.prod.outlook.com/
https://lore.kernel.org/netdev/0c02e976-0da6-8ed8-4546-4df7af4ebed5@linutronix.de/

Thanks,
Florian
 
>>
>> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
>> Signed-off-by: Florian Kauer <florian.kauer@...utronix.de>
>> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.de>
>> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>> ---
>>  drivers/net/ethernet/intel/igc/igc.h      | 2 +-
>>  drivers/net/ethernet/intel/igc/igc_main.c | 6 +++---
>>  drivers/net/ethernet/intel/igc/igc_tsn.c  | 2 +-
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index 639a50c02537..9db384f66a8e 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -191,7 +191,7 @@ struct igc_adapter {
>>  	int tc_setup_type;
>>  	ktime_t base_time;
>>  	ktime_t cycle_time;
>> -	bool qbv_enable;
>> +	bool taprio_offload_enable;
>>  	u32 qbv_config_change_errors;
>>  	bool qbv_transition;
>>  	unsigned int qbv_count;
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 281a0e35b9d1..fae534ef1c4f 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>>  
>>  	switch (qopt->cmd) {
>>  	case TAPRIO_CMD_REPLACE:
>> -		adapter->qbv_enable = true;
>> +		adapter->taprio_offload_enable = true;
>>  		break;
>>  	case TAPRIO_CMD_DESTROY:
>> -		adapter->qbv_enable = false;
>> +		adapter->taprio_offload_enable = false;
>>  		break;
>>  	default:
>>  		return -EOPNOTSUPP;
>>  	}
>>  
>> -	if (!adapter->qbv_enable)
>> +	if (!adapter->taprio_offload_enable)
>>  		return igc_tsn_clear_schedule(adapter);
>>  
>>  	if (qopt->base_time < 0)
>> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
>> index 3cdb0c988728..b76ebfc10b1d 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
>> @@ -37,7 +37,7 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter)
>>  {
>>  	unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED;
>>  
>> -	if (adapter->qbv_enable)
>> +	if (adapter->taprio_offload_enable)
>>  		new_flags |= IGC_FLAG_TSN_QBV_ENABLED;
>>  
>>  	if (is_any_launchtime(adapter))
>> -- 
>> 2.38.1
>>
>>