Re: [PATCH iwl-next v4 09/15] idpf: refactor idpf to use libie control queues
From: Tantilov, Emil S
Date: Wed Jun 18 2025 - 22:58:06 EST
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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@xxxxxxxxx>
Co-developed-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
---
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