[PATCH iwl-net v5 0/3] idpf: destroy the vports on shutdown

[PATCH iwl-net v5 0/3] idpf: destroy the vports on shutdown

[Tian Xun Ng]

From: Tian Xun Ng <tianxun.ng@bytedance.com>

On shutdown idpf shuts its virtchnl transaction manager down before the
vports are torn down, so the device is never told that they are gone.
Patch 3 keeps the mailbox up for that teardown, as remove already does,
with a short timeout so that an unresponsive control plane cannot hold
up a reboot for long. Patches 1 and 2 first close two existing races in
which the mailbox task uses state that the teardown has released: the
async PTP Tx timestamp reply, which patch 3 would otherwise make
reachable on shutdown, and link change events, which can already arrive
in that window on both remove and shutdown.

An LLM coding assistant helped analyse the problem and draft the changes
and their messages. Each patch carries an Assisted-by tag.

Changes in v5:
- New patch 2: idpf_handle_event_link() returns once IDPF_REMOVE_IN_PROG
  is set, so that a link event cannot use a vport, or its netdev, that
  idpf_deinit_task() has released (Sashiko on v4). The window predates
  this series: mbx_task keeps running until idpf_vc_core_deinit()
  cancels it after the vports are gone, with or without the requeue in
  patch 1, whose drain makes the check race-free. The same window on a
  hard reset is not changed.
- Patch 1 is retitled to the mailbox reply it guards. The Tx timestamp
  state cached for the transmit and Tx completion paths is freed by
  idpf_ptp_release() before NAPI is disabled; that older lifetime issue
  is not touched here.
- Patch 3: a missing Tx queue marker no longer stops the mailbox
  messages that follow. The remaining queues of that vport skip their
  marker wait, and UNMAP_QUEUE_VECTOR, DESTROY_VPORT and DEALLOC_VECTORS
  are still sent with the 500 ms cap. This departs from the helper
  suggested on v3, which also failed the later messages: the device has
  just answered DISABLE_QUEUES, so its mailbox works. With the first
  marker treated as missing, all six transactions completed and 3 of 3
  warm reboots were clean.
- Patch 3: the kdoc and the message now say that only a synchronous
  transaction that times out fails the later ones at once, and that a
  reset already running in the event task keeps its own timeouts.
  IDPF_REMOVE_IN_PROG is documented as covering shutdown.

Testing of v5, besides what patch 3 describes: W=1 builds of idpf and
libie with allmodconfig and allyesconfig showed no new warnings after
each patch. On one server, modprobe -r idpf and a reload completed
without warnings or IOMMU faults. No link event arrived during a
teardown in these tests, so the path patch 2 changes was not exercised;
these devices do not support PTP, so neither was the one patch 1
changes.

Changes in v4 (patch numbers as in v4):
- New patch 1, closing the Tx timestamp callback race that Sashiko
  pointed out on v3. It already affects remove; with patch 2 it would
  also reach shutdown.
- Patch 2: the Tx queue SW marker wait follows the same bail-out as the
  mailbox transactions, so a control plane that acks DISABLE_QUEUES but
  posts no markers costs one marker timeout instead of one per queue
  (Emil).
- Patch 2: the statistics task stops sending once IDPF_REMOVE_IN_PROG is
  set (Emil).
- Patch 2: the shutdown comment now says that a transaction already in
  flight keeps its own timeout; bounding that would need support in
  libie.
- Not changed: if one teardown reply or Tx queue marker takes longer
  than 500 ms, the rest of the teardown is skipped. On the tested PFs
  the queue-related messages took at most 240 ms, and the slowest,
  DEALLOC_VECTORS (about 300 ms), is sent last, after DESTROY_VPORT.
  With the driver made to treat the first Tx queue marker as missing, so
  that UNMAP_QUEUE_VECTOR, DESTROY_VPORT and DEALLOC_VECTORS were
  skipped, 3 of 3 warm reboots were still clean on these devices. A
  function reset as a fallback was patch 2 of v1; in our tests a
  function reset on shutdown did not stop the faults, so it was dropped.
  I can add one for when the timeout fires if that is preferred.

Changes in v3:
- No new flags: adapter->vc_xn_timeout_ms (60 s from probe) is lowered
  to 500 ms on shutdown, and idpf_send_mb_msg() fails the rest at once
  after a shortened transaction times out (Emil).
- IDPF_REMOVE_IN_PROG is set before the service and event tasks are
  cancelled (Emil); the comment and message no longer state that a reset
  does not clear the vports (Emil).

v4: https://lore.kernel.org/all/20261012063027.27750-1-luckilystar08@gmail.com/
v3: https://lore.kernel.org/all/20261009032706.94324-1-luckilystar08@gmail.com/
v2: https://lore.kernel.org/all/20261007055741.30629-1-luckilystar08@gmail.com/
v1: https://lore.kernel.org/all/20260917105205.37561-1-luckilystar08@gmail.com/

Tian Xun Ng (3):
  idpf: ignore Tx timestamp replies once removal has started
  idpf: ignore link events once removal has started
  idpf: keep the mailbox up while tearing down vports on shutdown

 drivers/net/ethernet/intel/idpf/idpf.h        |  5 +-
 drivers/net/ethernet/intel/idpf/idpf_lib.c    |  3 ++
 drivers/net/ethernet/intel/idpf/idpf_main.c   | 11 +++++
 .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 49 ++++++++++++++++---
 .../net/ethernet/intel/idpf/idpf_virtchnl.h   |  1 +
 .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   |  3 +-
 6 files changed, 64 insertions(+), 8 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH iwl-net v5 1/3] idpf: ignore Tx timestamp replies once removal has started

[Tian Xun Ng]

From: Tian Xun Ng <tianxun.ng@bytedance.com>

On remove, idpf_vc_core_deinit() keeps the virtchnl transaction manager
up until the vports have been torn down, so mbx_task keeps processing
replies while idpf_ptp_release() frees the Tx timestamp trackers. An
async VIRTCHNL2_OP_PTP_GET_VPORT_TX_TSTAMP reply handled in that window
reaches idpf_ptp_get_tx_tstamp_async_handler(), which reads
vport->tx_tstamp_caps and takes its lock after
idpf_ptp_release_vport_tstamp() may already have freed it.

Once IDPF_REMOVE_IN_PROG is set, wait for a mailbox task that may be
running a callback before releasing anything, and make the Tx timestamp
callback return when the bit is set. Matched replies to synchronous
transactions are not affected, so the vport teardown still receives
them; mbx_task is requeued straight away for that.

The last patch of this series sets IDPF_REMOVE_IN_PROG on shutdown too,
which would otherwise expose shutdown to the same window.

Fixes: 1a49cf814fe1 ("idpf: add Tx timestamp flows")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Tian Xun Ng <tianxun.ng@bytedance.com>
---
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c     | 12 +++++++++++-
 drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c |  3 ++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 1caf52706..d0db60647 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -3204,8 +3204,18 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter)
 
 	/* Avoid transaction timeouts when called during reset */
 	remove_in_prog = test_bit(IDPF_REMOVE_IN_PROG, adapter->flags);
-	if (!remove_in_prog)
+	if (!remove_in_prog) {
 		libie_ctlq_xn_shutdown(adapter->xnm);
+	} else {
+		/* The transaction manager stays up for the teardown below, so
+		 * async replies can still arrive. Wait for a mailbox task that
+		 * may be running an async callback; callbacks that run later
+		 * see IDPF_REMOVE_IN_PROG and leave the state released below
+		 * alone.
+		 */
+		cancel_delayed_work_sync(&adapter->mbx_task);
+		queue_delayed_work(adapter->mbx_wq, &adapter->mbx_task, 0);
+	}
 
 	idpf_ptp_release(adapter);
 	idpf_deinit_task(adapter);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
index 14dba40a9..1422b30d0 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
@@ -550,7 +550,8 @@ idpf_ptp_get_tx_tstamp_async_handler(void *ctx, struct kvec *mem, int status)
 	u16 num_latches;
 	u32 vport_id;
 
-	if (status)
+	/* The Tx timestamp trackers are released on remove and shutdown */
+	if (status || test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
 		return;
 
 	recv_tx_tstamp_msg = mem->iov_base;
-- 
2.50.1 (Apple Git-155)

[PATCH iwl-net v5 2/3] idpf: ignore link events once removal has started

[Tian Xun Ng]

From: Tian Xun Ng <tianxun.ng@bytedance.com>

mbx_task keeps handling mailbox events while idpf_deinit_task() releases
the vports: it requeues itself, or is queued from the mailbox interrupt,
until idpf_vc_core_deinit() cancels it once the vports are gone. A
VIRTCHNL2_EVENT_LINK_CHANGE event handled in that window looks its vport
up in adapter->vports[] without a lock, and idpf_vport_dealloc()
releases the netdev and frees the vport before it clears that slot, so
idpf_handle_event_link() can follow a NULL netdev pointer or use the
freed vport.

Make idpf_handle_event_link() return once IDPF_REMOVE_IN_PROG is set.
The previous patch waits for a running mbx_task after the bit is set and
before anything is released, so on remove a link event is either handled
before the vports are torn down or ignored. Shutdown has the same window
today; the last patch of this series sets the bit there as well. The
window also remains on a hard reset, which tears the vports down without
setting the bit; that path is not changed here.

Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Tian Xun Ng <tianxun.ng@bytedance.com>
---
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index d0db60647..2cfd216ae 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -40,6 +40,10 @@ static void idpf_handle_event_link(struct idpf_adapter *adapter,
 	struct idpf_netdev_priv *np;
 	struct idpf_vport *vport;
 
+	/* The vports are released on remove and shutdown */
+	if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
+		return;
+
 	vport = idpf_vid_to_vport(adapter, le32_to_cpu(v2e->vport_id));
 	if (!vport) {
 		dev_err_ratelimited(&adapter->pdev->dev, "Failed to find vport_id %d for link event\n",
-- 
2.50.1 (Apple Git-155)

[PATCH iwl-net v5 3/3] idpf: keep the mailbox up while tearing down vports on shutdown

[Tian Xun Ng]

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
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; the first two
patches keep the Tx timestamp callback and the link event handler away
from the state released on the way. 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 synchronous ones at once after
the first one times out. If the SW marker of one disabled Tx queue does
not arrive, the remaining queues of that vport do not wait for theirs,
but the mailbox messages that follow are still sent: the device has just
answered DISABLE_QUEUES. A control plane that dies at that point costs
one marker timeout per vport and one mailbox timeout. The statistics
task stops sending once IDPF_REMOVE_IN_PROG is set. A request it already
has waiting, or a reset already running in the event task, keeps its own
timeouts, as bounding those 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 series 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, with all 64 Tx queue markers received; the
slowest transaction, VIRTCHNL2_OP_DEALLOC_VECTORS, took about 300 ms.
With the first Tx queue marker treated as missing, the other 63 queues
did not wait, all six transactions still completed, the teardown took
about 2 s per PF, and 3 of 3 warm reboots were clean. With the driver
made to skip sending the teardown messages, 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>
---
 drivers/net/ethernet/intel/idpf/idpf.h        |  5 ++-
 drivers/net/ethernet/intel/idpf/idpf_lib.c    |  3 ++
 drivers/net/ethernet/intel/idpf/idpf_main.c   | 11 +++++++
 .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 33 ++++++++++++++++---
 .../net/ethernet/intel/idpf/idpf_virtchnl.h   |  1 +
 5 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
index 470bc23c8..a789230dd 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -85,7 +85,7 @@ enum idpf_state {
  * @IDPF_HR_FUNC_RESET: Hard reset when TxRx timeout
  * @IDPF_HR_DRV_LOAD: Set on driver load for a clean HW
  * @IDPF_HR_RESET_IN_PROG: Reset in progress
- * @IDPF_REMOVE_IN_PROG: Driver remove in progress
+ * @IDPF_REMOVE_IN_PROG: Driver remove or shutdown in progress
  * @IDPF_MB_INTR_MODE: Mailbox in interrupt mode
  * @IDPF_VC_CORE_INIT: virtchnl core has been init
  * @IDPF_FLAGS_NBITS: Must be last
@@ -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_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 827c795af..e4af4ebd4 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1358,6 +1358,9 @@ void idpf_statistics_task(struct work_struct *work)
 	for (i = 0; i < adapter->max_vports; i++) {
 		struct idpf_vport *vport = adapter->vports[i];
 
+		if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags))
+			return;
+
 		if (vport && !test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags))
 			idpf_send_get_stats_msg(netdev_priv(vport->netdev),
 						&vport->port_stats);
diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
index 129bccaa6..21be83dc8 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_main.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
@@ -194,8 +194,18 @@ 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 the teardown transactions started from here on, so that an
+	 * unresponsive control plane costs a single timeout. A reset or a
+	 * transaction that is already running keeps its own timeouts.
+	 */
+	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 +277,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 2cfd216ae..331d56fae 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -194,20 +194,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
+ * synchronous 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);
@@ -218,10 +230,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;
 }
 
 /**
@@ -371,6 +388,11 @@ static int idpf_wait_for_marker_event_set(const struct idpf_queue_set *qs)
 	struct net_device *netdev;
 	struct idpf_tx_queue *txq;
 	bool markers_rcvd = true;
+	bool wait_all;
+
+	/* On shutdown, once one marker is missing, don't wait for the rest */
+	wait_all = READ_ONCE(qs->adapter->vc_xn_timeout_ms) >=
+		   IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
 
 	for (u32 i = 0; i < qs->num; i++) {
 		switch (qs->qs[i].type) {
@@ -380,7 +402,8 @@ 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);
+			if (markers_rcvd || wait_all)
+				idpf_wait_for_sw_marker_completion(txq);
 			markers_rcvd &= !idpf_queue_has(SW_MARKER, txq);
 			break;
 		default:
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;
-- 
2.50.1 (Apple Git-155)