Re: [PATCH iwl-next v4 09/15] idpf: refactor idpf to use libie control queues

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

On 6/18/2025 12:54 AM, Larysa Zaremba wrote:
> On Tue, Jun 17, 2025 at 05:04:55PM -0700, Tantilov, Emil S wrote:
>>
>>
>> On 5/16/2025 7:58 AM, Larysa Zaremba wrote:
>>> From: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
>>>
>>> Support to initialize and configure controlqs, and manage their
>>> transactions was introduced in libie. As part of it, most of the existing
>>> controlq structures are renamed and modified. Use those APIs in idpf and
>>> make all the necessary changes.
>>>
>>> Previously for the send and receive virtchnl messages, there used to be a
>>> memcpy involved in controlq code to copy the buffer info passed by the send
>>> function into the controlq specific buffers. There was no restriction to
>>> use automatic memory in that case. The new implementation in libie removed
>>> copying of the send buffer info and introduced DMA mapping of the send
>>> buffer itself. To accommodate it, use dynamic memory for the larger send
>>> buffers. For smaller ones (<= 128 bytes) libie still can copy them into the
>>> pre-allocated message memory.
>>>
>>> In case of receive, idpf receives a page pool buffer allocated by the libie
>>> and care should be taken to release it after use in the idpf.
>>>
>>> The changes are fairly trivial and localized, with a notable exception
>>> being the consolidation of idpf_vc_xn_shutdown and idpf_deinit_dflt_mbx
>>> under the latter name. This has some additional consequences that are
>>> addressed in the following patches.
>>
>> There is an issue with this approach that impacts the ability of the driver
>> to force a reset. See below ...
>>
>>>
>>> This refactoring introduces roughly additional 40KB of module storage used
>>> for systems that only run idpf, so idpf + libie_cp + libie_pci takes about
>>> 7% more storage than just idpf before refactoring.
>>>
>>> We now pre-allocate small TX buffers, so that does increase the memory
>>> usage, but reduces the need to allocate. This results in additional 256 *
>>> 128B of memory permanently used, increasing the worst-case memory usage by
>>> 32KB but our ctlq RX buffers need to be of size 4096B anyway (not changed
>>> by the patchset), so this is hardly noticeable.
>>>
>>> As for the timings, the fact that we are mostly limited by the HW response
>>> time which is far from instant, is not changed by this refactor.
>>>
>>> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
>>> Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
>>> Co-developed-by: Larysa Zaremba <larysa.zaremba@intel.com>
>>> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/idpf/Kconfig       |    2 +-
>>>    drivers/net/ethernet/intel/idpf/Makefile      |    2 -
>>>    drivers/net/ethernet/intel/idpf/idpf.h        |   27 +-
>>>    .../net/ethernet/intel/idpf/idpf_controlq.c   |  624 -------
>>>    .../net/ethernet/intel/idpf/idpf_controlq.h   |  130 --
>>>    .../ethernet/intel/idpf/idpf_controlq_api.h   |  177 --
>>>    .../ethernet/intel/idpf/idpf_controlq_setup.c |  171 --
>>>    drivers/net/ethernet/intel/idpf/idpf_dev.c    |   54 +-
>>>    .../net/ethernet/intel/idpf/idpf_ethtool.c    |   37 +-
>>>    drivers/net/ethernet/intel/idpf/idpf_lib.c    |   44 +-
>>>    drivers/net/ethernet/intel/idpf/idpf_main.c   |    4 -
>>>    drivers/net/ethernet/intel/idpf/idpf_mem.h    |   20 -
>>>    drivers/net/ethernet/intel/idpf/idpf_txrx.h   |    2 +-
>>>    drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |   60 +-
>>>    .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 1617 ++++++-----------
>>>    .../net/ethernet/intel/idpf/idpf_virtchnl.h   |   90 +-
>>>    .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   |  204 +--
>>>    17 files changed, 765 insertions(+), 2500 deletions(-)
>>>    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.c
>>>    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.h
>>>    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq_api.h
>>>    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq_setup.c
>>>    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_mem.h
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>>> index 68330b884967..500bff1091d9 100644
>>> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
>>> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>>> @@ -1190,6 +1190,7 @@ void idpf_statistics_task(struct work_struct *work)
>>>     */
>>>    void idpf_mbx_task(struct work_struct *work)
>>>    {
>>> +	struct libie_ctlq_xn_recv_params xn_params = {};
>>>    	struct idpf_adapter *adapter;
>>>    	adapter = container_of(work, struct idpf_adapter, mbx_task.work);
>>> @@ -1200,7 +1201,11 @@ void idpf_mbx_task(struct work_struct *work)
>>>    		queue_delayed_work(adapter->mbx_wq, &adapter->mbx_task,
>>>    				   msecs_to_jiffies(300));
>>> -	idpf_recv_mb_msg(adapter, adapter->hw.arq);
>>> +	xn_params.xnm = adapter->xn_init_params.xnm;
>>> +	xn_params.ctlq = adapter->arq;
>>> +	xn_params.ctlq_msg_handler = idpf_recv_event_msg;
>>> +
>>> +	libie_ctlq_xn_recv(&xn_params);
>>>    }
>>>    /**
>>> @@ -1757,7 +1762,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>>>    		idpf_vc_core_deinit(adapter);
>>>    		if (!is_reset)
>>
>> Since one of the checks in idpf_is_reset_detected() is !adapter->arq, this
>> will never be possible through the event task. I think we may be able to
>> remove this check altogether, but as-is this patch introduces large delays
>> in the Tx hang recovery and depending on the cause may not recover at all.
>>
>>>    			reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
>>> -		idpf_deinit_dflt_mbx(adapter);
>>>    	} else {
>>>    		dev_err(dev, "Unhandled hard reset cause\n");
>>>    		err = -EBADRQC;
>>> @@ -1825,7 +1829,7 @@ void idpf_vc_event_task(struct work_struct *work)
>>>    	return;
>>>    func_reset:
>>> -	idpf_vc_xn_shutdown(adapter->vcxn_mngr);
>>> +	idpf_deinit_dflt_mbx(adapter);
>>
>> This is not a straightforward swap, whereas previously we just discard
>> messages knowing that we cannot communicate with the CP in a reset, this
>> goes much further as it dismantles the MBX resources, and as a result the
>> check `if (!is_reset)` in idpf_init_hard_reset() will never be true.
>>
> 
> Thanks for finding this!
> 
> Given the problem seems to only relate to idpf_vc_event_task() in case of
> IDPF_HR_FUNC_RESET and the following call sequence:
> 
> 	idpf_deinit_dflt_mbx(adapter);
> 	set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
> 	idpf_init_hard_reset(adapter);
> 		netif_carrier_off();
> 		netif_tx_disable();
> 		is_reset = idpf_is_reset_detected(adapter);
> 		idpf_set_vport_state(adapter);
> 		idpf_vc_core_deinit(adapter);
> 			idpf_deinit_dflt_mbx(adapter);
> 			...
> 		...
> 
> I think, it is safe to remove idpf_deinit_dflt_mbx() from idpf_vc_event_task(),
> given no mailbox communication is attempted in between it and
> idpf_vc_core_deinit(). Anything going on in parallel also should not suffer from
> having mailbox available just a little bit longer.
> 
> What do you think?

I think this will work. The small difference here is that the MBX will 
be disabled a bit later than before. Say there were already messages in 
flight when the reset happened. Previously we flush the MBX and then 
begin to handle the reset. If we remove idpf_deinit_dflt_mbx() from the 
event task the MBX will be disabled as part of the reset handling. FWIW 
I ran a few tests with the change you propose and did not see any issues 
as a result.

Thanks,
Emil

> 
>> <snip>
>>
>> Thanks,
>> Emil
>>