On Wed, Dec 10, 2025 at 07:42:53PM -0800, Tantilov, Emil S wrote:
>
> On 11/17/2025 5:48 AM, Larysa Zaremba wrote:
> > From: Pavan Kumar Linga <[email protected]>
> >
> > 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.
> >
> > 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: Aleksandr Loktionov <[email protected]>
> > Signed-off-by: Pavan Kumar Linga <[email protected]>
> > Co-developed-by: Larysa Zaremba <[email protected]>
> > Signed-off-by: Larysa Zaremba <[email protected]>
> > ---
> > drivers/net/ethernet/intel/idpf/Makefile | 2 -
> > drivers/net/ethernet/intel/idpf/idpf.h | 28 +-
> > .../net/ethernet/intel/idpf/idpf_controlq.c | 633 -------
> > .../net/ethernet/intel/idpf/idpf_controlq.h | 142 --
> > .../ethernet/intel/idpf/idpf_controlq_api.h | 177 --
> > .../ethernet/intel/idpf/idpf_controlq_setup.c | 171 --
> > drivers/net/ethernet/intel/idpf/idpf_dev.c | 60 +-
> > .../net/ethernet/intel/idpf/idpf_ethtool.c | 20 +-
> > drivers/net/ethernet/intel/idpf/idpf_lib.c | 67 +-
> > drivers/net/ethernet/intel/idpf/idpf_main.c | 5 -
> > 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 | 67 +-
> > .../net/ethernet/intel/idpf/idpf_virtchnl.c | 1580 ++++++-----------
> > .../net/ethernet/intel/idpf/idpf_virtchnl.h | 90 +-
> > .../ethernet/intel/idpf/idpf_virtchnl_ptp.c | 239 ++-
> > 16 files changed, 783 insertions(+), 2520 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 e15b1e8effc8..7751a81fc29d 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > @@ -1363,6 +1363,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);
> > @@ -1373,7 +1374,14 @@ void idpf_mbx_task(struct work_struct *work)
> > queue_delayed_work(adapter->mbx_wq, &adapter->mbx_task,
> > usecs_to_jiffies(300));
> > - idpf_recv_mb_msg(adapter, adapter->hw.arq);
> > + xn_params = (struct libie_ctlq_xn_recv_params) {
> > + .xnm = adapter->xn_init_params.xnm,
> > + .ctlq = adapter->arq,
> > + .ctlq_msg_handler = idpf_recv_event_msg,
> > + .budget = LIBIE_CTLQ_MAX_XN_ENTRIES,
> > + };
> > +
> > + libie_ctlq_xn_recv(&xn_params);
> > }
> > /**
> > @@ -1907,7 +1915,6 @@ static void idpf_init_hard_reset(struct idpf_adapter
> > *adapter)
> > idpf_vc_core_deinit(adapter);
> > if (!is_reset)
> > 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;
> > @@ -1972,19 +1979,11 @@ void idpf_vc_event_task(struct work_struct *work)
> > if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
> > return;
> > - if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags))
> > - goto func_reset;
> > -
> > - if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags))
> > - goto drv_load;
> > -
> > - return;
> > -
> > -func_reset:
> > - idpf_vc_xn_shutdown(adapter->vcxn_mngr);
>
> This will cause a regression where VC can timeout on reset:
> https://lore.kernel.org/intel-wired-lan/[email protected]/
>
> I think we can keep this logic, remove the calls to vc_xn_shutdown in
> idpf_vc_core_deinit() and add it to idpf_remove().
>
> Thanks,
> Emil
>
Thank you for bringging this up!
It's a shame that the solution that we have agreed with you on previouly has
such unintended consequences. Well, after looking at it this way, I see no good
solution except bringing back xnm shutdown, but in libie. See the suggested
diff
below. Please, say if it works for you.
When fixed up in a final patch idpf_vc_event_task will only have one changed
line:
@@ -1981,7 +1986,7 @@ void idpf_vc_event_task(struct work_struct *work)
return;
func_reset:
- idpf_vc_xn_shutdown(adapter->vcxn_mngr);
+ libie_ctlq_xn_shutdown(adapter->xnm);
drv_load:
set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
idpf_init_hard_reset(adapter);
libie_ctlq_xn_shutdown() sets the state to shutdown, so no new xns can be
taken (-EAGAIN) and running xns are prematurely completed
resulting in a timed out error. At the same it does not free any memory, so no
use-after-free risks.
---------------------
The main diff:
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c
b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 69eb72ed6b99..dff931ebbd9f 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1977,12 +1977,19 @@ void idpf_vc_event_task(struct work_struct *work)
if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
return;
+ if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags))
+ goto func_reset;
- if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags) ||
- test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
- set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
- idpf_init_hard_reset(adapter);
- }
+ if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags))
+ goto drv_load;
+
+ return;
+
+func_reset:
+ libie_ctlq_xn_shutdown(adapter->xn_init_params.xnm);
+drv_load:
+ set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
+ idpf_init_hard_reset(adapter);
}
/**
diff --git a/drivers/net/ethernet/intel/libie/controlq.c
b/drivers/net/ethernet/intel/libie/controlq.c
index 9b24a87872e5..a39ee6ea37f1 100644
--- a/drivers/net/ethernet/intel/libie/controlq.c
+++ b/drivers/net/ethernet/intel/libie/controlq.c
@@ -694,7 +694,7 @@ static void libie_ctlq_xn_put(struct libie_ctlq_xn_manager
*xnm,
*/
static void libie_ctlq_xn_deinit_dma(struct device *dev,
struct libie_ctlq_xn_manager *xnm,
- u32 num_entries)
+ u32 num_entries)
{
for (u32 i = 0; i < num_entries; i++) {
struct libie_ctlq_xn *xn = &xnm->ring[i];
@@ -1093,14 +1093,12 @@ u32 libie_ctlq_xn_send_clean(const struct
libie_ctlq_xn_clean_params *params)
EXPORT_SYMBOL_NS_GPL(libie_ctlq_xn_send_clean, "LIBIE_CP");
/**
- * libie_ctlq_xn_deinit - deallocate and free the transaction manager resources
+ * libie_ctlq_xn_shutdown - terminate control queue transactions
* @xnm: pointer to the transaction manager
- * @ctx: controlq context structure
*
- * All Rx processing must be stopped beforehand.
+ * Synchronously terminate existing transactions and stop accepting new ones.
*/
-void libie_ctlq_xn_deinit(struct libie_ctlq_xn_manager *xnm,
- struct libie_ctlq_ctx *ctx)
+void libie_ctlq_xn_shutdown(struct libie_ctlq_xn_manager *xnm)
{
bool must_wait = false;
u32 i;
@@ -1129,7 +1127,20 @@ void libie_ctlq_xn_deinit(struct libie_ctlq_xn_manager
*xnm,
if (must_wait)
wait_for_completion(&xnm->can_destroy);
+}
+EXPORT_SYMBOL_NS_GPL(libie_ctlq_xn_shutdown, "LIBIE_CP");
+/**
+ * libie_ctlq_xn_deinit - deallocate and free the transaction manager resources
+ * @xnm: pointer to the transaction manager
+ * @ctx: controlq context structure
+ *
+ * All Rx processing must be stopped beforehand.
+ */
+void libie_ctlq_xn_deinit(struct libie_ctlq_xn_manager *xnm,
+ struct libie_ctlq_ctx *ctx)
+{
+ libie_ctlq_xn_shutdown(xnm);
libie_ctlq_xn_deinit_dma(&ctx->mmio_info.pdev->dev, xnm,
LIBIE_CTLQ_MAX_XN_ENTRIES);
kfree(xnm);
diff --git a/include/linux/intel/libie/controlq.h
b/include/linux/intel/libie/controlq.h
index 4a627670814e..98f082b5d039 100644
--- a/include/linux/intel/libie/controlq.h
+++ b/include/linux/intel/libie/controlq.h
@@ -414,6 +414,7 @@ struct libie_ctlq_xn_init_params {
int libie_ctlq_xn_init(struct libie_ctlq_xn_init_params *params);
void libie_ctlq_xn_deinit(struct libie_ctlq_xn_manager *xnm,
struct libie_ctlq_ctx *ctx);
+void libie_ctlq_xn_shutdown(struct libie_ctlq_xn_manager *xnm);
int libie_ctlq_xn_send(struct libie_ctlq_xn_send_params *params);
u32 libie_ctlq_xn_recv(struct libie_ctlq_xn_recv_params *params);
u32 libie_ctlq_xn_send_clean(const struct libie_ctlq_xn_clean_params *params);
