> -----Original Message----- > From: Intel-wired-lan <[email protected]> On Behalf Of > Larysa Zaremba > Sent: Monday, December 15, 2025 8:10 AM > To: Tantilov, Emil S <[email protected]> > Cc: [email protected]; Nguyen, Anthony L > <[email protected]>; Lobakin, Aleksander > <[email protected]>; Samudrala, Sridhar > <[email protected]>; Singhai, Anjali <[email protected]>; > Michal Swiatkowski <[email protected]>; Fijalkowski, Maciej > <[email protected]>; Chittim, Madhu <[email protected]>; > Hay, Joshua A <[email protected]>; Keller, Jacob E > <[email protected]>; Shanmugam, Jayaprakash > <[email protected]>; Wochtman, Natalia > <[email protected]>; Jiri Pirko <[email protected]>; David S. Miller > <[email protected]>; Eric Dumazet <[email protected]>; Jakub > Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Simon > Horman <[email protected]>; Jonathan Corbet <[email protected]>; Richard > Cochran <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; Andrew Lunn <[email protected]>; > [email protected]; [email protected]; linux- > [email protected]; Loktionov, Aleksandr > <[email protected]> > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v5 09/15] idpf: refactor idpf > to > use libie control queues > > 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/20250508184715.7631-1-emil.s.t > > [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);
Tested-by: Samuel Salin <[email protected]>
