* Re: [PATCH iwl-net v3] idpf: keep the mailbox up while tearing down vports on shutdown
2026-10-09 3:27 [PATCH iwl-net v3] idpf: keep the mailbox up while tearing down vports on shutdown Tian Xun Ng
@ 2026-10-09 21:48 ` Tantilov, Emil S
0 siblings, 0 replies; 2+ messages in thread
From: Tantilov, Emil S @ 2026-10-09 21:48 UTC (permalink / raw)
To: Tian Xun Ng, intel-wired-lan
Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, aleksander.lobakin,
aleksandr.loktionov, decot, andrew+netdev, davem, edumazet, kuba,
pabeni, Tian Xun Ng, decot@google.com, Hay, Joshua A
On 10/8/2026 8:27 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, and the device is never told that the vports are gone.
>
> A reset should clear that state, but on the system where this was found
> the device kept the queues enabled across a warm reboot. The first queue
> reconfiguration in the next kernel (udev setting the MTU) sends
> VIRTCHNL2_OP_DEL_QUEUES, and the device then writes SW_MARKER TX
This case -> DEL_QUEUES/SW_MARKER_TX will also need to be addressed by
this patch. A CP that acks DISABLE_QUEUES and doesn't post markers would
end up timing out by 500ms per TxQ.
Something like the following can be folded into this patch:
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 3afd6e62e86f..7ffd512561ea 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -372,6 +372,39 @@ static int idpf_send_chunked_msg(struct
idpf_adapter *adapter,
return 0;
}
+/**
+ * idpf_wait_for_marker - wait for TxQ marker
+ * @adapter: driver specific private structure
+ * @txq: Tx queue to wait for
+ *
+ * Return: %true if the marker was received
+ */
+static bool idpf_wait_for_marker(struct idpf_adapter *adapter,
+ struct idpf_tx_queue *txq)
+{
+ u32 timeout_ms = READ_ONCE(adapter->vc_xn_timeout_ms);
+
+ idpf_queue_set(SW_MARKER, txq);
+
+ if (!timeout_ms)
+ return false;
+
+ idpf_wait_for_sw_marker_completion(txq);
+ if (!idpf_queue_has(SW_MARKER, txq))
+ return true;
+
+ if (timeout_ms < IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC)
+ WRITE_ONCE(adapter->vc_xn_timeout_ms, 0);
+
+ return false;
+}
+
/**
* idpf_wait_for_marker_event_set - wait for software marker response for
* selected Tx queues
@@ -392,9 +425,7 @@ static int idpf_wait_for_marker_event_set(const
struct idpf_queue_set *qs)
netdev = txq->netdev;
- idpf_queue_set(SW_MARKER, txq);
- idpf_wait_for_sw_marker_completion(txq);
- markers_rcvd &= !idpf_queue_has(SW_MARKER, txq);
+ markers_rcvd &= idpf_wait_for_marker(qs->adapter, txq);
break;
default:
break;
In addition, adding a check for the REMOVE_IN_PROG inside the loop in
idpf_statistics_task() should help, since your patch keeps the original
timeout until stopping the tasks:
if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
return;
Rest looks good to me, although I would suggest waiting for the AI
review before posting v4:
https://sashiko.dev/#/patchset/20261009032706.94324-1-luckilystar08%40gmail.com
Thanks,
Emil
> 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 corrupt pages the new kernel has reused.
>
> Set IDPF_REMOVE_IN_PROG at the start of idpf_shutdown(), as idpf_remove()
> does, so that the event task cannot be requeued and the vports are
> destroyed while the mailbox is still up. So that an unresponsive control
> plane does not hold up a reboot for long, cap each transaction sent
> during the teardown at 500 ms instead of 60 s, through a new
> adapter->vc_xn_timeout_ms, and fail later ones at once after the first
> timeout. A transaction that is already waiting when shutdown starts
> keeps its own timeout; bounding that would need support in libie. If
> the function has already been reset, for instance a VF whose PF went
> down first, idpf_is_reset_detected() fails the messages 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: 20
> warm reboots without a fault. Each PF's teardown took about 1.2 s for
> six mailbox transactions; the slowest, VIRTCHNL2_OP_DEALLOC_VECTORS,
> took about 300 ms, and the queue-related ones at most 240 ms. With the
> driver made to skip sending the teardown messages, the first transaction
> timed out after 500 ms, the rest failed at once, 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>
> ---
> v3:
> - No new flags (Emil): idpf_shutdown() lowers adapter->vc_xn_timeout_ms
> (60 s from probe), and idpf_send_mb_msg() caps each transaction with
> it and sets it to 0 once a shortened one times out, which fails the
> rest at once.
> - Shutdown timeout 500 ms instead of 2 s (Emil).
> - Set IDPF_REMOVE_IN_PROG before cancelling the service and event tasks,
> so the event task cannot be requeued (Emil). The timeout is lowered
> after the tasks are cancelled, so a reset that was still running keeps
> its normal timeouts.
> - The comment and commit message no longer state that a reset does not
> clear the vports (Emil), and say that a transaction already waiting
> when shutdown starts keeps its own timeout.
> - Retested as a 6.17 backport: 20 warm reboots without a fault; the
> slowest teardown transaction took 306 ms.
> - Not addressed here: with the transaction manager shut down later, an
> async PTP Tx timestamp reply can race idpf_ptp_release(). The same
> window already exists on remove; left for a separate fix.
>
> v2: https://lore.kernel.org/all/20261007055741.30629-1-luckilystar08@gmail.com/
> - Dropped patch 2 (function reset on shutdown): on a VF it queued
> VIRTCHNL2_OP_RESET_VF and then freed the mailbox.
> - Bounded the shutdown teardown and failed the rest after the first
> timeout; no VF special case, since a VF whose PF has been reset already
> fails fast through idpf_is_reset_detected(). Not tested on a VF.
>
> v1: https://lore.kernel.org/all/20260917105205.37561-1-luckilystar08@gmail.com/
>
> drivers/net/ethernet/intel/idpf/idpf.h | 3 +++
> drivers/net/ethernet/intel/idpf/idpf_main.c | 10 ++++++++
> .../net/ethernet/intel/idpf/idpf_virtchnl.c | 25 ++++++++++++++++---
> .../net/ethernet/intel/idpf/idpf_virtchnl.h | 1 +
> 4 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
> index 470bc23c8..0739861a1 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf.h
> @@ -625,6 +625,8 @@ struct idpf_vport_config {
> * @asq: Send control queue info
> * @arq: Receive control queue info
> * @xnm: Xn transaction manager
> + * @vc_xn_timeout_ms: Upper bound for each Xn transaction timeout, shortened
> + * on shutdown and set to 0 once a shortened one timed out
> * @num_avail_msix: Available number of MSIX vectors
> * @num_msix_entries: Number of entries in MSIX table
> * @msix_entries: MSIX table
> @@ -684,6 +686,7 @@ struct idpf_adapter {
> struct libie_ctlq_info *asq;
> struct libie_ctlq_info *arq;
> struct libie_ctlq_xn_manager *xnm;
> + u32 vc_xn_timeout_ms;
> u16 num_avail_msix;
> u16 num_msix_entries;
> struct msix_entry *msix_entries;
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index 129bccaa6..ec751f4a6 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -194,8 +194,17 @@ static void idpf_shutdown(struct pci_dev *pdev)
> {
> struct idpf_adapter *adapter = pci_get_drvdata(pdev);
>
> + /* As in idpf_remove(): keep the event task from being requeued, and
> + * destroy the vports while the mailbox is still up.
> + */
> + set_bit(IDPF_REMOVE_IN_PROG, adapter->flags);
> cancel_delayed_work_sync(&adapter->serv_task);
> cancel_delayed_work_sync(&adapter->vc_event_task);
> +
> + /* Bound each teardown transaction, so that an unresponsive control
> + * plane does not hold up a reboot for long.
> + */
> + WRITE_ONCE(adapter->vc_xn_timeout_ms, IDPF_VC_XN_SHUTDOWN_TIMEOUT_MSEC);
> idpf_vc_core_deinit(adapter);
> idpf_deinit_dflt_mbx(adapter);
>
> @@ -267,6 +276,7 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> adapter->req_rx_splitq = true;
>
> adapter->pdev = pdev;
> + adapter->vc_xn_timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
>
> err = idpf_dev_init(adapter, ent);
> if (err) {
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 1caf52706..3afd6e62e 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -190,20 +190,32 @@ 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.
> *
> + * The transaction timeout is capped at adapter->vc_xn_timeout_ms. Once a
> + * transaction with a shortened timeout times out, later ones fail at once
> + * instead of waiting again.
> + *
> * 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 shortened transaction
> + * already timed out, 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)
> {
> + u32 timeout_ms = READ_ONCE(adapter->vc_xn_timeout_ms);
> 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 (!timeout_ms)
> + err = -ETIMEDOUT;
> +
> + if (err) {
> if (!libie_cp_can_send_onstack(send_buf_size))
> kfree(send_buf);
>
> - return -EBUSY;
> + return err;
> }
>
> idpf_prepare_ptp_mb_msg(adapter, xn_params->chnl_opcode, &ctlq_msg);
> @@ -214,10 +226,15 @@ int idpf_send_mb_msg(struct idpf_adapter *adapter,
> xn_params->xnm = adapter->xnm;
> xn_params->ctlq = xn_params->ctlq ? xn_params->ctlq : adapter->asq;
> xn_params->rel_tx_buf = kfree;
> + xn_params->timeout_ms = min_t(u64, xn_params->timeout_ms, timeout_ms);
>
> idpf_mb_clean(xn_params->ctlq, false);
>
> - return libie_ctlq_xn_send(xn_params);
> + err = libie_ctlq_xn_send(xn_params);
> + if (err == -ETIMEDOUT && timeout_ms < IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC)
> + WRITE_ONCE(adapter->vc_xn_timeout_ms, 0);
> +
> + return err;
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h
> index 5d27805ff..911a25ad9 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 500
>
> struct idpf_adapter;
> struct idpf_netdev_priv;
^ permalink raw reply related [flat|nested] 2+ messages in thread