Re: [PATCH iwl-net v2] idpf: keep the mailbox up while tearing down vports on shutdown

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

On 10/6/2026 10:57 PM, Tian Xun Ng wrote:
> From: Tian Xun Ng <tianxun.ng@bytedance.com>
> 
> Since commit 4c9106f4906a ("idpf: fix adapter NULL pointer dereference
> on reboot"), idpf_shutdown() calls idpf_vc_core_deinit() directly instead
> of idpf_remove(), so IDPF_REMOVE_IN_PROG is not set on shutdown.
> idpf_vc_core_deinit() uses that flag to decide when to shut the virtchnl
> transaction manager down. Without it, libie_ctlq_xn_shutdown() runs
> before idpf_deinit_task() tears the vports down, so every message sent
> during that teardown (disable vport, disable queues, destroy vport)
> fails at once. The device is never told to stop its queues and keeps
> them enabled, with the ring addresses of the kernel that is going away.
> 
> After a warm reboot, the first queue reconfiguration of a port that has
> not been opened yet (udev setting the MTU) sends VIRTCHNL2_OP_DEL_QUEUES.
> The device then drains the queues it still considers live and writes
> SW_MARKER TX completions to the previous kernel's completion rings. With
> the IOMMU translating, those writes fault about 19 s into every such
> boot:
> 
>    arm-smmu-v3 arm-smmu-v3.12.auto: event: F_TRANSLATION client: 0006:01:00.0 sid: 0x30100 ssid: 0x0 iova: 0x3ef60840 ipa: 0x0
> 
> In IOMMU pass-through mode they land on pages the new kernel has already
> reused, which shows up as "pagealloc: memory corruption" with
> page_poison=1 and as crashes in unrelated code without it.
> 
> A function reset on shutdown does not help on these devices: with a PF
> reset issued in idpf_shutdown() and PFGEN_RSTAT polled until the reset
> completed, every warm reboot still faulted, and the reset the next
> kernel issues at probe does not clear the queues either. Delivering the
> teardown messages does.
> 
> Set IDPF_REMOVE_IN_PROG in idpf_shutdown(), as idpf_remove() does, so
> that the vports are destroyed while the mailbox is still up. So that an
> unresponsive control plane cannot hold up a reboot, give each mailbox
> transaction during shutdown a 2 s timeout instead of 60 s, and fail the
> remaining ones at once after the first timeout. If the function has
> already been reset, for instance a VF whose PF went down first,
> idpf_is_reset_detected() fails them without waiting.
> 
> Tested on two arm64 (64K pages) servers, each with two idpf PFs, with
> this change backported to a 6.17 kernel and the IOMMU translating: no
> faults in 25 warm reboots, against a fault on every warm reboot
> without it. Each PF's teardown took about 1.2 s for six mailbox
> transactions; the slowest, VIRTCHNL2_OP_DEALLOC_VECTORS, took about
> 300 ms. To stand in for a control plane that does not reply, the driver

I'd probably bump this to 500ms to be sure. It's good that you measured 
the slowest in your environment, but there is no guarantee that would be 
the same elsewhere.

> was also built to skip sending mailbox messages during shutdown: the
> first transaction timed out after 2 s, the rest failed at once, the
> teardown took 2.8 s per PF, and every reboot faulted again.
> 
> Fixes: 4c9106f4906a ("idpf: fix adapter NULL pointer dereference on reboot")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Tian Xun Ng <tianxun.ng@bytedance.com>
> ---
> v2:
> - Drop patch 2 (function reset on shutdown). On a VF it queued
>    VIRTCHNL2_OP_RESET_VF and then freed the mailbox, and on these devices
>    a PF reset does not clear the stale queues anyway (numbers in the v1
>    thread).
> - Set IDPF_REMOVE_IN_PROG in idpf_shutdown() instead of changing the
>    condition in idpf_vc_core_deinit(); no new idpf_is_reset_detected()
>    call (Emil).
> - Bound the shutdown teardown: 2 s per transaction instead of 60 s, and
>    fail the remaining ones at once after the first timeout (Emil). 2 s is
>    about six times the slowest teardown transaction measured here.
> - No VF special case, unlike what I suggested in the thread: a VF whose
>    PF has been reset already fails fast through the existing
>    idpf_is_reset_detected() check in idpf_send_mb_msg(), and a VF with a
>    working mailbox leaves queues behind on a warm reboot just like a PF.
>    Not tested on a VF; I have no VF setup.
> - Compile-tested on net-queue dev-queue (W=1, allmodconfig and
>    allyesconfig, no new warnings); runtime-tested as the 6.17 backport
>    described above.
> 
> v1: https://lore.kernel.org/all/20260917105205.37561-1-luckilystar08@gmail.com/
> 
>   drivers/net/ethernet/intel/idpf/idpf.h        |  4 +++
>   drivers/net/ethernet/intel/idpf/idpf_main.c   |  7 +++++
>   .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 28 ++++++++++++++++---
>   .../net/ethernet/intel/idpf/idpf_virtchnl.h   |  1 +
>   4 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
> index 470bc23c8..5d2e8a346 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf.h
> @@ -88,6 +88,8 @@ enum idpf_state {
>    * @IDPF_REMOVE_IN_PROG: Driver remove in progress
>    * @IDPF_MB_INTR_MODE: Mailbox in interrupt mode
>    * @IDPF_VC_CORE_INIT: virtchnl core has been init
> + * @IDPF_SHUTDOWN_IN_PROG: Driver shutdown in progress
> + * @IDPF_SHUTDOWN_XN_TIMEOUT: A mailbox transaction timed out during shutdown
>    * @IDPF_FLAGS_NBITS: Must be last
>    */
>   enum idpf_flags {
> @@ -97,6 +99,8 @@ enum idpf_flags {
>   	IDPF_REMOVE_IN_PROG,
>   	IDPF_MB_INTR_MODE,
>   	IDPF_VC_CORE_INIT,
> +	IDPF_SHUTDOWN_IN_PROG,
> +	IDPF_SHUTDOWN_XN_TIMEOUT,

In general we try to avoid adding new flags as they tend to spread 
accross the code in weird ways. I think we can replicate the logic in 
your patch without the need for introducing new flags.

>   	IDPF_FLAGS_NBITS,
>   };
>   
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index 129bccaa6..fa27ee1cd 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -196,6 +196,13 @@ static void idpf_shutdown(struct pci_dev *pdev)
>   
>   	cancel_delayed_work_sync(&adapter->serv_task);
>   	cancel_delayed_work_sync(&adapter->vc_event_task);
> +
> +	/* Destroy the vports while the mailbox is still up, so that the
> +	 * device stops its queues before the next kernel reuses their memory.
> +	 * A reset does not clear them on every device.
> +	 */
> +	set_bit(IDPF_SHUTDOWN_IN_PROG, adapter->flags);
> +	set_bit(IDPF_REMOVE_IN_PROG, adapter->flags);

Case in point, now we have an overlap with SHUTDOWN_IN_PROG and 
REMOVE_IN_PROG, with the shutdown logic depending on the side effects of 
REMOVE_IN_PROG. I did a rough draft, keeping your logic and I think we 
should be able to avoid the new flags by introducing a global variable 
adapter->vc_xn_timeout_ms and set it based on the flow we're in, like in 
this spot instead of the above, something like:

+	adapter->vc_xn_timeout_ms = IDPF_VC_XN_SHUTDOWN_TIMEOUT_MSEC;
+	set_bit(IDPF_REMOVE_IN_PROG, adapter->flags);

followed with the error logic in idpf_send_mb_msg() that allows us to 
bail early.

Thanks,
Emil

>   	idpf_vc_core_deinit(adapter);
>   	idpf_deinit_dflt_mbx(adapter);
>   
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 1caf52706..5f5d72671 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -190,22 +190,38 @@ static void idpf_prepare_ptp_mb_msg(struct idpf_adapter *adapter, u32 op,
>    * Cleanup the mailbox queue entries of the previously sent message to
>    * unmap and release the buffer.
>    *
> + * During shutdown each transaction gets a short timeout, and once one of
> + * them times out the rest fail at once, so that an unresponsive control
> + * plane cannot hold up a reboot.
> + *
>    * Return: 0 if the request was successful, -%EBUSY if reset is detected
> - *	   or Tx control queue is full, other negative error code on failure.
> + *	   or Tx control queue is full, -%ETIMEDOUT if a transaction already
> + *	   timed out during shutdown, other negative error code on failure.
>    */
>   int idpf_send_mb_msg(struct idpf_adapter *adapter,
>   		     struct libie_ctlq_xn_send_params *xn_params,
>   		     void *send_buf, size_t send_buf_size)
>   {
> +	bool shutdown = test_bit(IDPF_SHUTDOWN_IN_PROG, adapter->flags);
>   	struct libie_ctlq_msg ctlq_msg = {};
> +	int err = 0;
>   
> -	if (idpf_is_reset_detected(adapter)) {
> +	if (idpf_is_reset_detected(adapter))
> +		err = -EBUSY;
> +	else if (shutdown && test_bit(IDPF_SHUTDOWN_XN_TIMEOUT, adapter->flags))
> +		err = -ETIMEDOUT;
> +
> +	if (err) {
>   		if (!libie_cp_can_send_onstack(send_buf_size))
>   			kfree(send_buf);
>   
> -		return -EBUSY;
> +		return err;
>   	}
>   
> +	if (shutdown)
> +		xn_params->timeout_ms = min_t(u64, xn_params->timeout_ms,
> +					      IDPF_VC_XN_SHUTDOWN_TIMEOUT_MSEC);
> +
>   	idpf_prepare_ptp_mb_msg(adapter, xn_params->chnl_opcode, &ctlq_msg);
>   	xn_params->ctlq_msg = ctlq_msg.opcode ? &ctlq_msg : NULL;
>   
> @@ -217,7 +233,11 @@ int idpf_send_mb_msg(struct idpf_adapter *adapter,
>   
>   	idpf_mb_clean(xn_params->ctlq, false);
>   
> -	return libie_ctlq_xn_send(xn_params);
> +	err = libie_ctlq_xn_send(xn_params);
> +	if (err == -ETIMEDOUT && shutdown)
> +		set_bit(IDPF_SHUTDOWN_XN_TIMEOUT, adapter->flags);
> +
> +	return err;
>   }
>   
>   /**
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h
> index 5d27805ff..b0809d149 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h
> @@ -7,6 +7,7 @@
>   #include <linux/net/intel/virtchnl2.h>
>   
>   #define IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC	(60 * 1000)
> +#define IDPF_VC_XN_SHUTDOWN_TIMEOUT_MSEC	2000
>   
>   struct idpf_adapter;
>   struct idpf_netdev_priv;