netdev - Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix transaction timeouts on reset

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] [day] [month] [year] [list]

Message-ID: <3eef0493-7443-43bd-938a-7a1dea83ac11@intel.com>
Date: Wed, 18 Dec 2024 08:31:27 -0800
From: "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<larysa.zaremba@...el.com>, <decot@...gle.com>, <willemb@...gle.com>,
	<anthony.l.nguyen@...el.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix transaction timeouts
 on reset



On 12/18/2024 7:13 AM, Alexander Lobakin wrote:
> From: Emil Tantilov <emil.s.tantilov@...el.com>
> Date: Tue, 17 Dec 2024 17:44:17 -0800
> 
>> Restore the call to idpf_vc_xn_shutdown() at the beginning of
>> idpf_vc_core_deinit() provided the function is not called on
>> remove. In the reset path this call is needed to prevent mailbox
>> transactions from timing out.
>>
>> Fixes: 09d0fb5cb30e ("idpf: deinit virtchnl transaction manager after vport and vectors")
>> Reviewed-by: Larysa Zaremba <larysa.zaremba@...el.com>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@...el.com>
>> ---
>> Testing hints:
>> echo 1 > /sys/class/net/<netif>/device/reset
>> ---
>>   drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>> index d46c95f91b0d..0387794daf17 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>> @@ -3080,9 +3080,15 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter)
>>   	if (!test_bit(IDPF_VC_CORE_INIT, adapter->flags))
>>   		return;
>>   
>> +	/* Avoid transaction timeouts when called during reset */
>> +	if (!test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
>> +		idpf_vc_xn_shutdown(adapter->vcxn_mngr);
>> +
>>   	idpf_deinit_task(adapter);
>>   	idpf_intr_rel(adapter);
>> -	idpf_vc_xn_shutdown(adapter->vcxn_mngr);
>> +
>> +	if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
>> +		idpf_vc_xn_shutdown(adapter->vcxn_mngr);
> 
> Why test it two times...
> 
> 	bool reset;
> 
> 	...
> 
> 	reset = test_bit(REMOVE_IN_PROG);
> 	if (!reset)
> 		vc_xn_shutdown();
> 
> 	deinit_task();
> 	intr_rel();
> 
> 	if (reset)
> 		vc_xn_shutdown();
Good point, I will update in v2.

> 
> BTW can't we just move that call unconditionally?
This will be a revert of 09d0fb5cb30e:
https://lore.kernel.org/intel-wired-lan/20240904095418.6426-1-larysa.zaremba@intel.com/

Which based on internal discussions should be OK, since the control 
plane (CP) is expected to cleanup regardless. The consensus was that 
driver communicates with CP when it can, hence the checks.

Thanks,
Emil

> 
>>   
>>   	cancel_delayed_work_sync(&adapter->serv_task);
>>   	cancel_delayed_work_sync(&adapter->mbx_task);
> 
> Thanks,
> Olek