* [PATCH net v2] net: mana: Avoid waiting on dead HWC after device reset
@ 2026-10-30 12:49 Sahil Chandna
2026-11-03 13:06 ` netdev-bot+sashiko
0 siblings, 1 reply; 2+ messages in thread
From: Sahil Chandna @ 2026-10-30 12:49 UTC (permalink / raw)
To: haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet, kuba,
pabeni, kotaranov, horms, ernis, gargaditya, mawasthi,
kees+treewide, schakrabarti, leitao, linux-hyperv, netdev,
linux-kernel, linux-rdma
Cc: stable
After device reset is issued due to TX drain timeout in
mana_dealloc_queues(), hardware channel (HWC) state is non-functional.
However, the driver continues to issue HWC commands (mana_disable_vport_rx,
mana_fence_rqs, mana_destroy_wq_obj) which will each wait for a
completion that never arrives.
The first such command (MANA_CONFIG_VPORT_RX) blocks for the full
hwc_timeout (60s as set by the firmware on the tested system), after
which the timeout is reduced to 1ms for subsequent commands. This
causes the calling thread to hold the RTNL lock for over 180 seconds
(120s TX drain + 60s HWC timeout), triggering hung task warnings and
blocking all other networking operations on the system.
Fix this by:
1. Replacing the sleep-based TX drain with a CQE polling approach:
disable NAPI per queue, do a final synchronous poll with retries
to collect any remaining completions, free uncompleted SKBs, and
reset the device only if sends remain unacknowledged. The device
is reset at most once per call, and not at all if an earlier reset
already marked the HWC dead. This eliminates the 120s sleep entirely.
2. Setting hwc_dead flag after a successful device reset.
3. Checking hwc_timeout and hwc_dead flag in mana_hwc_send_request()
before taking an HWC message slot and posting the WQE and returning
-ETIMEDOUT immediately without submitting commands to hardware that
cannot respond.
4. Skipping mana_fence_rqs() and the SHM channel teardown handshake
Fixes: a7dfeda6fdec ("net: mana: Fix MANA VF unload when hardware is unresponsive")
Cc: stable@vger.kernel.org
Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
Signed-off-by: Sahil Chandna <sahilchandna@linux.microsoft.com>
---
Changes in v2 :
- Add bounded wait for late TX completions before deciding that a
queue is stuck, poll the CQ up to 3 times, 100 ms apart.
- check hwc_timeout and hwc_dead before taking an HWC message slot,
not only before posting the WQE.
Link to v1: https://lore.kernel.org/all/20261025072838.1991785-1-sahilchandna@linux.microsoft.com/
.../net/ethernet/microsoft/mana/gdma_main.c | 4 +-
.../net/ethernet/microsoft/mana/gdma_pci.c | 2 +-
.../net/ethernet/microsoft/mana/hw_channel.c | 39 ++++--
drivers/net/ethernet/microsoft/mana/mana_en.c | 131 ++++++++++++++----
include/net/mana/hw_channel.h | 1 +
include/net/mana/mana.h | 2 +
6 files changed, 140 insertions(+), 39 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 8c915e008784..b249752ce828 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -153,7 +153,9 @@ bool mana_need_log(struct gdma_context *gc, int err)
return true;
hwc = gc->hwc.driver_data;
- if (hwc && hwc->hwc_timeout == 0)
+ if (hwc &&
+ (READ_ONCE(hwc->hwc_timeout) == 0 ||
+ READ_ONCE(hwc->hwc_dead)))
return false;
return true;
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_pci.c b/drivers/net/ethernet/microsoft/mana/gdma_pci.c
index ed8925296c8a..3995e4ab4c96 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_pci.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_pci.c
@@ -102,7 +102,7 @@ static void mana_serv_reset(struct pci_dev *pdev)
}
/* HWC is not responding in this case, so don't wait */
- hwc->hwc_timeout = 0;
+ WRITE_ONCE(hwc->hwc_timeout, 0);
dev_info(&pdev->dev, "MANA reset cycle start\n");
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 3bca4b683134..56fc6876d8d1 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -192,7 +192,7 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
switch (type) {
case HWC_DATA_CFG_HWC_TIMEOUT:
- hwc->hwc_timeout = val;
+ WRITE_ONCE(hwc->hwc_timeout, val);
break;
case HWC_DATA_HW_LINK_CONNECT:
@@ -819,7 +819,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
* non-zero, the HWC worked and we should tear down the HWC here.
*/
if (gc->max_num_cqs > 0) {
- mana_smc_teardown_hwc(&gc->shm_channel, false);
+ if (READ_ONCE(hwc->hwc_dead))
+ dev_warn(hwc->dev,
+ "HWC unavailable, skip SHM teardown handshake\n");
+ else
+ mana_smc_teardown_hwc(&gc->shm_channel, false);
+
gc->max_num_cqs = 0;
}
@@ -862,9 +867,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
u32 dest_vrcq;
u32 dest_vrq;
u32 command;
+ u32 timeout;
u16 msg_id;
int err;
+ /* Fail fast before taking the HWC slot, after a device reset, a
+ * request already holding it may never complete.
+ */
+
+ if (!READ_ONCE(hwc->hwc_timeout) || READ_ONCE(hwc->hwc_dead))
+ return -ETIMEDOUT;
+
mana_hwc_get_msg_index(hwc, &msg_id);
tx_wr = &txq->msg_buf->reqs[msg_id];
@@ -897,6 +910,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
dest_vrq = hwc->dest_vrq_id;
dest_vrcq = hwc->dest_vrcq_id;
+ /* If the HWC is unavailable (e.g. hwc_timeout was set to 0 or
+ * hwc_dead set after device reset), do not post the WQE, the
+ * hardware cannot respond, so fail fast instead of waiting for
+ * the full timeout.
+ */
+ timeout = READ_ONCE(hwc->hwc_timeout);
+ if (timeout == 0 || READ_ONCE(hwc->hwc_dead)) {
+ err = -ETIMEDOUT;
+ goto out;
+ }
+
err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
if (err) {
dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
@@ -904,14 +928,13 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
}
if (!wait_for_completion_timeout(&ctx->comp_event,
- (msecs_to_jiffies(hwc->hwc_timeout)))) {
- if (hwc->hwc_timeout != 0)
- dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
- command, hwc->hwc_timeout);
+ (msecs_to_jiffies(timeout)))) {
+ dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
+ command, timeout);
/* Reduce further waiting if HWC no response */
- if (hwc->hwc_timeout > 1)
- hwc->hwc_timeout = 1;
+ if (READ_ONCE(hwc->hwc_timeout) > 1)
+ WRITE_ONCE(hwc->hwc_timeout, 1);
err = -ETIMEDOUT;
goto out;
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 6b7c027e497e..ca0b809bfc13 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2086,7 +2086,10 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
if (atomic_sub_return(pkt_transmitted, &txq->pending_sends) < 0)
WARN_ON_ONCE(1);
- /* Feed DIM with the completion rate observed here, in NAPI context. */
+ /* Feed DIM with the completion rate observed here, in NAPI context.
+ * or in the TX drain of mana_dealloc_queues() after NAPI is disabled.
+ */
+
cq->tx_dim_pkts += pkt_transmitted;
cq->tx_dim_bytes += tx_bytes;
@@ -2612,6 +2615,8 @@ static void mana_update_tx_dim(struct mana_cq *cq)
/* cq->tx_dim_pkts/bytes are accumulated in mana_poll_tx_cq(), in the
* same NAPI context as this read, so they track the hardware
* completion rate and need no u64_stats_sync protection.
+ * The TX drain in mana_dealloc_queues() also adds to them,
+ * but only after NAPI is disabled, so not concurrently with this read.
*/
dim_update_sample(READ_ONCE(cq->dim_event_ctr), cq->tx_dim_pkts,
cq->tx_dim_bytes, &dim_sample);
@@ -2725,10 +2730,10 @@ static void mana_destroy_txq(struct mana_port_context *apc)
apc->tx_qp[i]->txq.napi_initialized = false;
}
- /* Forget the SKBs freed without being completed after a drain
- * timeout, now that NAPI can't complete any. Doing it here also
- * covers the queues that a lower channel count won't recreate,
- * which could otherwise stay stopped.
+ /* Forget the SKBs freed in mana_dealloc_queue() without
+ * completion, now that NAPI can't complete any. Doing it
+ * here also covers the queues that a lower channel count
+ * won't recreate, which could otherwise stay stopped.
*/
netdev_tx_reset_queue(apc->tx_qp[i]->txq.net_txq);
@@ -3750,19 +3755,24 @@ int mana_attach(struct net_device *ndev)
return 0;
}
+#define MANA_TX_DRAIN_RETRIES 3
+
static int mana_dealloc_queues(struct net_device *ndev)
{
struct mana_port_context *apc = netdev_priv(ndev);
- unsigned long timeout = jiffies + 120 * HZ;
struct gdma_dev *gd = apc->ac->gdma_dev;
+ struct hw_channel_context *hwc;
+ int flr_pending, i, err, retry;
+ bool trigger_flr = false;
struct mana_txq *txq;
struct sk_buff *skb;
- int i, err;
- u32 tsleep;
+ u32 flr_txq_id = 0;
if (apc->port_is_up)
return -EINVAL;
+ hwc = gd->gdma_context->hwc.driver_data;
+
if (apc->rxqs)
mana_chn_setxdp(apc, NULL);
@@ -3774,42 +3784,104 @@ static int mana_dealloc_queues(struct net_device *ndev)
* a txq because it may not timely see apc->port_is_up being cleared
* to false, but it doesn't matter since mana_start_xmit() drops any
* new packets due to apc->port_is_up being false.
- *
- * Drain all the in-flight TX packets.
- * A timeout of 120 seconds for all the queues is used.
- * This will break the while loop when h/w is not responding.
- * This value of 120 has been decided here considering max
- * number of queues.
+ * Drain the in-flight TX completions for each queue. TX is already
+ * stopped, so pending_sends can no longer grow. For each queue,
+ * disable NAPI so it cannot run at the same time as this drain,
+ * then process all completions still in the CQ. If some are still
+ * missing after that, the hardware is not responding: reset device
+ * and stop waiting on the now-unavailable HWC.
*/
if (apc->tx_qp) {
for (i = 0; i < apc->num_queues; i++) {
+ struct napi_struct *napi;
+ struct mana_cq *tx_cq;
+ int pending;
+
+ if (!apc->tx_qp[i])
+ continue;
+
+ tx_cq = &apc->tx_qp[i]->tx_cq;
txq = &apc->tx_qp[i]->txq;
- tsleep = 1000;
- while (atomic_read(&txq->pending_sends) > 0 &&
- time_before(jiffies, timeout)) {
- usleep_range(tsleep, tsleep + 1000);
- tsleep <<= 1;
- }
- if (atomic_read(&txq->pending_sends)) {
- err = mana_gd_dev_reset(gd->gdma_context);
- if (err) {
- netdev_err(ndev, "device reset failed %d with %d pkts pending in txq %u\n",
- err,
- atomic_read(&txq->pending_sends),
- txq->gdma_txq_id);
+ napi = &tx_cq->napi;
+
+ if (txq->napi_initialized) {
+ /* Stop the NAPI poll so it cannot run
+ * concurrently with the synchronous drain.
+ */
+ napi_synchronize(napi);
+ napi_disable_locked(napi);
+ cancel_work_sync(&tx_cq->dim.work);
+
+ /* Reap all TX completions currently in the CQ.
+ * For completion which have still not arrived
+ * allow time to settle.
+ * A zero budget frees skbs via the non-NAPI
+ * path, which is required outside softirq.
+ */
+ tx_cq->budget = 0;
+ for (retry = 0; retry < MANA_TX_DRAIN_RETRIES;
+ retry++) {
+ do {
+ tx_cq->work_done = 0;
+ mana_poll_tx_cq(tx_cq);
+ } while (tx_cq->work_done);
+
+ if (!atomic_read(&txq->pending_sends) ||
+ trigger_flr)
+ break;
+
+ msleep(100);
}
- break;
}
+
+ pending = atomic_read(&txq->pending_sends);
+
+ if (!trigger_flr && pending > 0) {
+ trigger_flr = true;
+ flr_txq_id = txq->gdma_txq_id;
+ flr_pending = pending;
+ }
+ }
+
+ if (trigger_flr && !(hwc && hwc->hwc_dead)) {
+ /* Completions never arrived: the device is
+ * unresponsive. Issue device reset and
+ * mark the HWC dead so subsequent HWC commands
+ * fail fast instead of each waiting the full
+ * hwc_timeout.
+ */
+ err = mana_gd_dev_reset(gd->gdma_context);
+ if (err)
+ netdev_err(ndev, "device reset failed %d with %d pkts pending in txq %u\n",
+ err,
+ flr_pending,
+ flr_txq_id);
+ else if (hwc)
+ WRITE_ONCE(hwc->hwc_dead, true);
}
for (i = 0; i < apc->num_queues; i++) {
+ struct napi_struct *napi;
+ struct mana_cq *tx_cq;
+
+ if (!apc->tx_qp[i])
+ continue;
+
+ tx_cq = &apc->tx_qp[i]->tx_cq;
+ napi = &tx_cq->napi;
+
txq = &apc->tx_qp[i]->txq;
while ((skb = skb_dequeue(&txq->pending_skbs))) {
mana_unmap_skb(skb, apc);
dev_kfree_skb_any(skb);
}
atomic_set(&txq->pending_sends, 0);
+
+ if (txq->napi_initialized) {
+ netif_napi_del_locked(napi);
+ txq->napi_initialized = false;
+ }
}
}
@@ -3822,7 +3894,8 @@ static int mana_dealloc_queues(struct net_device *ndev)
if (err && mana_en_need_log(apc, err))
netdev_err(ndev, "Failed to disable vPort: %d\n", err);
- mana_fence_rqs(apc);
+ if (!(hwc && hwc->hwc_dead))
+ mana_fence_rqs(apc);
/* Even in err case, still need to cleanup the vPort */
mana_destroy_rxqs(apc);
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 1ab2d66c891e..0bc222c32244 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -179,6 +179,7 @@ struct hw_channel_context {
struct gdma_dev *gdma_dev;
struct device *dev;
+ bool hwc_dead;
u16 num_inflight_msg;
u32 max_req_msg_size;
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index e1fa9131c490..f4ad04b7a106 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -343,6 +343,8 @@ struct mana_cq {
/* Cumulative TX completions fed to DIM. Updated and read only in
* NAPI context (mana_poll_tx_cq() / mana_update_tx_dim()), so they
* measure the hardware completion rate and need no u64_stats_sync.
+ * The TX drain in mana_dealloc_queues() also updates them, but only
+ * after NAPI is disabled and the DIM work is cancelled.
*/
u64 tx_dim_pkts;
u64 tx_dim_bytes;
--
2.53.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH net v2] net: mana: Avoid waiting on dead HWC after device reset
2026-10-30 12:49 [PATCH net v2] net: mana: Avoid waiting on dead HWC after device reset Sahil Chandna
@ 2026-11-03 13:06 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-11-03 13:06 UTC (permalink / raw)
To: sahilchandna
Cc: haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet, kuba,
pabeni, kotaranov, horms, ernis, gargaditya, mawasthi,
kees+treewide, schakrabarti, leitao, linux-hyperv, netdev,
linux-kernel, linux-rdma, stable
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 4 (3) · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The new TX drain in mana_dealloc_queues()
(drivers/net/ethernet/microsoft/mana/mana_en.c, lines 3822-3844)…
- [Medium] In mana_dealloc_queues(), hwc_dead is set only when
mana_gd_dev_reset() succeeds: `if (err) netdev_err(...); else if (hwc)…
- [Medium] mana_hwc_send_request() checks hwc_dead and hwc_timeout
locklessly (READ_ONCE at hw_channel.c:878 and 918-919), then posts the…
- [Medium] mana_hwc_send_request() now returns -ETIMEDOUT without posting
the WQE whenever hwc_timeout == 0 (hw_channel.c:878-879 and 918-922),…
- [Low] The comment this patch rewrote in mana_destroy_txq()
(mana_en.c:2733) says "Forget the SKBs freed in mana_dealloc_queue()…
- [Low] The timeout path of mana_hwc_send_request() does `if
(READ_ONCE(hwc->hwc_timeout) > 1) WRITE_ONCE(hwc->hwc_timeout, 1);`…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but in
mana_dealloc_queues(), when mana_gd_dev_reset() fails after…
- [Medium] This problem wasn't introduced by this patch, but when
wait_for_completion_timeout() expires in mana_hwc_send_request()…
- [Medium] This problem wasn't introduced by this patch, but
mana_hwc_destroy_channel() (hw_channel.c:831-838) frees hwc->txq and…
--
net: mana: Avoid waiting on dead HWC after device reset
This patch replaces the sleep-based TX drain in mana_dealloc_queues(),
which could last up to 120s, with a bounded synchronous CQ poll (3
retries, 100 ms apart) after NAPI is disabled. If sends are still
pending it resets the device at most once. A successful reset sets a new
hwc_dead flag, which mana_hwc_send_request() checks together with
hwc_timeout == 0 so it can return -ETIMEDOUT without posting a WQE, and
mana_fence_rqs() and the SHM teardown handshake are skipped once
hwc_dead is set.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..a50a36ba25710 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -819,7 +819,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
> * non-zero, the HWC worked and we should tear down the HWC here.
> */
> if (gc->max_num_cqs > 0) {
> - mana_smc_teardown_hwc(&gc->shm_channel, false);
> + if (READ_ONCE(hwc->hwc_dead))
> + dev_warn(hwc->dev,
> + "HWC unavailable, skip SHM teardown handshake\n");
> + else
> + mana_smc_teardown_hwc(&gc->shm_channel, false);
> +
> gc->max_num_cqs = 0;
> }
[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Can the
rest of this teardown race with an HWC completion interrupt that is
already running on another CPU?
Setting gc->max_num_cqs = 0 is a plain store. It stops new CQ dispatch,
but it doesn't wait for a mana_hwc_comp_event() that is already in
progress. After that:
mana_hwc_destroy_channel()
mana_hwc_destroy_wq(hwc, hwc->txq) <- frees msg buffers and hwc_wq
mana_hwc_destroy_wq(hwc, hwc->rxq)
mana_hwc_destroy_cq()
kfree(hwc_cq->comp_buf);
mana_gd_destroy_queue(gc, hwc_cq->gdma_eq)
...
mana_gd_deregister_irq() <- list_del_rcu() + synchronize_rcu()
The only drain of the EQ callback happens after comp_buf and the work
queues have been freed. An in-flight handler uses hwc_cq->comp_buf, and
the tx/rx event handlers dereference hwc->txq and hwc->rxq. The hwc_dead
branch added here doesn't change this ordering.
> @@ -862,9 +867,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
[ ... ]
> + /* Fail fast before taking the HWC slot, after a device reset, a
> + * request already holding it may never complete.
> + */
> +
> + if (!READ_ONCE(hwc->hwc_timeout) || READ_ONCE(hwc->hwc_dead))
> + return -ETIMEDOUT;
[Severity: Medium]
Is it intended that hwc_timeout == 0 now means the WQE is never posted?
This check and the second one before mana_hwc_post_tx_wqe() return
-ETIMEDOUT without submitting anything whenever the timeout is 0, not
only after a device reset. Before this patch, a zero timeout meant the
WQE was still posted and wait_for_completion_timeout() was called with 0
jiffies.
mana_serv_reset() sets the zero timeout to mean "don't wait", and it
never resets the device:
/* HWC is not responding in this case, so don't wait */
WRITE_ONCE(hwc->hwc_timeout, 0);
...
mana_gd_suspend(pdev, PMSG_SUSPEND);
That path handles GDMA_EQE_HWC_RESET_REQUEST, gf_stats HWC timeouts and
probe-time recovery. Along mana_gd_suspend() -> mana_remove() ->
mana_detach() -> mana_dealloc_queues(), none of these commands reach the
hardware now: the MANA_CONFIG_VPORT_RX disable, the fences, and the
WQ/CQ object and DMA region destroys. An FLR only happens if TX sends
happen to be pending.
mana_destroy_rxq() still DMA-unmaps and frees the RX buffers and the
page pool while the hardware RQ and RX steering may still be live. The
SHM teardown that makes the hardware invalidate MST entries only runs
later, in mana_gd_cleanup(). If the RX data path is still active at that
point, can RX DMA land in buffers that were already unmapped and freed?
mana_fence_rqs() also still runs on this path, since only hwc_dead is
checked. Its msleep(100) per queue now waits on fences that were never
posted.
> +
> mana_hwc_get_msg_index(hwc, &msg_id);
[ ... ]
> @@ -897,6 +910,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
[ ... ]
> + timeout = READ_ONCE(hwc->hwc_timeout);
> + if (timeout == 0 || READ_ONCE(hwc->hwc_dead)) {
> + err = -ETIMEDOUT;
> + goto out;
> + }
> +
> err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
[Severity: Medium]
What serializes this lockless check against the
WRITE_ONCE(hwc->hwc_dead, true) in mana_dealloc_queues()?
Some HWC users don't hold RTNL, for example
mana_gf_stats_work_handler() -> mana_query_gf_stats() every 2 seconds,
or mana_ib verbs. They can run while mana_dealloc_queues() issues the
FLR. Such a request may already be posted when the FLR happens. It may
also pass this check, get preempted, and then post to the freshly reset
device.
Setting hwc_dead, or hwc_timeout = 0 in mana_serv_reset(), never
completes ctx->comp_event. That request then sleeps in
wait_for_completion_timeout() for the full captured timeout (60s in the
commit message's example).
The commit message says the driver returns "-ETIMEDOUT immediately
without submitting commands to hardware that cannot respond", but in
this window commands are still submitted and wait. Doesn't this also
delay the gf_stats worker's scheduling of the service reset by the same
amount?
> if (err) {
> dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
> @@ -904,14 +928,13 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> }
>
> if (!wait_for_completion_timeout(&ctx->comp_event,
> - (msecs_to_jiffies(hwc->hwc_timeout)))) {
> - if (hwc->hwc_timeout != 0)
> - dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
> - command, hwc->hwc_timeout);
> + (msecs_to_jiffies(timeout)))) {
> + dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
> + command, timeout);
>
> /* Reduce further waiting if HWC no response */
> - if (hwc->hwc_timeout > 1)
> - hwc->hwc_timeout = 1;
> + if (READ_ONCE(hwc->hwc_timeout) > 1)
> + WRITE_ONCE(hwc->hwc_timeout, 1);
[Severity: Low]
This read-check-write isn't atomic. If mana_serv_reset() runs
WRITE_ONCE(hwc->hwc_timeout, 0) between the READ_ONCE() and the
WRITE_ONCE() here, can the 0 be overwritten with 1?
With this patch, hwc_timeout == 0 is the sticky "don't submit" signal
for both new checks above. If it is lost, later requests during the
service-reset teardown take a slot, post a WQE and wait 1 ms instead of
failing fast. mana_need_log() also goes back to returning true for them.
Would a cmpxchg() here keep the zero from being lost?
>
> err = -ETIMEDOUT;
> goto out;
[Severity: Medium]
This isn't a bug introduced by this patch. On this timeout path,
mana_hwc_put_msg_index() at the out label releases the slot while the
posted WQE is still outstanding, and ctx->comp_event is never
reinitialized. A late response is then handled by
mana_hwc_handle_resp():
if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
...
memcpy(ctx->output_buf, resp_msg, resp_len);
...
complete(&ctx->comp_event);
If a new request has taken the same msg_id (find_first_zero_bit() tends
to hand out slot 0 again), can it get the old command's response? Can it
return immediately because of the leftover completion count?
If the handler passes test_bit() just before bitmap_clear() and is then
delayed, can memcpy() write into the stack buffer of a caller that has
already returned?
The 1 ms reduction after the first timeout is kept, which makes such
late responses more likely while the HWC is slow but not dead.
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 6b7c027e497ed..2bf17fdcfa2bd 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -2725,10 +2730,10 @@ static void mana_destroy_txq(struct mana_port_context *apc)
> apc->tx_qp[i]->txq.napi_initialized = false;
> }
>
> - /* Forget the SKBs freed without being completed after a drain
> - * timeout, now that NAPI can't complete any. Doing it here also
> - * covers the queues that a lower channel count won't recreate,
> - * which could otherwise stay stopped.
> + /* Forget the SKBs freed in mana_dealloc_queue() without
> + * completion, now that NAPI can't complete any. Doing it
> + * here also covers the queues that a lower channel count
> + * won't recreate, which could otherwise stay stopped.
> */
[Severity: Low]
This isn't a bug, but there is no function called mana_dealloc_queue().
Should this say mana_dealloc_queues()?
Also, on the dealloc path the NAPI teardown block just above this
comment is now always skipped. mana_dealloc_queues() already calls
netif_napi_del_locked() and clears txq->napi_initialized before it calls
mana_destroy_txq(). Could the comment say that NAPI teardown for this
path now happens in mana_dealloc_queues()?
[ ... ]
> @@ -3774,42 +3784,104 @@ static int mana_dealloc_queues(struct net_device *ndev)
[ ... ]
> if (apc->tx_qp) {
> for (i = 0; i < apc->num_queues; i++) {
[ ... ]
> + if (txq->napi_initialized) {
> + /* Stop the NAPI poll so it cannot run
> + * concurrently with the synchronous drain.
> + */
> + napi_synchronize(napi);
> + napi_disable_locked(napi);
> + cancel_work_sync(&tx_cq->dim.work);
[ ... ]
> + tx_cq->budget = 0;
> + for (retry = 0; retry < MANA_TX_DRAIN_RETRIES;
> + retry++) {
> + do {
> + tx_cq->work_done = 0;
> + mana_poll_tx_cq(tx_cq);
> + } while (tx_cq->work_done);
> +
> + if (!atomic_read(&txq->pending_sends) ||
> + trigger_flr)
> + break;
> +
> + msleep(100);
> }
> - break;
> }
> +
> + pending = atomic_read(&txq->pending_sends);
> +
> + if (!trigger_flr && pending > 0) {
> + trigger_flr = true;
> + flr_txq_id = txq->gdma_txq_id;
> + flr_pending = pending;
> + }
[Severity: Medium]
Is this drain window as long as it looks? On the last pass (retry == 2)
the loop polls, sees pending_sends > 0, does msleep(100), and then exits
without polling again. pending is read straight after that sleep.
pending_sends is only decremented in mana_poll_tx_cq(), and NAPI is
already disabled, so CQEs written during that final sleep are never
counted. In practice, completions are only seen if they arrive within
about 200 ms of napi_disable_locked(). Once trigger_flr is set, every
later queue gets a single poll.
If pending is still nonzero, mana_gd_dev_reset() resets the whole PCI
function:
static int mana_pci_dev_reset(struct gdma_context *gc)
{
return pcie_flr(to_pci_dev(gc->dev));
}
hwc_dead is then set, which affects every vPort and the RDMA aux device
until a service reset rebuilds the HWC.
The driver itself allows much longer TX stalls on healthy hardware, in
mana_probe_port():
/* Recommended timeout based on HW FPGA re-config scenario. */
ndev->watchdog_timeo = 15 * HZ;
Could a slow but working device get a function level reset here during a
routine close, MTU change, channel or ring change, or XDP attach?
The commit message says the device is reset "only if sends remain
unacknowledged", but acknowledgements that arrive during the final sleep
are never checked. Is there a hardware basis for the roughly 200-300 ms
budget?
> + }
> +
> + if (trigger_flr && !(hwc && hwc->hwc_dead)) {
[ ... ]
> + err = mana_gd_dev_reset(gd->gdma_context);
> + if (err)
> + netdev_err(ndev, "device reset failed %d with %d pkts pending in txq %u\n",
> + err,
> + flr_pending,
> + flr_txq_id);
> + else if (hwc)
> + WRITE_ONCE(hwc->hwc_dead, true);
> }
[Severity: Medium]
What happens when mana_gd_dev_reset() fails here? trigger_flr means the
device already looks unresponsive, but the failure branch only logs an
error. hwc_dead stays false and hwc_timeout keeps the firmware value.
The reset can fail in several ways: pcie_flr() fails when pci_dev_wait()
times out, cdx_dev_reset() can fail, and mana_gd_dev_reset() returns
-EOPNOTSUPP when there is no dev_reset op. In that case both new checks
in mana_hwc_send_request() pass:
mana_dealloc_queues()
mana_disable_vport_rx()
...
mana_hwc_send_request()
mana_hwc_post_tx_wqe()
wait_for_completion_timeout() <- full hwc_timeout, e.g. 60s
The !(hwc && hwc->hwc_dead) check doesn't skip mana_fence_rqs() either,
so each RX queue adds another 1 ms timeout plus msleep(100).
All of this runs under RTNL. Can the RTNL stall this patch is meant to
fix still happen when the reset fails?
>
> for (i = 0; i < apc->num_queues; i++) {
[ ... ]
> txq = &apc->tx_qp[i]->txq;
> while ((skb = skb_dequeue(&txq->pending_skbs))) {
> mana_unmap_skb(skb, apc);
> dev_kfree_skb_any(skb);
> }
[Severity: Medium]
This isn't a bug introduced by this patch. When mana_gd_dev_reset()
fails above, this loop still unmaps and frees every SKB on
txq->pending_skbs. The device may still own WQEs for those
unacknowledged sends.
If the device is still alive, can it DMA-read through the stale IOVAs
into memory that has been freed and reused? With an IOMMU that would
show up as IOMMU faults. Without one, unrelated kernel data could be
sent on the wire.
The code before this patch did the same thing: it broke out of the loop
after the failed reset and then freed the SKBs.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030125009.3758592-1-sahilchandna%40linux.microsoft.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-11-03 13:06 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-30 12:49 [PATCH net v2] net: mana: Avoid waiting on dead HWC after device reset Sahil Chandna
2026-11-03 13:06 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®