* [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF
2026-09-08 3:51 [PATCH net-next v5 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
@ 2026-09-08 3:51 ` Long Li
2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
2026-09-08 3:51 ` [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state Long Li
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Long Li @ 2026-09-08 3:51 UTC (permalink / raw)
To: Long Li, Long Li, Konstantin Taranov, Jakub Kicinski,
David S . Miller, Paolo Abeni, Eric Dumazet, Andrew Lunn,
Jason Gunthorpe, Leon Romanovsky, Haiyang Zhang,
K . Y . Srinivasan, Wei Liu, Dexuan Cui, shradhagupta,
Simon Horman, ernis, stephen, shirazsaleem
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
Track possible PF queue ownership with setup_active for HWC reinit.
Set it before submitting ESTABLISH_HWC. Cleanup now attempts DESTROY_HWC
after submitted setup failures even before MAX_NUM_CQS arrives, adding
potentially tens of seconds of polling. Resources are still freed if
teardown fails.
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v5 (v4 -> v5):
- Describe additional teardown attempts and polling after setup failure.
- Clarify the flag's context lifetime and setup precondition.
- Shorten comments; no executable changes from v4.
Changes in v4 (standalone net-next rework after the v3 split):
- Introduce this dedicated handover-tracking preparation patch.
- Set the submission flag inside mana_smc_setup_hwc() and use it as the
cleanup gate, including setup failures before MAX_NUM_CQS arrives.
- Do not carry the separate net series' teardown-failure resource retention.
Changes in v3 (historical net fixes-only posting):
- Handover tracking remained in the teardown-safety patch (5/6).
- That patch dropped pcie_flr() recovery and retained resources after
failed teardown. Those changes are not part of this preparation patch.
Changes in v2 (v1 -> v2):
- Handover tracking remained part of teardown-safety patch 5/7.
- No separate preparation patch was posted.
v1:
- The combined series introduced setup_active in teardown-safety patch 5/7.
drivers/net/ethernet/microsoft/mana/hw_channel.c | 14 +++++++-------
drivers/net/ethernet/microsoft/mana/shm_channel.c | 8 +++++++-
include/net/mana/hw_channel.h | 3 +++
include/net/mana/shm_channel.h | 2 +-
4 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 263e7c4e2934186af037be4c80350a6e322b6771..88e92e94e2e90ff31ca6710a7e9b8e34b5fa191c 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -683,7 +683,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
cq->mem_info.dma_handle,
rq->mem_info.dma_handle,
sq->mem_info.dma_handle,
- eq->eq.msix_index);
+ eq->eq.msix_index, &hwc->setup_active);
if (err)
return err;
@@ -815,13 +815,13 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
if (!hwc)
return;
- /* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
- * 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);
- gc->max_num_cqs = 0;
+ if (hwc->setup_active) {
+ if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
+ hwc->setup_active = false;
+ else
+ dev_err(hwc->dev, "Failed to tear down HWC\n");
}
+ gc->max_num_cqs = 0;
if (hwc->txq)
mana_hwc_destroy_wq(hwc, hwc->txq);
diff --git a/drivers/net/ethernet/microsoft/mana/shm_channel.c b/drivers/net/ethernet/microsoft/mana/shm_channel.c
index d21b5db06e5092d82249fb1053d07f65aa38490c..3cf6a9f8e32c4ff5e5423fc950e88082941e21a8 100644
--- a/drivers/net/ethernet/microsoft/mana/shm_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/shm_channel.c
@@ -129,9 +129,12 @@ void mana_smc_init(struct shm_channel *sc, struct device *dev,
sc->base = base;
}
+/* Requires no outstanding HWC handover. *submitted records possible PF
+ * ownership, including when setup fails after submission.
+ */
int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
u64 cq_addr, u64 rq_addr, u64 sq_addr,
- u32 eq_msix_index)
+ u32 eq_msix_index, bool *submitted)
{
union smc_proto_hdr *hdr;
u16 all_addr_h4bits = 0;
@@ -144,6 +147,8 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
int err;
int i;
+ *submitted = false;
+
/* Ensure VF already has possession of shared memory */
err = mana_smc_poll_register(sc->base, false);
if (err) {
@@ -229,6 +234,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
/* Write 256-message buffer to shared memory (final 32-bit write
* triggers HW to set possession bit to PF).
*/
+ *submitted = true;
dword = (u32 *)shm_buf;
for (i = 0; i < SMC_APERTURE_DWORDS; i++)
writel(*dword++, sc->base + i * SMC_BASIC_UNIT);
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 16feb39616c1bead1a043b3fadc2e18a90651516..befa09674ce5614a955441e75e480a21aa8695fb 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -199,6 +199,9 @@ struct hw_channel_context {
u32 pf_dest_vrcq_id;
u32 hwc_timeout;
+ /* PF may own the queue mappings; state lasts only for this context. */
+ bool setup_active;
+
struct hwc_caller_ctx *caller_ctx;
};
diff --git a/include/net/mana/shm_channel.h b/include/net/mana/shm_channel.h
index dbabcfb95daf3e87b39a657e3a5f23a1508d4f31..e96387d795259d52aeec7b3fe0981825d90a3822 100644
--- a/include/net/mana/shm_channel.h
+++ b/include/net/mana/shm_channel.h
@@ -20,7 +20,7 @@ void mana_smc_init(struct shm_channel *sc, struct device *dev,
int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
u64 cq_addr, u64 rq_addr, u64 sq_addr,
- u32 eq_msix_index);
+ u32 eq_msix_index, bool *submitted);
int mana_smc_teardown_hwc(struct shm_channel *sc, bool reset_vf);
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF
2026-09-08 3:51 ` [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF Long Li
@ 2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 3:52 UTC (permalink / raw)
To: Long Li; +Cc: linux-rdma, linux-hyperv
> Track possible PF queue ownership with setup_active for HWC reinit.
> Set it before submitting ESTABLISH_HWC. Cleanup now attempts DESTROY_HWC
> after submitted setup failures even before MAX_NUM_CQS arrives, adding
> potentially tens of seconds of polling. Resources are still freed if
> teardown fails.
>
> Signed-off-by: Long Li <longli@microsoft.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908035201.402424-1-longli@microsoft.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF
2026-09-08 3:51 ` [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF Long Li
2026-09-09 3:52 ` sashiko-bot
@ 2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 6:53 UTC (permalink / raw)
To: longli
Cc: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, shirazsaleem, netdev, linux-rdma, linux-hyperv,
linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 (1) · Medium: 2 · Low: 0
- [Medium] The patch silently converts a real correctness fix but is
presented as pure preparation and carries no Fixes: tag.
- [Medium] `hwc->setup_active` is written in `mana_smc_setup_hwc()`
(shm_channel.c:149 and 237) and read/cleared in…
Pre-existing issues:
- [High] In `mana_hwc_destroy_channel()`
(drivers/net/ethernet/microsoft/mana/hw_channel.c:818-824), a failed…
--
net: mana: track when the HWC has been handed to the PF
This adds a setup_active flag to struct hw_channel_context, set inside
mana_smc_setup_hwc() around the point where the ESTABLISH_HWC message is
published to the PF, and uses that flag instead of gc->max_num_cqs as the
gate for issuing DESTROY_HWC from mana_hwc_destroy_channel().
A few questions on the behaviour changes that come with this.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 263e7c4e29341..88e92e94e2e90 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -683,7 +683,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
> cq->mem_info.dma_handle,
> rq->mem_info.dma_handle,
> sq->mem_info.dma_handle,
> - eq->eq.msix_index);
> + eq->eq.msix_index, &hwc->setup_active);
> if (err)
> return err;
>
> @@ -815,13 +815,13 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
> if (!hwc)
> return;
>
> - /* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
> - * 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);
> - gc->max_num_cqs = 0;
> + if (hwc->setup_active) {
> + if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
> + hwc->setup_active = false;
[Severity: Medium]
The changelog presents this as preparation work, but doesn't this hunk also
change behaviour on an existing failure path?
gc->max_num_cqs is only set once HWC_INIT_DATA_MAX_NUM_CQS arrives in
mana_hwc_init_event_handler(). So with the old gate, when ESTABLISH_HWC had
already been submitted but establish failed before that data arrived,
DESTROY_HWC was skipped entirely.
Two such cases in mana_hwc_establish_channel():
if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
return -ETIMEDOUT;
and a failing mana_smc_read_response() inside mana_smc_setup_hwc(). In both
cases mana_hwc_create_channel() -> out: mana_hwc_destroy_channel() then went
on to free the EQ/CQ/RQ/SQ DMA memory whose frame addresses had already been
published to the PF.
mana_smc_teardown_hwc() documents why that ordering matters:
/* Read shmem response (polling for VF possession) and validate.
* For teardown, waiting for response is required to ensure hardware
* invalidates MST entries before software frees memory.
*/
Since the new gate closes that window, should this patch carry a Fixes: tag
and state that it fixes the skipped teardown, instead of being described as
tracking/preparation only?
> + else
> + dev_err(hwc->dev, "Failed to tear down HWC\n");
> }
> + gc->max_num_cqs = 0;
>
> if (hwc->txq)
> mana_hwc_destroy_wq(hwc, hwc->txq);
[Severity: High]
This isn't a bug introduced by this patch, since the pre-patch code ignored
the mana_smc_teardown_hwc() return value as well, but the new gate makes the
failing-teardown case reachable for every submitted-but-unconfirmed
establish, so it seems worth asking about here.
On failure the return value is only logged, setup_active is left true, and
execution falls straight through to:
if (hwc->txq)
mana_hwc_destroy_wq(hwc, hwc->txq);
if (hwc->rxq)
mana_hwc_destroy_wq(hwc, hwc->rxq);
if (hwc->cq)
mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
...
kfree(hwc);
Those helpers reach mana_hwc_dealloc_dma_buf() and mana_gd_destroy_queue()
-> mana_gd_free_memory() (dma_free_coherent), releasing exactly the four
regions whose frame addresses were handed to the PF by the ESTABLISH_HWC
writel loop.
mana_smc_teardown_hwc() fails when mana_smc_poll_register() times out after
roughly 20 seconds because the PF never returns aperture possession, or when
mana_smc_read_response() reports an error or an invalid response. In that
state its own comment says the MST entries may still be valid:
* For teardown, waiting for response is required to ensure hardware
* invalidates MST entries before software frees memory.
Can the device then keep writing EQEs/CQEs into pages handed back to the
allocator, or into unmapped pages once the IOMMU mapping is gone? The EQ
MSI-X is also released while the PF may still raise it, and kfree(hwc) drops
the state that would be needed to retry the teardown later.
The commit message says "Resources are still freed if teardown fails" - what
makes freeing safe here, given the comment above?
> diff --git a/drivers/net/ethernet/microsoft/mana/shm_channel.c b/drivers/net/ethernet/microsoft/mana/shm_channel.c
> index d21b5db06e509..3cf6a9f8e32c4 100644
> --- a/drivers/net/ethernet/microsoft/mana/shm_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/shm_channel.c
> @@ -129,9 +129,12 @@ void mana_smc_init(struct shm_channel *sc, struct device *dev,
> sc->base = base;
> }
>
> +/* Requires no outstanding HWC handover. *submitted records possible PF
> + * ownership, including when setup fails after submission.
> + */
> int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
> u64 cq_addr, u64 rq_addr, u64 sq_addr,
> - u32 eq_msix_index)
> + u32 eq_msix_index, bool *submitted)
> {
[ ... ]
> @@ -229,6 +234,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
> /* Write 256-message buffer to shared memory (final 32-bit write
> * triggers HW to set possession bit to PF).
> */
> + *submitted = true;
> dword = (u32 *)shm_buf;
> for (i = 0; i < SMC_APERTURE_DWORDS; i++)
> writel(*dword++, sc->base + i * SMC_BASIC_UNIT);
[Severity: Medium]
setup_active is stored here (and cleared at function entry) and is
read/cleared in mana_hwc_destroy_channel(), with no lock and no
READ_ONCE()/WRITE_ONCE(). What serializes the two contexts?
mana_schedule_serv_work() queues the worker on the system workqueue:
INIT_WORK(&mns_wk->serv_work, mana_serv_func);
schedule_work(&mns_wk->serv_work);
Neither mana_gd_remove() nor mana_gd_cleanup_device() cancels or flushes it,
and the worker path mana_serv_reset() -> mana_gd_suspend()/mana_gd_resume()
-> mana_gd_setup() -> mana_hwc_create_channel() -> mana_smc_setup_hwc() runs
without device_lock, so it can overlap remove/shutdown/PM.
That gives this interleaving on the single shared-memory aperture:
CPU0 mana_smc_setup_hwc()
*submitted = true;
writel() loop /* ESTABLISH_HWC handed to PF */
mana_smc_read_response() /* polling */
CPU1 mana_hwc_destroy_channel()
if (hwc->setup_active) { /* reads true */
if (!mana_smc_teardown_hwc(...)) /* writels DESTROY_HWC */
Can CPU0 then consume the DESTROY_HWC request header written by CPU1 (a
bogus -EPROTO), or CPU1 consume the ESTABLISH_HWC response, leaving the
PF/VF handover state inconsistent while both also race on the plain
read-modify-write of setup_active?
With the old gc->max_num_cqs > 0 gate this window was closed by accident,
because the counter is still zero during the setup handover, so the destroy
path issued no shared-memory access at all.
The underlying unserialized create-versus-destroy of the HWC context
predates this patch, and later in the series mana_hwc_destroy_channel()
gains the comment "The caller must serialize setup and teardown operations".
Would it help to state that requirement in this patch, next to the new
setup_active field, since this is where the flag starts gating aperture
access?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state
2026-09-08 3:51 [PATCH net-next v5 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
2026-09-08 3:51 ` [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF Long Li
@ 2026-09-08 3:51 ` Long Li
2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
2026-09-08 3:51 ` [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests Long Li
2026-09-08 3:51 ` [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
3 siblings, 2 replies; 13+ messages in thread
From: Long Li @ 2026-09-08 3:51 UTC (permalink / raw)
To: Long Li, Long Li, Konstantin Taranov, Jakub Kicinski,
David S . Miller, Paolo Abeni, Eric Dumazet, Andrew Lunn,
Jason Gunthorpe, Leon Romanovsky, Haiyang Zhang,
K . Y . Srinivasan, Wei Liu, Dexuan Cui, shradhagupta,
Simon Horman, ernis, stephen, shirazsaleem
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
Add per-slot locking, sender/response references and a responded flag;
make ctx->error signed. Stop copying after buffer withdrawal or completion.
Return a response recorded at the timeout check instead of -ETIMEDOUT,
without shortening later waits.
Ignore zero timeout-query replies; asynchronous updates remain unfiltered.
Timed-out slots remain reusable; the next patch adds quarantine.
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v5 (v4 -> v5):
- Describe buffer withdrawal and response precedence at the timeout check.
- State that zero filtering applies only to timeout-query replies.
- Shorten the message and comment; no executable changes from v4.
Changes in v4 (standalone net-next rework after the v3 split):
- Extract per-slot ownership into this preparation patch: lock,
sender/response references, responded flag and signed error.
- Initialize the slot before publishing its bitmap bit.
- Return a recorded response when it races timeout; ignore zero timeout-
query replies. Keep timed-out slot reuse here for the next patch to change.
Changes in v3 (historical net fixes-only posting):
- A separate stale-response fix (6/6) supplied per-slot locking/references
and buffer withdrawal, together with a channel timeout latch and
asynchronous zero-timeout filtering. The latter policy is not carried here.
Changes in v2 (v1 -> v2):
- Per-slot state remained within concurrency patch 6/7.
- Its channel-lifetime accounting and teardown drain were revised; see
the concurrency patch's history.
v1:
- Per-slot locking and references were introduced in concurrency patch 6/7.
.../net/ethernet/microsoft/mana/gdma_main.c | 6 +-
.../net/ethernet/microsoft/mana/hw_channel.c | 167 +++++++++++++++---
include/net/mana/hw_channel.h | 18 +-
3 files changed, 163 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index f92b2d0bf926e1b715ff665d37f8173a2103e6fe..8d86de0a334b21d77ab6bfb578917c56404bc856 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -331,7 +331,11 @@ static int mana_gd_query_hwc_timeout(struct pci_dev *pdev, u32 *timeout_val)
if (err || resp.hdr.status)
return err ? err : -EPROTO;
- *timeout_val = resp.timeout_ms;
+ /* Keep the current timeout on a zero query reply. Asynchronous
+ * HWC_DATA_CFG_HWC_TIMEOUT updates remain unfiltered.
+ */
+ if (resp.timeout_ms)
+ *timeout_val = resp.timeout_ms;
return 0;
}
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 88e92e94e2e90ff31ca6710a7e9b8e34b5fa191c..6605e7a9c481bcb11c95f90f627b7c422b62cc28 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -6,9 +6,11 @@
#include <net/mana/hw_channel.h>
#include <linux/vmalloc.h>
+/* Acquire a free inflight message slot, waiting for one if all are in use. */
static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
{
struct gdma_resource *r = &hwc->inflight_msg_res;
+ struct hwc_caller_ctx *ctx;
unsigned long flags;
u32 index;
@@ -19,6 +21,17 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
index = find_first_zero_bit(hwc->inflight_msg_res.map,
hwc->inflight_msg_res.size);
+ ctx = &hwc->caller_ctx[index];
+ reinit_completion(&ctx->comp_event);
+ /* Take both references (sender + handle_resp) before publishing the
+ * slot, so an early response cannot free it under the sender.
+ */
+ refcount_set(&ctx->refcnt, 2);
+ ctx->responded = false;
+ ctx->msg_id = index;
+ ctx->error = -EINPROGRESS;
+
+ /* Publish the slot last, after it is fully initialised. */
bitmap_set(hwc->inflight_msg_res.map, index, 1);
spin_unlock_irqrestore(&r->lock, flags);
@@ -40,6 +53,13 @@ static void mana_hwc_put_msg_index(struct hw_channel_context *hwc, u16 msg_id)
up(&hwc->sema);
}
+static void hwc_ctx_put(struct hw_channel_context *hwc,
+ struct hwc_caller_ctx *ctx)
+{
+ if (refcount_dec_and_test(&ctx->refcnt))
+ mana_hwc_put_msg_index(hwc, ctx->msg_id);
+}
+
static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx *caller_ctx,
const struct gdma_resp_hdr *resp_msg,
u32 resp_len)
@@ -90,22 +110,35 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
}
ctx = hwc->caller_ctx + msg_id;
- err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
- if (err)
- goto out;
- ctx->status_code = resp_msg->status;
+ spin_lock(&ctx->lock);
- memcpy(ctx->output_buf, resp_msg, resp_len);
-out:
+ /* Honour a response only while the sender owns the slot (output_buf
+ * published) and has not already been answered; otherwise drop it as
+ * premature, stale or duplicate without touching the refcount.
+ */
+ if (!ctx->output_buf || ctx->responded) {
+ spin_unlock(&ctx->lock);
+ mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+ return;
+ }
+ ctx->responded = true;
+
+ err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
+ if (!err) {
+ ctx->status_code = resp_msg->status;
+ memcpy(ctx->output_buf, resp_msg, resp_len);
+ }
ctx->error = err;
- /* Must post rx wqe before complete(), otherwise the next rx may
- * hit no_wqe error.
+ /* Post RX WQE before completing — the next response may arrive
+ * immediately and needs a posted buffer.
*/
mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
-
complete(&ctx->comp_event);
+ spin_unlock(&ctx->lock);
+
+ hwc_ctx_put(hwc, ctx);
}
static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -657,8 +690,10 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
if (!ctx)
return -ENOMEM;
- for (i = 0; i < q_depth; ++i)
+ for (i = 0; i < q_depth; ++i) {
+ spin_lock_init(&ctx[i].lock);
init_completion(&ctx[i].comp_event);
+ }
hwc->caller_ctx = ctx;
@@ -669,6 +704,12 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
u32 *max_req_msg_size,
u32 *max_resp_msg_size)
{
+ /* mana_hwc_init_event_handler() fills the bootstrap fields from hard
+ * IRQ on GDMA_EQE_HWC_INIT_DATA and then signals hwc_init_eqe_comp on
+ * GDMA_EQE_HWC_INIT_DONE. The wait_for_completion() below pairs with
+ * that complete(), so every value stored before INIT_DONE is ordered
+ * against the reads that follow it here.
+ */
struct hw_channel_context *hwc = gc->hwc.driver_data;
struct gdma_queue *rq = hwc->rxq->gdma_wq;
struct gdma_queue *sq = hwc->txq->gdma_wq;
@@ -860,13 +901,19 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
struct hwc_wq *txq = hwc->txq;
struct gdma_req_hdr *req_msg;
struct hwc_caller_ctx *ctx;
+ unsigned long flags;
+ bool drop_resp_ref;
u32 dest_vrcq = 0;
u32 dest_vrq = 0;
u32 command;
+ u32 status;
+ u32 wait_ms;
u16 msg_id;
int err;
- mana_hwc_get_msg_index(hwc, &msg_id);
+ err = mana_hwc_get_msg_index(hwc, &msg_id);
+ if (err)
+ return err;
tx_wr = &txq->msg_buf->reqs[msg_id];
@@ -878,8 +925,11 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
}
ctx = hwc->caller_ctx + msg_id;
+
+ spin_lock_irqsave(&ctx->lock, flags);
ctx->output_buf = resp;
ctx->output_buflen = resp_len;
+ spin_unlock_irqrestore(&ctx->lock, flags);
req_msg = (struct gdma_req_hdr *)tx_wr->buf_va;
if (req)
@@ -895,43 +945,108 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
dest_vrcq = hwc->pf_dest_vrcq_id;
}
+ /* The response-side reference (from get_msg_index) keeps the slot
+ * alive if hardware responds right after the doorbell.
+ */
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);
goto out;
}
+ wait_ms = hwc->hwc_timeout;
if (!wait_for_completion_timeout(&ctx->comp_event,
- (msecs_to_jiffies(hwc->hwc_timeout)))) {
- if (hwc->hwc_timeout != 0)
+ msecs_to_jiffies(wait_ms))) {
+ /* Clear output_buf so a late response cannot write the caller's
+ * buffer, then check whether one already arrived
+ * (error != -EINPROGRESS).
+ */
+ spin_lock_irqsave(&ctx->lock, flags);
+ ctx->output_buf = NULL;
+ err = ctx->error;
+ status = ctx->status_code;
+ spin_unlock_irqrestore(&ctx->lock, flags);
+
+ if (err != -EINPROGRESS) {
+ /* A response raced in just after the timeout, so the
+ * hardware is alive: keep the channel and report what
+ * that response said rather than a timeout. It may
+ * itself be an error -- a malformed response leaves
+ * -EPROTO here -- which is still the answer to this
+ * command.
+ */
+ hwc_ctx_put(hwc, ctx);
+ goto check_status;
+ }
+
+ if (wait_ms != 0)
dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
- command, hwc->hwc_timeout);
+ command, wait_ms);
- /* Reduce further waiting if HWC no response */
+ err = -ETIMEDOUT;
+
+ /* No-wait teardown (hwc_timeout == 0) is expected to expire;
+ * just release the slot so the next teardown command can reuse
+ * it.
+ */
+ if (wait_ms == 0)
+ goto out;
+
+ /* Genuine timeout: shorten later waits so subsequent commands
+ * fail fast instead of each draining the full timeout.
+ */
if (hwc->hwc_timeout > 1)
hwc->hwc_timeout = 1;
- err = -ETIMEDOUT;
+ /* Release the slot via out:; a late response no longer touches
+ * it, so the sender must drop the reference here.
+ */
goto out;
}
- if (ctx->error) {
- err = ctx->error;
- goto out;
- }
+ /* Clear output_buf and read the result under the lock; the slot may
+ * be reused after hwc_ctx_put().
+ */
+ spin_lock_irqsave(&ctx->lock, flags);
+ ctx->output_buf = NULL;
+ err = ctx->error;
+ status = ctx->status_code;
+ spin_unlock_irqrestore(&ctx->lock, flags);
+ hwc_ctx_put(hwc, ctx);
+
+check_status:
+ if (err)
+ goto done;
- if (ctx->status_code && ctx->status_code != GDMA_STATUS_MORE_ENTRIES) {
- if (ctx->status_code == GDMA_STATUS_CMD_UNSUPPORTED) {
+ if (status && status != GDMA_STATUS_MORE_ENTRIES) {
+ if (status == GDMA_STATUS_CMD_UNSUPPORTED) {
err = -EOPNOTSUPP;
- goto out;
+ goto done;
}
+
if (command != MANA_QUERY_PHY_STAT)
dev_err(hwc->dev, "Command 0x%x failed with status: 0x%x\n",
- command, ctx->status_code);
+ command, status);
err = -EPROTO;
- goto out;
+ goto done;
}
+
+ err = 0;
+ goto done;
out:
- mana_hwc_put_msg_index(hwc, msg_id);
+ /* Error, no-wait teardown, or timeout: drop the sender's and the
+ * response-side references. Latch ->responded so a racing response
+ * is a no-op, and only drop the response-side ref if it has not.
+ */
+ ctx = hwc->caller_ctx + msg_id;
+ spin_lock_irqsave(&ctx->lock, flags);
+ ctx->output_buf = NULL;
+ drop_resp_ref = !ctx->responded;
+ ctx->responded = true;
+ spin_unlock_irqrestore(&ctx->lock, flags);
+ if (drop_resp_ref)
+ refcount_dec(&ctx->refcnt);
+ hwc_ctx_put(hwc, ctx);
+done:
return err;
}
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index befa09674ce5614a955441e75e480a21aa8695fb..b377e221aa5c8183825e65ac0972c6b9f959004c 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -171,8 +171,24 @@ struct hwc_caller_ctx {
void *output_buf;
u32 output_buflen;
- u32 error; /* Linux error code */
+ int error; /* Linux error code (negative errno or 0) */
u32 status_code;
+
+ /* Protects output_buf against concurrent access from
+ * handle_resp() (CQ interrupt) and the sender timeout path.
+ */
+ spinlock_t lock;
+
+ /* Tracks sender + handle_resp ownership. The last put
+ * (refcount reaches 0) releases the bitmap slot.
+ */
+ refcount_t refcnt;
+ u16 msg_id;
+
+ /* Set by the first handle_resp(), or by the sender's timeout path,
+ * so a later or duplicate response is dropped.
+ */
+ bool responded;
};
struct hw_channel_context {
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state
2026-09-08 3:51 ` [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state Long Li
@ 2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 3:52 UTC (permalink / raw)
To: Long Li; +Cc: linux-hyperv, linux-rdma
> Add per-slot locking, sender/response references and a responded flag;
> make ctx->error signed. Stop copying after buffer withdrawal or completion.
> Return a response recorded at the timeout check instead of -ETIMEDOUT,
> without shortening later waits.
>
> Ignore zero timeout-query replies; asynchronous updates remain unfiltered.
> Timed-out slots remain reusable; the next patch adds quarantine.
>
> Signed-off-by: Long Li <longli@microsoft.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908035201.402424-1-longli@microsoft.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state
2026-09-08 3:51 ` [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state Long Li
2026-09-09 3:52 ` sashiko-bot
@ 2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 6:53 UTC (permalink / raw)
To: longli
Cc: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, shirazsaleem, netdev, linux-rdma, linux-hyperv,
linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 1
- [Medium] The commit message presents this patch as pure preparation
("Add per-slot locking, sender/response references and a responded…
- [Low] The new field comments added to struct hwc_caller_ctx in
include/net/mana/hw_channel.h describe a narrower contract than…
Pre-existing issues:
- [High] In this patch, mana_hwc_send_request()'s out: label latches
ctx->responded, clears output_buf and drops both the sender and the…
- [Medium] hwc->hwc_timeout is read and read-modify-written by senders in
process context (`wait_ms = hwc->hwc_timeout;` and `if…
--
net: mana: give each HWC message slot its own completion state
This adds a per-slot spinlock, a sender/response reference pair and a
responded flag to struct hwc_caller_ctx, makes ctx->error signed, and
withdraws the caller's response buffer once the sender stops waiting.
It also ignores a zero timeout value in a GDMA_QUERY_HWC_TIMEOUT reply.
No Link: tags in the commit header.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 88e92e94e2e90..6605e7a9c481b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -19,6 +21,17 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
> index = find_first_zero_bit(hwc->inflight_msg_res.map,
> hwc->inflight_msg_res.size);
>
> + ctx = &hwc->caller_ctx[index];
> + reinit_completion(&ctx->comp_event);
> + /* Take both references (sender + handle_resp) before publishing the
> + * slot, so an early response cannot free it under the sender.
> + */
> + refcount_set(&ctx->refcnt, 2);
> + ctx->responded = false;
> + ctx->msg_id = index;
> + ctx->error = -EINPROGRESS;
> +
> + /* Publish the slot last, after it is fully initialised. */
> bitmap_set(hwc->inflight_msg_res.map, index, 1);
>
> spin_unlock_irqrestore(&r->lock, flags);
[ ... ]
> @@ -90,22 +110,35 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
> }
>
> ctx = hwc->caller_ctx + msg_id;
> - err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> - if (err)
> - goto out;
>
> - ctx->status_code = resp_msg->status;
> + spin_lock(&ctx->lock);
>
> - memcpy(ctx->output_buf, resp_msg, resp_len);
> -out:
> + /* Honour a response only while the sender owns the slot (output_buf
> + * published) and has not already been answered; otherwise drop it as
> + * premature, stale or duplicate without touching the refcount.
> + */
> + if (!ctx->output_buf || ctx->responded) {
> + spin_unlock(&ctx->lock);
> + mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> + return;
> + }
> + ctx->responded = true;
[Severity: Medium]
Should this hunk carry a Fixes: tag rather than being described as
preparation?
The commit message says:
Add per-slot locking, sender/response references and a responded flag;
make ctx->error signed. Stop copying after buffer withdrawal or
completion.
"Stop copying after buffer withdrawal or completion" reads like a refactor
item, but at the baseline mana_hwc_send_request() released the message slot
on timeout while leaving ctx->output_buf and ctx->output_buflen pointing at
the caller's response buffer, which is usually on the caller's stack:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_query_hwc_timeout() {
struct gdma_query_hwc_timeout_resp resp = {};
...
err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
}
The baseline mana_hwc_handle_resp() then copied a late response into that
pointer as soon as the bit was set again by the next sender:
if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
...
}
ctx = hwc->caller_ctx + msg_id;
err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
...
memcpy(ctx->output_buf, resp_msg, resp_len);
With HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1 the same index is always
reused, so the write lands in a stack frame that has already returned, and
the length check runs against the stale output_buflen.
Is the new output_buf test here, together with the ctx->output_buf = NULL
withdrawal in mana_hwc_send_request(), what closes that window? If so, would
a Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
Adapter (MANA)") and a mention of the corruption in the changelog make this
visible to stable? The changelog notes the work was re-targeted from a "net
fixes-only posting" to net-next, which makes the fix easy to miss.
>
> err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> + if (!err) {
> + ctx->status_code = resp_msg->status;
> + memcpy(ctx->output_buf, resp_msg, resp_len);
> + }
> ctx->error = err;
>
> - /* Must post rx wqe before complete(), otherwise the next rx may
> - * hit no_wqe error.
> + /* Post RX WQE before completing — the next response may arrive
> + * immediately and needs a posted buffer.
> */
> mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> -
> complete(&ctx->comp_event);
> + spin_unlock(&ctx->lock);
> +
> + hwc_ctx_put(hwc, ctx);
> }
[ ... ]
> @@ -878,8 +925,11 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> }
>
> ctx = hwc->caller_ctx + msg_id;
> +
> + spin_lock_irqsave(&ctx->lock, flags);
> ctx->output_buf = resp;
> ctx->output_buflen = resp_len;
> + spin_unlock_irqrestore(&ctx->lock, flags);
[ ... ]
> @@ -895,43 +945,108 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> + wait_ms = hwc->hwc_timeout;
> if (!wait_for_completion_timeout(&ctx->comp_event,
> - (msecs_to_jiffies(hwc->hwc_timeout)))) {
> - if (hwc->hwc_timeout != 0)
> + msecs_to_jiffies(wait_ms))) {
> + /* Clear output_buf so a late response cannot write the caller's
> + * buffer, then check whether one already arrived
> + * (error != -EINPROGRESS).
> + */
> + spin_lock_irqsave(&ctx->lock, flags);
> + ctx->output_buf = NULL;
> + err = ctx->error;
> + status = ctx->status_code;
> + spin_unlock_irqrestore(&ctx->lock, flags);
[ ... ]
> out:
> - mana_hwc_put_msg_index(hwc, msg_id);
> + /* Error, no-wait teardown, or timeout: drop the sender's and the
> + * response-side references. Latch ->responded so a racing response
> + * is a no-op, and only drop the response-side ref if it has not.
> + */
> + ctx = hwc->caller_ctx + msg_id;
> + spin_lock_irqsave(&ctx->lock, flags);
> + ctx->output_buf = NULL;
> + drop_resp_ref = !ctx->responded;
> + ctx->responded = true;
> + spin_unlock_irqrestore(&ctx->lock, flags);
> + if (drop_resp_ref)
> + refcount_dec(&ctx->refcnt);
> + hwc_ctx_put(hwc, ctx);
> +done:
> return err;
> }
> diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
> index befa09674ce56..b377e221aa5c8 100644
> --- a/include/net/mana/hw_channel.h
> +++ b/include/net/mana/hw_channel.h
> @@ -171,8 +171,24 @@ struct hwc_caller_ctx {
> void *output_buf;
> u32 output_buflen;
>
> - u32 error; /* Linux error code */
> + int error; /* Linux error code (negative errno or 0) */
> u32 status_code;
> +
> + /* Protects output_buf against concurrent access from
> + * handle_resp() (CQ interrupt) and the sender timeout path.
> + */
> + spinlock_t lock;
> +
> + /* Tracks sender + handle_resp ownership. The last put
> + * (refcount reaches 0) releases the bitmap slot.
> + */
> + refcount_t refcnt;
> + u16 msg_id;
> +
> + /* Set by the first handle_resp(), or by the sender's timeout path,
> + * so a later or duplicate response is dropped.
> + */
> + bool responded;
> };
[Severity: Low]
These three comments look narrower than what hw_channel.c actually relies
on. Could they be extended?
The lock comment mentions output_buf only, but the out: block in
mana_hwc_send_request() also uses it to serialize ->responded and to decide
who owns the response-side reference:
spin_lock_irqsave(&ctx->lock, flags);
ctx->output_buf = NULL;
drop_resp_ref = !ctx->responded;
ctx->responded = true;
spin_unlock_irqrestore(&ctx->lock, flags);
and ctx->error / ctx->status_code are read and written under the same lock
in both mana_hwc_handle_resp() and mana_hwc_send_request().
The responded comment says it is set "by the first handle_resp(), or by the
sender's timeout path", but the out: block above is also reached from the
req_len > tx_wr->buf_len rejection, from mana_hwc_post_tx_wqe() failure, and
from the no-wait teardown path where hwc_timeout is 0.
The error comment says "Linux error code (negative errno or 0)", while
mana_hwc_get_msg_index() stores -EINPROGRESS as an in-band "no response yet"
state that mana_hwc_send_request() special-cases with
if (err != -EINPROGRESS). Should that sentinel be documented next to the
field, since every reader of ->error has to know about it?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests
2026-09-08 3:51 [PATCH net-next v5 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
2026-09-08 3:51 ` [PATCH net-next v5 1/4] net: mana: track when the HWC has been handed to the PF Long Li
2026-09-08 3:51 ` [PATCH net-next v5 2/4] net: mana: give each HWC message slot its own completion state Long Li
@ 2026-09-08 3:51 ` Long Li
2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
2026-09-08 3:51 ` [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
3 siblings, 2 replies; 13+ messages in thread
From: Long Li @ 2026-09-08 3:51 UTC (permalink / raw)
To: Long Li, Long Li, Konstantin Taranov, Jakub Kicinski,
David S . Miller, Paolo Abeni, Eric Dumazet, Andrew Lunn,
Jason Gunthorpe, Leon Romanovsky, Haiyang Zhang,
K . Y . Srinivasan, Wei Liu, Dexuan Cui, shradhagupta,
Simon Horman, ernis, stephen, shirazsaleem
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
Serialize SQ posting and protect HWC lookup and sender accounting with
hwc_lock. Teardown stops admission, force-completes requests and drains
senders before destroying CQ, TXQ and RXQ. Preserve cancellation errors;
return -EPROTO instead of success for a response accepted before posting.
Bound FIFO slot admission with down_timeout(), independently of the
response wait. Admission expiry returns -ETIMEDOUT even for contention,
so existing callers may reset a responsive channel.
Quarantine posted requests on timeout, including zero-timeout cleanup,
until a response or teardown releases their slot. This closes the
late-response reuse window present at depth one. Keep depth one here.
Existing lifecycle and timeout-field races are not resolved here.
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v5 (v4 -> v5):
- Return -EPROTO rather than success if a response precedes submission;
preserve nonzero cancellation errors and existing reference cleanup.
- Document separate FIFO admission and response budgets, including
contention-triggered recovery and zero-timeout quarantine.
- Correct and shorten locking, publication and teardown comments.
Changes in v4 (standalone net-next rework after the v3 split):
- Rework former patch 6/7 as patch 3/4 on the new ownership preparation.
- Use down_timeout() for semaphore admission and keep the slot until a
timed-out request's response arrives or teardown releases it.
- Retain quarantine for zero-timeout cleanup; do not use the former
channel-wide timeout latch.
- Serialize cancellation with SQ posting, preserve already recorded
results during teardown, and retain guarded sender draining.
Changes in v3 (historical net fixes-only posting):
- Defer the concurrency feature from the net submission.
- Related slot-timeout ownership work was posted separately as fix 6/6.
Changes in v2 (v1 -> v2, former patch 6/7):
- Replace atomic sender accounting with an hwc_lock-protected count and
wait_event_lock_irq() drain to fence the final sender's wakeup.
- Move force-completion and draining before teardown/FLR failure exits.
v1:
- Introduce waitqueue/bitmap admission, per-slot synchronization,
posting serialization and channel teardown gating in patch 6/7.
.../net/ethernet/microsoft/mana/gdma_main.c | 38 ++-
.../net/ethernet/microsoft/mana/hw_channel.c | 229 ++++++++++++++----
include/net/mana/gdma.h | 9 +
include/net/mana/hw_channel.h | 16 ++
4 files changed, 239 insertions(+), 53 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 8d86de0a334b21d77ab6bfb578917c56404bc856..eb88bae2b14d86de33e79eb597a076a7d6e54436 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -162,6 +162,8 @@ static int mana_gd_init_registers(struct pci_dev *pdev)
bool mana_need_log(struct gdma_context *gc, int err)
{
struct hw_channel_context *hwc;
+ bool need_log = true;
+ unsigned long flags;
if (err != -ETIMEDOUT)
return true;
@@ -169,11 +171,13 @@ bool mana_need_log(struct gdma_context *gc, int err)
if (!gc)
return true;
+ spin_lock_irqsave(&gc->hwc_lock, flags);
hwc = gc->hwc.driver_data;
if (hwc && hwc->hwc_timeout == 0)
- return false;
+ need_log = false;
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
- return true;
+ return need_log;
}
static int mana_gd_query_max_resources(struct pci_dev *pdev)
@@ -391,9 +395,27 @@ static int mana_gd_detect_devices(struct pci_dev *pdev)
int mana_gd_send_request(struct gdma_context *gc, u32 req_len, const void *req,
u32 resp_len, void *resp)
{
- struct hw_channel_context *hwc = gc->hwc.driver_data;
+ struct hw_channel_context *hwc;
+ unsigned long flags;
+ int err;
+
+ spin_lock_irqsave(&gc->hwc_lock, flags);
+ hwc = gc->hwc.driver_data;
+ if (!hwc) {
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
+ return -ENODEV;
+ }
+ hwc->active_senders++;
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
+ err = mana_hwc_send_request(hwc, req_len, req, resp_len, resp);
+
+ spin_lock_irqsave(&gc->hwc_lock, flags);
+ if (--hwc->active_senders == 0)
+ wake_up(&gc->hwc_drain_waitq);
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
- return mana_hwc_send_request(hwc, req_len, req, resp_len, resp);
+ return err;
}
EXPORT_SYMBOL_NS(mana_gd_send_request, "NET_MANA");
@@ -714,6 +736,7 @@ static void mana_serv_reset(struct pci_dev *pdev)
{
struct gdma_context *gc = pci_get_drvdata(pdev);
struct hw_channel_context *hwc;
+ unsigned long flags;
int ret;
if (!gc) {
@@ -723,14 +746,17 @@ static void mana_serv_reset(struct pci_dev *pdev)
return;
}
+ spin_lock_irqsave(&gc->hwc_lock, flags);
hwc = gc->hwc.driver_data;
if (!hwc) {
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
dev_err(&pdev->dev, "MANA service: no HWC\n");
goto out;
}
/* HWC is not responding in this case, so don't wait */
hwc->hwc_timeout = 0;
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
dev_info(&pdev->dev, "MANA reset cycle start\n");
@@ -1337,6 +1363,7 @@ static int mana_gd_create_dma_region(struct gdma_dev *gd,
if (gmi->nr_pages == 0 && !MANA_PAGE_ALIGNED(gmi->virt_addr))
return -EINVAL;
+ /* The caller must keep the HWC alive throughout queue creation. */
hwc = gc->hwc.driver_data;
req_msg_size = struct_size(req, page_addr_list, num_page);
if (req_msg_size > hwc->max_req_msg_size)
@@ -1542,7 +1569,9 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
struct hw_channel_context *hwc;
int err;
+ /* The setup caller must exclude concurrent HWC teardown. */
hwc = gc->hwc.driver_data;
+
mana_gd_init_req_hdr(&req.hdr, GDMA_VERIFY_VF_DRIVER_VERSION,
sizeof(req), sizeof(resp));
@@ -2536,6 +2565,7 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
mutex_init(&gc->eq_test_event_mutex);
mutex_init(&gc->gic_mutex);
+ spin_lock_init(&gc->hwc_lock);
pci_set_drvdata(pdev, gc);
gc->bar0_pa = pci_resource_start(pdev, 0);
gc->bar0_size = pci_resource_len(pdev, 0);
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 6605e7a9c481bcb11c95f90f627b7c422b62cc28..a4f7346d285f740c40f4f63c20348e30531f1435 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -6,7 +6,6 @@
#include <net/mana/hw_channel.h>
#include <linux/vmalloc.h>
-/* Acquire a free inflight message slot, waiting for one if all are in use. */
static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
{
struct gdma_resource *r = &hwc->inflight_msg_res;
@@ -14,12 +13,30 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
unsigned long flags;
u32 index;
- down(&hwc->sema);
+ /* FIFO slot admission has a separate budget from the response wait.
+ * Expiry reports -ETIMEDOUT even while earlier requests make progress,
+ * so callers may initiate recovery on contention alone.
+ */
+ if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
+ return -ETIMEDOUT;
spin_lock_irqsave(&r->lock, flags);
- index = find_first_zero_bit(hwc->inflight_msg_res.map,
- hwc->inflight_msg_res.size);
+ if (!hwc->channel_up) {
+ spin_unlock_irqrestore(&r->lock, flags);
+ up(&hwc->sema);
+ return -ENODEV;
+ }
+
+ /* The semaphore admits at most r->size holders at a time, so a slot
+ * acquired above always has a free bit waiting for it here.
+ */
+ index = find_first_zero_bit(r->map, r->size);
+ if (WARN_ON_ONCE(index >= r->size)) {
+ spin_unlock_irqrestore(&r->lock, flags);
+ up(&hwc->sema);
+ return -EIO;
+ }
ctx = &hwc->caller_ctx[index];
reinit_completion(&ctx->comp_event);
@@ -28,11 +45,12 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
*/
refcount_set(&ctx->refcnt, 2);
ctx->responded = false;
+ ctx->resp_pending = true;
ctx->msg_id = index;
ctx->error = -EINPROGRESS;
/* Publish the slot last, after it is fully initialised. */
- bitmap_set(hwc->inflight_msg_res.map, index, 1);
+ bitmap_set(r->map, index, 1);
spin_unlock_irqrestore(&r->lock, flags);
@@ -101,6 +119,7 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
{
const struct gdma_resp_hdr *resp_msg = rx_req->buf_va;
struct hwc_caller_ctx *ctx;
+ bool release;
int err;
if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
@@ -113,13 +132,30 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
spin_lock(&ctx->lock);
- /* Honour a response only while the sender owns the slot (output_buf
- * published) and has not already been answered; otherwise drop it as
- * premature, stale or duplicate without touching the refcount.
+ /* The sender has not published its buffer yet, so nothing asked for
+ * this response. Keep the slot reserved and drop the message.
+ */
+ if (!ctx->output_buf && !ctx->responded) {
+ spin_unlock(&ctx->lock);
+ mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+ return;
+ }
+
+ /* Take the response-side reference away exactly once: releasing it
+ * is what frees a slot whose sender has already given up.
*/
- if (!ctx->output_buf || ctx->responded) {
+ release = ctx->resp_pending;
+ ctx->resp_pending = false;
+
+ if (ctx->responded) {
+ /* The sender timed out and abandoned the slot, or a response
+ * was already applied. Consume this one without writing
+ * anything, then release the slot it was holding.
+ */
spin_unlock(&ctx->lock);
mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
+ if (release)
+ hwc_ctx_put(hwc, ctx);
return;
}
ctx->responded = true;
@@ -138,7 +174,8 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
complete(&ctx->comp_event);
spin_unlock(&ctx->lock);
- hwc_ctx_put(hwc, ctx);
+ if (release)
+ hwc_ctx_put(hwc, ctx);
}
static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
@@ -593,6 +630,7 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
hwc_wq->gdma_wq = queue;
hwc_wq->queue_depth = q_depth;
hwc_wq->hwc_cq = hwc_cq;
+ spin_lock_init(&hwc_wq->lock);
err = mana_hwc_alloc_dma_buf(hwc, q_depth, max_msg_size,
&hwc_wq->msg_buf);
@@ -610,7 +648,7 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
return err;
}
-static int mana_hwc_post_tx_wqe(const struct hwc_wq *hwc_txq,
+static int mana_hwc_post_tx_wqe(struct hwc_wq *hwc_txq,
struct hwc_work_request *req,
u32 dest_virt_rq_id, u32 dest_virt_rcq_id,
bool dest_pf)
@@ -649,7 +687,10 @@ static int mana_hwc_post_tx_wqe(const struct hwc_wq *hwc_txq,
req->wqe_req.inline_oob_data = tx_oob;
req->wqe_req.client_data_unit = 0;
+ spin_lock(&hwc_txq->lock);
err = mana_gd_post_and_ring(hwc_txq->gdma_wq, &req->wqe_req, NULL);
+ spin_unlock(&hwc_txq->lock);
+
if (err)
dev_err(dev, "Failed to post WQE on HWC SQ: %d\n", err);
return err;
@@ -675,6 +716,7 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
struct hwc_wq *hwc_rxq = hwc->rxq;
struct hwc_work_request *req;
struct hwc_caller_ctx *ctx;
+ unsigned long flags;
int err;
int i;
@@ -697,7 +739,19 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
hwc->caller_ctx = ctx;
- return mana_gd_test_eq(gc, hwc->cq->gdma_eq);
+ /* Enable admission for the test EQ request. */
+ spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+ hwc->channel_up = true;
+ spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+
+ err = mana_gd_test_eq(gc, hwc->cq->gdma_eq);
+ if (err) {
+ spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+ hwc->channel_up = false;
+ spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+ }
+
+ return err;
}
static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
@@ -797,6 +851,7 @@ int mana_hwc_create_channel(struct gdma_context *gc)
u32 max_req_msg_size, max_resp_msg_size;
struct gdma_dev *gd = &gc->hwc;
struct hw_channel_context *hwc;
+ unsigned long flags;
u16 q_depth_max;
int err;
@@ -805,10 +860,11 @@ int mana_hwc_create_channel(struct gdma_context *gc)
return -ENOMEM;
gd->gdma_context = gc;
- gd->driver_data = hwc;
hwc->gdma_dev = gd;
hwc->dev = gc->dev;
hwc->hwc_timeout = HW_CHANNEL_WAIT_RESOURCE_TIMEOUT_MS;
+ hwc->active_senders = 0;
+ init_waitqueue_head(&gc->hwc_drain_waitq);
/* HWC's instance number is always 0. */
gd->dev_id.as_uint32 = 0;
@@ -817,6 +873,11 @@ int mana_hwc_create_channel(struct gdma_context *gc)
gd->pdid = INVALID_PDID;
gd->doorbell = INVALID_DOORBELL;
+ /* Publish for setup; queue initialization below must precede senders. */
+ spin_lock_irqsave(&gc->hwc_lock, flags);
+ gc->hwc.driver_data = hwc;
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
/* mana_hwc_init_queues() only creates the required data structures,
* and doesn't touch the HWC device.
*/
@@ -851,11 +912,60 @@ int mana_hwc_create_channel(struct gdma_context *gc)
void mana_hwc_destroy_channel(struct gdma_context *gc)
{
+ /* The caller must serialize setup and teardown operations. */
struct hw_channel_context *hwc = gc->hwc.driver_data;
+ unsigned long flags;
if (!hwc)
return;
+ /* Nonzero num_inflight_msg means queue initialization completed. */
+ if (hwc->num_inflight_msg) {
+ spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
+ hwc->channel_up = false;
+ spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
+ }
+
+ /* Block new mana_gd_send_request() references before draining. */
+ spin_lock_irqsave(&gc->hwc_lock, flags);
+ gc->hwc.driver_data = NULL;
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
+ /* Complete occupied slots and drop pending response-side references. */
+ if (hwc->caller_ctx) {
+ struct hwc_caller_ctx *ctx;
+ bool drop_resp_ref;
+ int i;
+
+ for (i = 0; i < hwc->num_inflight_msg; i++) {
+ if (!test_bit(i, hwc->inflight_msg_res.map))
+ continue;
+
+ ctx = &hwc->caller_ctx[i];
+
+ spin_lock_irqsave(&ctx->lock, flags);
+ /* Preserve an already recorded result. */
+ if (!ctx->responded)
+ ctx->error = -ENODEV;
+ drop_resp_ref = ctx->resp_pending;
+ ctx->resp_pending = false;
+ ctx->responded = true;
+ complete(&ctx->comp_event);
+ spin_unlock_irqrestore(&ctx->lock, flags);
+
+ if (drop_resp_ref)
+ hwc_ctx_put(hwc, ctx);
+ }
+ }
+
+ /* Pair with the last sender's wakeup under hwc_lock, so it finishes
+ * accessing gc before the drain returns.
+ */
+ spin_lock_irq(&gc->hwc_lock);
+ wait_event_lock_irq(gc->hwc_drain_waitq,
+ hwc->active_senders == 0, gc->hwc_lock);
+ spin_unlock_irq(&gc->hwc_lock);
+
if (hwc->setup_active) {
if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
hwc->setup_active = false;
@@ -864,14 +974,28 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
}
gc->max_num_cqs = 0;
+ /* Deregister the HWC EQ before freeing the work queues. */
+ if (hwc->cq)
+ mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
+
if (hwc->txq)
mana_hwc_destroy_wq(hwc, hwc->txq);
if (hwc->rxq)
mana_hwc_destroy_wq(hwc, hwc->rxq);
- if (hwc->cq)
- mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
+ if (hwc->caller_ctx) {
+ struct hwc_caller_ctx *ctx;
+ int i;
+
+ for (i = 0; i < hwc->num_inflight_msg; i++) {
+ if (!test_bit(i, hwc->inflight_msg_res.map))
+ continue;
+
+ ctx = &hwc->caller_ctx[i];
+ hwc_ctx_put(hwc, ctx);
+ }
+ }
kfree(hwc->caller_ctx);
hwc->caller_ctx = NULL;
@@ -886,7 +1010,6 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
hwc->hwc_timeout = 0;
kfree(hwc);
- gc->hwc.driver_data = NULL;
gc->hwc.gdma_context = NULL;
vfree(gc->cq_table);
@@ -903,6 +1026,8 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
struct hwc_caller_ctx *ctx;
unsigned long flags;
bool drop_resp_ref;
+ bool abandoned = false;
+ bool cancelled;
u32 dest_vrcq = 0;
u32 dest_vrq = 0;
u32 command;
@@ -945,10 +1070,21 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
dest_vrcq = hwc->pf_dest_vrcq_id;
}
- /* The response-side reference (from get_msg_index) keeps the slot
- * alive if hardware responds right after the doorbell.
+ /* Serialize cancellation with submission. An unsubmitted request
+ * cannot succeed, even if an unsolicited response was accepted.
*/
- err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
+ spin_lock_irqsave(&ctx->lock, flags);
+ cancelled = ctx->responded;
+ if (cancelled)
+ err = ctx->error ?: -EPROTO;
+ else
+ err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq,
+ false);
+ spin_unlock_irqrestore(&ctx->lock, flags);
+
+ if (cancelled)
+ goto out;
+
if (err) {
dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
goto out;
@@ -965,43 +1101,40 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
ctx->output_buf = NULL;
err = ctx->error;
status = ctx->status_code;
+ if (err == -EINPROGRESS) {
+ /* Publish abandonment with buffer withdrawal so a late
+ * response can reclaim the slot. Keep its reference.
+ */
+ ctx->responded = true;
+ abandoned = true;
+ }
spin_unlock_irqrestore(&ctx->lock, flags);
- if (err != -EINPROGRESS) {
- /* A response raced in just after the timeout, so the
- * hardware is alive: keep the channel and report what
- * that response said rather than a timeout. It may
- * itself be an error -- a malformed response leaves
- * -EPROTO here -- which is still the answer to this
- * command.
- */
+ if (!abandoned) {
+ /* A completion won the race with timeout; use its result. */
hwc_ctx_put(hwc, ctx);
goto check_status;
}
- if (wait_ms != 0)
+ if (wait_ms != 0) {
dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
command, wait_ms);
- err = -ETIMEDOUT;
-
- /* No-wait teardown (hwc_timeout == 0) is expected to expire;
- * just release the slot so the next teardown command can reuse
- * it.
- */
- if (wait_ms == 0)
- goto out;
+ /* Genuine timeout: shorten later waits so subsequent
+ * commands fail fast instead of each draining the
+ * full timeout.
+ */
+ if (hwc->hwc_timeout > 1)
+ hwc->hwc_timeout = 1;
+ }
- /* Genuine timeout: shorten later waits so subsequent commands
- * fail fast instead of each draining the full timeout.
- */
- if (hwc->hwc_timeout > 1)
- hwc->hwc_timeout = 1;
+ err = -ETIMEDOUT;
- /* Release the slot via out:; a late response no longer touches
- * it, so the sender must drop the reference here.
+ /* Drop only the sender's reference; the response-side one is
+ * what keeps the slot reserved.
*/
- goto out;
+ hwc_ctx_put(hwc, ctx);
+ goto done;
}
/* Clear output_buf and read the result under the lock; the slot may
@@ -1034,14 +1167,12 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
err = 0;
goto done;
out:
- /* Error, no-wait teardown, or timeout: drop the sender's and the
- * response-side references. Latch ->responded so a racing response
- * is a no-op, and only drop the response-side ref if it has not.
- */
+ /* Release any references still held by this unsubmitted request. */
ctx = hwc->caller_ctx + msg_id;
spin_lock_irqsave(&ctx->lock, flags);
ctx->output_buf = NULL;
- drop_resp_ref = !ctx->responded;
+ drop_resp_ref = ctx->resp_pending;
+ ctx->resp_pending = false;
ctx->responded = true;
spin_unlock_irqrestore(&ctx->lock, flags);
if (drop_resp_ref)
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 308950f9b54b0485bac66b80d63e257eaf5f787e..571a533e62e64790f9000d42ab0e833fe36ccff6 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -468,6 +468,15 @@ struct gdma_context {
/* Hardware communication channel (HWC) */
struct gdma_dev hwc;
+ /* Sender drain; the final wakeup runs under hwc_lock. */
+ wait_queue_head_t hwc_drain_waitq;
+
+ /* Protects HWC publication, sender references, and short accesses in
+ * mana_need_log()/mana_serv_reset(). Setup and DMA-region readers
+ * still require lifecycle ordering. Not all timeout writers use it.
+ */
+ spinlock_t hwc_lock;
+
/* Azure network adapter */
struct gdma_dev mana;
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index b377e221aa5c8183825e65ac0972c6b9f959004c..fba27d8620a388a41ae7ddd3bf2b4792f7beec3d 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -164,6 +164,9 @@ struct hwc_wq {
u16 queue_depth;
struct hwc_cq *hwc_cq;
+
+ /* Serializes SQ posting; unused for the RQ. */
+ spinlock_t lock;
};
struct hwc_caller_ctx {
@@ -189,6 +192,9 @@ struct hwc_caller_ctx {
* so a later or duplicate response is dropped.
*/
bool responded;
+
+ /* Response-side reference outstanding; protected by lock. */
+ bool resp_pending;
};
struct hw_channel_context {
@@ -196,6 +202,7 @@ struct hw_channel_context {
struct device *dev;
u16 num_inflight_msg;
+
u32 max_req_msg_size;
u16 hwc_init_q_depth_max;
@@ -208,6 +215,9 @@ struct hw_channel_context {
struct hwc_wq *txq;
struct hwc_cq *cq;
+ /* Admission permits. Timed-out requests retain theirs until a
+ * response or teardown releases the slot.
+ */
struct semaphore sema;
struct gdma_resource inflight_msg_res;
@@ -215,9 +225,15 @@ struct hw_channel_context {
u32 pf_dest_vrcq_id;
u32 hwc_timeout;
+ /* Checked after slot acquisition; cleared on teardown to reject sends. */
+ bool channel_up;
+
/* PF may own the queue mappings; state lasts only for this context. */
bool setup_active;
+ /* mana_gd_send_request() callers, including waiters; under hwc_lock. */
+ unsigned int active_senders;
+
struct hwc_caller_ctx *caller_ctx;
};
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests
2026-09-08 3:51 ` [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests Long Li
@ 2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 3:52 UTC (permalink / raw)
To: Long Li; +Cc: linux-rdma, linux-hyperv
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Preexisting use-after-free of `hwc_cq->comp_buf` during HWC CQ destruction due to a race with the interrupt handler.
--
commit a87caaea6f635bdcf7c331ad2a7a65f2ff025635
Author: Long Li <longli@microsoft.com>
Subject: net: mana: support concurrent HWC requests
This commit serializes SQ posting and protects HWC lookup and sender accounting
with hwc_lock. Teardown now stops admission, force-completes requests, and
drains senders before destroying the CQ, TXQ, and RXQ.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 6605e7a9c481b..a4f7346d285f7 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -864,14 +974,28 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
> }
> gc->max_num_cqs = 0;
>
> + /* Deregister the HWC EQ before freeing the work queues. */
> + if (hwc->cq)
> + mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> +
[Severity: High]
This is a pre-existing issue, but since this patch moves the call to
mana_hwc_destroy_cq(), is it worth fixing a use-after-free bug in that
function's teardown path?
In mana_hwc_destroy_cq(), hwc_cq->comp_buf is freed before the EQ is
destroyed:
mana_hwc_destroy_cq() {
kfree(hwc_cq->comp_buf);
if (hwc_cq->gdma_cq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
if (hwc_cq->gdma_eq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
...
}
Because mana_gd_destroy_queue() on the EQ is what performs synchronize_rcu()
to wait for concurrent interrupt handlers to finish, a racing interrupt
handler on another CPU could still execute mana_hwc_comp_event() after the
buffer is freed:
mana_hwc_comp_event() {
...
completions = hwc_cq->comp_buf;
comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
...
}
Could this lead to a use-after-free and memory corruption when
mana_gd_poll_cq() writes hardware completion events into the freed
completions buffer?
> if (hwc->txq)
> mana_hwc_destroy_wq(hwc, hwc->txq);
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908035201.402424-1-longli@microsoft.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests
2026-09-08 3:51 ` [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests Long Li
2026-09-09 3:52 ` sashiko-bot
@ 2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 6:53 UTC (permalink / raw)
To: longli
Cc: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, shirazsaleem, netdev, linux-rdma, linux-hyperv,
linux-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 2
- [High] mana_hwc_get_msg_index() converts pure FIFO-slot contention into
-ETIMEDOUT: `if (down_timeout(&hwc->sema,…
- [Medium] The newly added gc->hwc_lock is documented as protecting
hwc->hwc_timeout accesses in mana_need_log()/mana_serv_reset(), but…
- [Low] The patch closes a real, user-visible late-response
misassociation bug (a timed-out request's slot was released…
- [Low] The new hwc_caller_ctx.resp_pending field is documented in
include/net/mana/hw_channel.h as 'Response-side reference outstanding;…
Pre-existing issues:
- [High] mana_hwc_destroy_cq() frees the completion buffer as its very
first action (`kfree(hwc_cq->comp_buf);`), then destroys the GDMA CQ,…
- [Medium] The new channel_up admission gate is evaluated only after
blocking on hwc->sema and after acquiring hwc->inflight_msg_res.lock,…
--
net: mana: support concurrent HWC requests
This adds hwc_lock to protect HWC publication and sender accounting,
serializes SQ posting with a new per-wq spinlock, bounds FIFO slot
admission with down_timeout(), and quarantines timed-out requests until a
response or teardown releases the slot. Teardown now stops admission,
force-completes outstanding requests and drains senders before destroying
the CQ, TXQ and RXQ.
A few questions below.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 6605e7a9c481b..a4f7346d285f7 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -6,7 +6,6 @@
> #include <net/mana/hw_channel.h>
> #include <linux/vmalloc.h>
>
> -/* Acquire a free inflight message slot, waiting for one if all are in use. */
> static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
> {
> struct gdma_resource *r = &hwc->inflight_msg_res;
> @@ -14,12 +13,30 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
> unsigned long flags;
> u32 index;
>
> - down(&hwc->sema);
> + /* FIFO slot admission has a separate budget from the response wait.
> + * Expiry reports -ETIMEDOUT even while earlier requests make progress,
> + * so callers may initiate recovery on contention alone.
> + */
> + if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
> + return -ETIMEDOUT;
[Severity: High]
Can pure slot contention here trigger a device reset on healthy hardware?
The channel is built at HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1 in
mana_hwc_create_channel(), so sema_init() grants a single permit and any
two concurrent mana_gd_send_request() callers contend for it. The loser
gets -ETIMEDOUT, which mana_gd_send_request() propagates unchanged.
Existing callers read that code as "the HWC is dead":
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_probe() {
if (err == -ETIMEDOUT || err == -EPROTO) {
...
schedule_delayed_work(&mana_dev_recovery_work.work, ...);
}
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
ret = mana_gd_resume(pdev);
if (ret == -ETIMEDOUT || ret == -EPROTO) {
... mana_serv_rescan(pdev);
}
mana_serv_rescan() calls pci_stop_and_remove_bus_device() followed by
pci_rescan_bus(). In mana_en the gf-stats work latches
ac->hwc_timeout_occurred = true on -ETIMEDOUT and stops collecting stats
permanently.
The admission budget is also the same field the response-timeout path in
mana_hwc_send_request() shrinks:
if (hwc->hwc_timeout > 1)
hwc->hwc_timeout = 1;
and the field mana_serv_reset() sets to 0, so after either event
down_timeout() takes the expired path immediately for a second caller even
though the channel is responsive.
Would returning a distinguishable code for contention, such as -EBUSY or
-EAGAIN, avoid teaching every caller to reset the device on queue depth
pressure? The commit message acknowledges this:
"Admission expiry returns -ETIMEDOUT even for contention, so existing
callers may reset a responsive channel."
Checking the end of the series, mana_hwc_get_msg_index() is byte-identical
there, so the later dynamic HWC queue depth work does not address this; it
only makes exhaustion less likely when the device advertises a depth
greater than one.
>
> spin_lock_irqsave(&r->lock, flags);
>
> - index = find_first_zero_bit(hwc->inflight_msg_res.map,
> - hwc->inflight_msg_res.size);
> + if (!hwc->channel_up) {
> + spin_unlock_irqrestore(&r->lock, flags);
> + up(&hwc->sema);
> + return -ENODEV;
> + }
> +
> + /* The semaphore admits at most r->size holders at a time, so a slot
> + * acquired above always has a free bit waiting for it here.
> + */
> + index = find_first_zero_bit(r->map, r->size);
> + if (WARN_ON_ONCE(index >= r->size)) {
> + spin_unlock_irqrestore(&r->lock, flags);
> + up(&hwc->sema);
> + return -EIO;
> + }
>
> ctx = &hwc->caller_ctx[index];
> reinit_completion(&ctx->comp_event);
> @@ -28,11 +45,12 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
> */
> refcount_set(&ctx->refcnt, 2);
> ctx->responded = false;
> + ctx->resp_pending = true;
[Severity: Low]
The new field is documented in include/net/mana/hw_channel.h as:
/* Response-side reference outstanding; protected by lock. */
bool resp_pending;
but this initializing write runs under hwc->inflight_msg_res.lock only,
and ctx->lock is never taken in mana_hwc_get_msg_index(). Every other
write to the field (mana_hwc_handle_resp(), the timeout path and the out:
path of mana_hwc_send_request(), and mana_hwc_destroy_channel()) does hold
ctx->lock.
On the consumer side, mana_hwc_handle_resp() gates on a lock-free
test_bit(msg_id, hwc->inflight_msg_res.map) before acquiring ctx->lock, so
the publishing stores to responded/resp_pending and the bitmap bit share no
lock or barrier with the reader that consumes them.
Since msg_id comes from the RX buffer and is read with READ_ONCE() in
mana_hwc_rx_event_handler() precisely because the host can modify it, a
response for the freshly published msg_id can arrive between bitmap_set()
and the sender's first spin_lock(&ctx->lock). If that handler observed the
new resp_pending == true together with a stale responded == true it would
consume the response-side reference, and a later sender timeout would then
drop the refcount to zero and reopen the late-response reuse window this
patch closes.
Should the comment name inflight_msg_res.lock plus the publication
ordering, or should the initialization move under ctx->lock?
> ctx->msg_id = index;
> ctx->error = -EINPROGRESS;
>
> /* Publish the slot last, after it is fully initialised. */
> - bitmap_set(hwc->inflight_msg_res.map, index, 1);
> + bitmap_set(r->map, index, 1);
>
> spin_unlock_irqrestore(&r->lock, flags);
>
[ ... ]
> @@ -817,6 +873,11 @@ int mana_hwc_create_channel(struct gdma_context *gc)
> gd->pdid = INVALID_PDID;
> gd->doorbell = INVALID_DOORBELL;
>
> + /* Publish for setup; queue initialization below must precede senders. */
> + spin_lock_irqsave(&gc->hwc_lock, flags);
> + gc->hwc.driver_data = hwc;
> + spin_unlock_irqrestore(&gc->hwc_lock, flags);
> +
> /* mana_hwc_init_queues() only creates the required data structures,
> * and doesn't touch the HWC device.
> */
[Severity: Medium]
This isn't a bug introduced by this patch, since the baseline also assigned
gd->driver_data = hwc before mana_hwc_init_queues(), but does the new
channel_up gate give the impression that lookups are now safe when it is
ordered after the blocking acquire?
driver_data is published here, while sema_init() and
mana_gd_alloc_res_map() (which allocates the bitmap and initializes
inflight_msg_res.lock) only run inside mana_hwc_init_queues() ->
mana_hwc_init_inflight_msg(), and caller_ctx plus channel_up = true only
appear later in mana_hwc_test_channel().
mana_gd_send_request() gates on nothing else:
hwc = gc->hwc.driver_data;
if (!hwc) { ... return -ENODEV; }
hwc->active_senders++;
so a sender that observes the freshly published pointer reaches
mana_hwc_get_msg_index() and executes:
if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
on a kzalloc'ed semaphore (count == 0, wait_list.next/prev == NULL, so
___down_common()'s list_add_tail() walks NULL pointers), then
spin_lock_irqsave(&r->lock, flags) on a never-initialized spinlock, all
before the channel_up check can reject it.
The same window exists in the depth-renegotiation path added later in the
series, which calls mana_hwc_destroy_queues() (frees the bitmap, kfrees
caller_ctx) and then mana_hwc_init_queues() again while driver_data stays
published.
I could not exhibit a concurrent sender, since mana_hwc_create_channel()
runs from mana_gd_setup() during probe and resume where the clients have
been removed or not yet probed. Is the requirement stated by the two new
comments ("queue initialization below must precede senders", "The setup
caller must exclude concurrent HWC teardown") enforced anywhere in code?
[ ... ]
> @@ -864,14 +974,28 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
> }
> gc->max_num_cqs = 0;
>
> + /* Deregister the HWC EQ before freeing the work queues. */
> + if (hwc->cq)
> + mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> +
> if (hwc->txq)
> mana_hwc_destroy_wq(hwc, hwc->txq);
>
> if (hwc->rxq)
> mana_hwc_destroy_wq(hwc, hwc->rxq);
>
> - if (hwc->cq)
> - mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
[Severity: High]
This is a pre-existing issue in the helper rather than something this patch
introduces, but since the comment here now makes mana_hwc_destroy_cq() the
fence for the interrupt handler, does that helper actually fence anything
before it frees the buffers the handler writes?
mana_hwc_destroy_cq() does:
kfree(hwc_cq->comp_buf);
if (hwc_cq->gdma_cq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
if (hwc_cq->gdma_eq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
Only the last step reaches the barrier, via mana_gd_destroy_eq() ->
mana_gd_deregister_irq():
list_del_rcu(&eq->entry);
...
synchronize_rcu();
Meanwhile the hard-IRQ callback writes into the buffer that was already
freed and dereferences the queue that was already destroyed:
completions = hwc_cq->comp_buf;
comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
An interrupt taken in that interval reaches mana_gd_intr() -> the HWC EQ,
which is still on gic->eq_list -> mana_gd_process_eqe() ->
mana_hwc_comp_event(), so can this write into the kfree()d comp_buf?
The gc->max_num_cqs = 0 store just above helps for the completion path,
because mana_gd_process_eqe() does:
if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
break;
but that plain store carries no barrier or RCU synchronization, so an
interrupt already inside mana_gd_process_eqe() on another CPU can still
pass the check, and the EQ-callback path has no such guard at all:
mana_hwc_init_event_handler() dereferences hwc->cq (for example
GDMA_EQE_HWC_INIT_EQ_ID_DB writing hwc->cq->gdma_eq->id) after it has been
kfree()d, and hwc->cq is not set to NULL here.
The window is also reachable when mana_smc_teardown_hwc() fails, since
mana_hwc_destroy_channel() only logs "Failed to tear down HWC" and
continues while the device may still be generating HWC EQEs and CQEs.
Would destroying and deregistering the EQ first, and freeing comp_buf and
the CQ last, close this?
[ ... ]
> @@ -965,43 +1101,40 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> ctx->output_buf = NULL;
> err = ctx->error;
> status = ctx->status_code;
> + if (err == -EINPROGRESS) {
> + /* Publish abandonment with buffer withdrawal so a late
> + * response can reclaim the slot. Keep its reference.
> + */
> + ctx->responded = true;
> + abandoned = true;
> + }
> spin_unlock_irqrestore(&ctx->lock, flags);
[Severity: Low]
This hunk, together with the resp_pending handling in
mana_hwc_handle_resp(), fixes a real misassociation on existing kernels:
the baseline timeout path did
out:
mana_hwc_put_msg_index(hwc, msg_id);
while the hardware response was still outstanding, and
mana_hwc_handle_resp() dispatches purely on msg_id, so the next request
could take the same slot and be completed with the previous command's
response and buffer contents.
Should this carry a Fixes: tag and a description of the observable failure?
As posted the fix is bundled with unrelated behavioural changes (admission
timeout, sender draining, SQ posting serialization), so stable maintainers
have nothing to select on.
[ ... ]
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 308950f9b54b0..571a533e62e64 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -468,6 +468,15 @@ struct gdma_context {
> /* Hardware communication channel (HWC) */
> struct gdma_dev hwc;
>
> + /* Sender drain; the final wakeup runs under hwc_lock. */
> + wait_queue_head_t hwc_drain_waitq;
> +
> + /* Protects HWC publication, sender references, and short accesses in
> + * mana_need_log()/mana_serv_reset(). Setup and DMA-region readers
> + * still require lifecycle ordering. Not all timeout writers use it.
> + */
> + spinlock_t hwc_lock;
[Severity: Medium]
Does taking hwc_lock around the hwc_timeout accesses in mana_need_log() and
mana_serv_reset() provide any mutual exclusion, given that the remaining
writers and readers of that field do not take it?
Writers without the lock:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_handler() {
case HWC_DATA_CFG_HWC_TIMEOUT:
hwc->hwc_timeout = val;
}
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
if (hwc->hwc_timeout > 1)
hwc->hwc_timeout = 1;
}
and mana_gd_verify_vf_version() passes &hwc->hwc_timeout to
mana_gd_query_hwc_timeout(), which writes through it unlocked.
Readers without the lock include wait_ms = hwc->hwc_timeout and the new
admission budget:
if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
The first of those writers runs in hard-IRQ context, so
spin_lock_irqsave() in mana_need_log() only masks the local CPU and does
not exclude the same store on another CPU.
There is also an intent problem now that the field is a blocking budget:
the value is sampled before sleeping, so mana_serv_reset() setting
hwc->hwc_timeout = 0;
under hwc_lock, specifically so that nothing waits, cannot shorten a wait
already in progress, and a reconfiguration EQE can restore a long timeout
in the middle of a reset cycle.
Would a single IRQ-safe lock for the field, or an explicit
READ_ONCE()/WRITE_ONCE() policy with defined update precedence, be a better
fit than the current "Not all timeout writers use it"?
>
> /* Azure network adapter */
> struct gdma_dev mana;
>
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path
2026-09-08 3:51 [PATCH net-next v5 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
` (2 preceding siblings ...)
2026-09-08 3:51 ` [PATCH net-next v5 3/4] net: mana: support concurrent HWC requests Long Li
@ 2026-09-08 3:51 ` Long Li
2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
3 siblings, 2 replies; 13+ messages in thread
From: Long Li @ 2026-09-08 3:51 UTC (permalink / raw)
To: Long Li, Long Li, Konstantin Taranov, Jakub Kicinski,
David S . Miller, Paolo Abeni, Eric Dumazet, Andrew Lunn,
Jason Gunthorpe, Leon Romanovsky, Haiyang Zhang,
K . Y . Srinivasan, Wei Liu, Dexuan Cui, shradhagupta,
Simon Horman, ernis, stephen, shirazsaleem
Cc: netdev, linux-rdma, linux-hyperv, linux-kernel
Rebuild depth-one HWC queues at the advertised maximum, capped at 128.
Preserve the 24-bit depth in u32, bound allocations and round message
buffers to a power of two. Advertise support in the driver-version request.
Keep bootstrap queues for incompatible initial dimensions. A failed
teardown or rebuild instead retries teardown of any submitted queues
before restoring depth one; never test queues after an indeterminate
destroy result. Abort creation if that teardown also fails. Reject
incompatible message sizes after bootstrap fallback, and guard CQ
unpublishing when a failed establish left no table.
Reset dimensions and routing IDs before each establish. The missing-
doorbell guard stops progression to the channel test on every attempt,
but does not cover BAR bounds or earlier IRQ rearming.
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v5 (v4 -> v5):
- Handle CQ cleanup when establishment failed before allocating cq_table.
- Route initial destroy failures through confirmed teardown and fresh
bootstrap reconstruction, rather than testing possibly destroyed queues.
- Share the teardown gate with failed larger-depth establishment.
- Correct dimension-validation and doorbell-guard scope descriptions;
shorten comments without broadening validation policy.
Changes in v4 (standalone net-next rework after the v3 split):
- Rework former patch 7/7 as patch 4/4 without the net fixes as prerequisites.
- Cap depth at 128 and round message-buffer allocations to a power of two.
- Require bootstrap message sizes before increasing depth, and validate
the rebuilt channel's report against its allocations.
- Reset dimensions/routing IDs on each establish and reject a missing
doorbell after the handshake.
- Retry teardown before restoring depth one after failed re-establishment.
- Retain u32 storage of the 24-bit depth and allocation-overflow checks.
Changes in v3 (historical net fixes-only posting):
- Defer the dynamic-depth feature; it was not included in the net series.
Changes in v2 (v1 -> v2):
- Retain dynamic-depth patch 7/7 on the revised concurrency prerequisites;
rebase the series onto net-next.
v1:
- Introduce depth-one bootstrap, rebuild at the reported depth, capability
advertisement, allocation checks and bootstrap fallback in patch 7/7.
.../net/ethernet/microsoft/mana/gdma_main.c | 8 +-
.../net/ethernet/microsoft/mana/hw_channel.c | 197 +++++++++++++++++-
include/net/mana/gdma.h | 4 +
include/net/mana/hw_channel.h | 9 +-
4 files changed, 209 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index eb88bae2b14d86de33e79eb597a076a7d6e54436..78424e1d884f18e6b216d627fc80e77630b0b350 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -1260,15 +1260,17 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
static void mana_gd_destroy_cq(struct gdma_context *gc,
struct gdma_queue *queue)
{
+ struct gdma_queue **cq_table = READ_ONCE(gc->cq_table);
u32 id = queue->id;
- if (id >= gc->max_num_cqs)
+ /* HWC re-establishment can fail before allocating the CQ table. */
+ if (!cq_table || id >= gc->max_num_cqs)
return;
- if (!gc->cq_table[id])
+ if (!cq_table[id])
return;
- gc->cq_table[id] = NULL;
+ cq_table[id] = NULL;
}
int mana_gd_create_hwc_queue(struct gdma_dev *gd,
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index a4f7346d285f740c40f4f63c20348e30531f1435..750ee8f72999489a42b2bf6e8561480e6ce07133 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -216,7 +216,12 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
break;
case HWC_INIT_DATA_QUEUE_DEPTH:
- hwc->hwc_init_q_depth_max = (u16)val;
+ /* HWC_INIT_DATA_QUEUE_DEPTH is a 24-bit field. Keep
+ * the full device-reported value here; it is clamped
+ * and validated in mana_hwc_create_channel() rather
+ * than silently truncated to u16.
+ */
+ hwc->hwc_init_q_depth_max = val;
break;
case HWC_INIT_DATA_MAX_REQUEST:
@@ -546,7 +551,11 @@ static int mana_hwc_alloc_dma_buf(struct hw_channel_context *hwc, u16 q_depth,
dma_buf->num_reqs = q_depth;
- buf_size = MANA_PAGE_ALIGN(q_depth * max_msg_size);
+ /* mana_gd_alloc_memory() only accepts a power-of-two length, as
+ * already assumed for the EQ and CQ rings above. The slots are
+ * carved from the head of the buffer, so any tail is unused.
+ */
+ buf_size = roundup_pow_of_two(MANA_PAGE_ALIGN(q_depth * max_msg_size));
gmi = &dma_buf->mem_info;
err = mana_gd_alloc_memory(gc, buf_size, gmi, false);
@@ -754,7 +763,7 @@ static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16 q_depth,
return err;
}
-static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
+static int mana_hwc_establish_channel(struct gdma_context *gc, u32 *q_depth,
u32 *max_req_msg_size,
u32 *max_resp_msg_size)
{
@@ -771,6 +780,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
struct gdma_queue *cq = hwc->cq->gdma_cq;
int err;
+ /* Do not reuse dimensions or routing IDs from a previous establish. */
+ hwc->hwc_init_q_depth_max = 0;
+ hwc->hwc_init_max_req_msg_size = 0;
+ hwc->hwc_init_max_resp_msg_size = 0;
+ gc->hwc.doorbell = INVALID_DOORBELL;
+ gc->hwc.pdid = INVALID_PDID;
+ hwc->pf_dest_vrq_id = 0;
+ hwc->pf_dest_vrcq_id = 0;
+
init_completion(&hwc->hwc_init_eqe_comp);
err = mana_smc_setup_hwc(&gc->shm_channel, false,
@@ -789,6 +807,14 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
+ /* Reject a missing doorbell before the channel test. This neither
+ * validates its BAR range nor protects earlier IRQ rearming.
+ */
+ if (gc->hwc.doorbell == INVALID_DOORBELL) {
+ dev_err(hwc->dev, "HWC: no doorbell in init data\n");
+ return -EPROTO;
+ }
+
/* Both were set in mana_hwc_init_event_handler(). */
if (WARN_ON(cq->id >= gc->max_num_cqs))
return -EPROTO;
@@ -807,6 +833,12 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
{
int err;
+ /* CQ depth is q_depth * 2 (SQ + RQ) passed as u16 to create_cq.
+ * Cap to prevent u16 truncation.
+ */
+ if (q_depth > U16_MAX / 2)
+ q_depth = U16_MAX / 2;
+
err = mana_hwc_init_inflight_msg(hwc, q_depth);
if (err)
return err;
@@ -846,13 +878,44 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
return err;
}
+/* Bring-up only: requires no senders or concurrent lifecycle operations.
+ * This helper does not unpublish the HWC or drain senders.
+ */
+static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
+{
+ struct gdma_context *gc = hwc->gdma_dev->gdma_context;
+
+ /* The CQ helper deregisters the HWC EQ before returning. */
+ if (hwc->cq) {
+ mana_hwc_destroy_cq(gc, hwc->cq);
+ hwc->cq = NULL;
+ }
+
+ kfree(hwc->caller_ctx);
+ hwc->caller_ctx = NULL;
+
+ if (hwc->txq) {
+ mana_hwc_destroy_wq(hwc, hwc->txq);
+ hwc->txq = NULL;
+ }
+
+ if (hwc->rxq) {
+ mana_hwc_destroy_wq(hwc, hwc->rxq);
+ hwc->rxq = NULL;
+ }
+
+ mana_gd_free_res_map(&hwc->inflight_msg_res);
+ hwc->num_inflight_msg = 0;
+}
+
int mana_hwc_create_channel(struct gdma_context *gc)
{
u32 max_req_msg_size, max_resp_msg_size;
struct gdma_dev *gd = &gc->hwc;
struct hw_channel_context *hwc;
+ struct gdma_queue **old_cq_table;
unsigned long flags;
- u16 q_depth_max;
+ u32 q_depth_max;
int err;
hwc = kzalloc_obj(*hwc);
@@ -896,8 +959,132 @@ int mana_hwc_create_channel(struct gdma_context *gc)
goto out;
}
+ if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
+ /* Bound DMA allocations before using the 24-bit depth. */
+ if (q_depth_max > HW_CHANNEL_MAX_QUEUE_DEPTH)
+ q_depth_max = HW_CHANNEL_MAX_QUEUE_DEPTH;
+
+ /* Keep bootstrap message sizes for mandatory commands.
+ * Incompatible reports skip rebuilding, not channel creation.
+ */
+ if (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
+ max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE ||
+ (u64)q_depth_max * max_req_msg_size >
+ U32_MAX - MANA_PAGE_SIZE ||
+ (u64)q_depth_max * max_resp_msg_size >
+ U32_MAX - MANA_PAGE_SIZE) {
+ dev_err(hwc->dev,
+ "HWC: invalid dims q=%u req=%u resp=%u\n",
+ q_depth_max, max_req_msg_size,
+ max_resp_msg_size);
+ q_depth_max = HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH;
+ goto skip_reinit;
+ }
+
+ err = mana_smc_teardown_hwc(&gc->shm_channel, false);
+ if (err) {
+ dev_err(hwc->dev,
+ "Failed to teardown HWC for reinit: %d\n",
+ err);
+ goto reinit_fallback;
+ }
+
+ hwc->setup_active = false;
+
+ /* Unpublish the CQ and drain its EQ before freeing the table. */
+ mana_hwc_destroy_queues(hwc);
+
+ old_cq_table = gc->cq_table;
+ gc->cq_table = NULL;
+ gc->max_num_cqs = 0;
+ synchronize_rcu();
+ vfree(old_cq_table);
+
+ err = mana_hwc_init_queues(hwc, q_depth_max,
+ max_req_msg_size,
+ max_resp_msg_size);
+ if (err) {
+ dev_err(hwc->dev, "Failed to reinit HWC: %d\n", err);
+ goto reinit_fallback;
+ }
+
+ err = mana_hwc_establish_channel(gc, &q_depth_max,
+ &max_req_msg_size,
+ &max_resp_msg_size);
+ if (!err &&
+ (q_depth_max < hwc->num_inflight_msg ||
+ max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
+ max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE)) {
+ /* The rebuilt channel must support the allocated depth
+ * and message sizes.
+ */
+ dev_err(hwc->dev,
+ "HWC: rebuilt q=%u req=%u resp=%u, built for %u/%u/%u\n",
+ q_depth_max, max_req_msg_size,
+ max_resp_msg_size, hwc->num_inflight_msg,
+ HW_CHANNEL_MAX_REQUEST_SIZE,
+ HW_CHANNEL_MAX_RESPONSE_SIZE);
+ err = -EPROTO;
+ }
+ if (err) {
+ dev_err(hwc->dev, "Failed to re-establish HWC: %d\n",
+ err);
+ goto reinit_fallback;
+ }
+ }
+
+ goto skip_reinit;
+
+reinit_fallback:
+ /* A failed handshake leaves queue ownership uncertain. */
+ if (hwc->setup_active) {
+ if (mana_smc_teardown_hwc(&gc->shm_channel, false)) {
+ dev_err(hwc->dev,
+ "Failed to tear down HWC before bootstrap fallback\n");
+ goto out;
+ }
+ hwc->setup_active = false;
+ }
+
+ /* The failed establish may not have allocated cq_table. */
+ dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap depth\n");
+
+ mana_hwc_destroy_queues(hwc);
+
+ old_cq_table = gc->cq_table;
+ gc->cq_table = NULL;
+ gc->max_num_cqs = 0;
+ synchronize_rcu();
+ vfree(old_cq_table);
+
+ err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
+ HW_CHANNEL_MAX_REQUEST_SIZE,
+ HW_CHANNEL_MAX_RESPONSE_SIZE);
+ if (err) {
+ dev_err(hwc->dev, "Failed to restore bootstrap HWC: %d\n", err);
+ goto out;
+ }
+
+ err = mana_hwc_establish_channel(gc, &q_depth_max, &max_req_msg_size,
+ &max_resp_msg_size);
+ if (!err &&
+ (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
+ max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE)) {
+ /* The restored channel must report the allocated message sizes. */
+ dev_err(hwc->dev, "HWC: bootstrap reports req=%u resp=%u\n",
+ max_req_msg_size, max_resp_msg_size);
+ err = -EPROTO;
+ }
+ if (err) {
+ dev_err(hwc->dev, "Failed to re-establish bootstrap HWC: %d\n",
+ err);
+ goto out;
+ }
+
+skip_reinit:
+
err = mana_hwc_test_channel(gc->hwc.driver_data,
- HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
+ hwc->num_inflight_msg,
max_req_msg_size, max_resp_msg_size);
if (err) {
dev_err(hwc->dev, "Failed to test HWC: %d\n", err);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 571a533e62e64790f9000d42ab0e833fe36ccff6..22dd7c9cecc8a74ed08075bee2b6db6f39ea9cc0 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -692,6 +692,9 @@ enum {
/* Driver supports dynamic interrupt moderation - DIM */
#define GDMA_DRV_CAP_FLAG_1_DYN_INTERRUPT_MODERATION BIT(28)
+/* Driver supports dynamic queue depth for HWC */
+#define GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH BIT(29)
+
/* Driver supports non-contiguous queue buffers */
#define GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS BIT(30)
@@ -710,6 +713,7 @@ enum {
GDMA_DRV_CAP_FLAG_1_PROBE_RECOVERY | \
GDMA_DRV_CAP_FLAG_1_HANDLE_STALL_SQ_RECOVERY | \
GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECOVERY | \
+ GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH | \
GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT | \
GDMA_DRV_CAP_FLAG_1_DYN_INTERRUPT_MODERATION | \
GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS)
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index fba27d8620a388a41ae7ddd3bf2b4792f7beec3d..dfd92336b2b4dc0e9943732f9080887683ebfbbc 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -11,6 +11,13 @@
#define HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH 1
+/* Upper bound on the device-reported HWC queue depth. The HWC is a
+ * control channel, so a modest depth already keeps enough management
+ * commands in flight, while the depth directly scales two coherent DMA
+ * buffers of q_depth * max_msg_size bytes each.
+ */
+#define HW_CHANNEL_MAX_QUEUE_DEPTH 128
+
#define HWC_INIT_DATA_CQID 1
#define HWC_INIT_DATA_RQID 2
#define HWC_INIT_DATA_SQID 3
@@ -205,7 +212,7 @@ struct hw_channel_context {
u32 max_req_msg_size;
- u16 hwc_init_q_depth_max;
+ u32 hwc_init_q_depth_max;
u32 hwc_init_max_req_msg_size;
u32 hwc_init_max_resp_msg_size;
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path
2026-09-08 3:51 ` [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
@ 2026-09-09 3:52 ` sashiko-bot
2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-09 3:52 UTC (permalink / raw)
To: Long Li; +Cc: linux-rdma, linux-hyperv
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Critical] Capping the hardware-reported HWC queue depth without negotiating it with the host causes a fatal ring buffer size mismatch, leading to out-of-bounds memory accesses.
Pre-existing issues:
- [High] CQ queue memory is freed without waiting for an RCU grace period, risking use-after-free in the IRQ handler.
- [High] Use-After-Free during channel teardown due to incorrect queue destruction order.
--
commit e9ca1d1d51e325f4557f02fd7fcfac8f36e41ace
Author: Long Li <longli@microsoft.com>
net: mana: add dynamic HWC queue depth with reinit path
Rebuild depth-one HWC queues at the advertised maximum, capped at 128.
Preserve the 24-bit depth in u32, bound allocations and round message
buffers to a power of two. Advertise support in the driver-version request.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index eb88bae2b14d8..78424e1d884f1 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1260,15 +1260,17 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
> static void mana_gd_destroy_cq(struct gdma_context *gc,
> struct gdma_queue *queue)
> {
> + struct gdma_queue **cq_table = READ_ONCE(gc->cq_table);
> u32 id = queue->id;
>
> - if (id >= gc->max_num_cqs)
> + /* HWC re-establishment can fail before allocating the CQ table. */
> + if (!cq_table || id >= gc->max_num_cqs)
> return;
>
> - if (!gc->cq_table[id])
> + if (!cq_table[id])
> return;
>
> - gc->cq_table[id] = NULL;
> + cq_table[id] = NULL;
[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a
use-after-free in the EQ interrupt handler?
The CQ memory is freed by the caller (mana_gd_destroy_queue()) immediately
after mana_gd_destroy_cq() unpublishes it from the table, without waiting for
an RCU grace period.
Concurrently, the EQ interrupt processor (mana_gd_process_eqe()) runs under
rcu_read_lock(), reads the CQ pointer from the table, and dereferences it. If
the CQ is freed during this window, the interrupt handler will access freed
memory. Should this wait for an RCU grace period before freeing the queue?
> }
>
> int mana_gd_create_hwc_queue(struct gdma_dev *gd,
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index a4f7346d285f7..750ee8f729994 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -846,13 +878,44 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
> return err;
> }
>
> +/* Bring-up only: requires no senders or concurrent lifecycle operations.
> + * This helper does not unpublish the HWC or drain senders.
> + */
> +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
> +{
> + struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> +
> + /* The CQ helper deregisters the HWC EQ before returning. */
> + if (hwc->cq) {
> + mana_hwc_destroy_cq(gc, hwc->cq);
[Severity: High]
This is a pre-existing issue, but does this queue destruction order cause a
use-after-free during teardown?
mana_hwc_destroy_cq() frees the CQ object before it destroys the EQ. The EQ
remains active and can still receive events from the hardware. If a teardown
fails or times out (which is actively exercised by the new reinit_fallback
logic) and an initialization event arrives after the CQ is freed but before
the EQ is disabled, the EQ interrupt handler will dereference the freed CQ
when processing HWC_INIT_DATA_CQID.
> + hwc->cq = NULL;
> + }
[ ... ]
> @@ -896,8 +959,132 @@ int mana_hwc_create_channel(struct gdma_context *gc)
> goto out;
> }
>
> + if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
> + /* Bound DMA allocations before using the 24-bit depth. */
> + if (q_depth_max > HW_CHANNEL_MAX_QUEUE_DEPTH)
> + q_depth_max = HW_CHANNEL_MAX_QUEUE_DEPTH;
[Severity: Critical]
Does capping the hardware-reported HWC queue depth without negotiating it
with the host cause a ring buffer size mismatch?
The driver limits the queue depth to HW_CHANNEL_MAX_QUEUE_DEPTH (128) and
allocates DMA rings based on this limit. However, the mana_smc_setup_hwc()
protocol lacks a queue size field, meaning this restriction isn't
communicated to the host hardware.
If the hardware advertises a larger depth (e.g., 256), will it write up to
256 entries into the rings and exceed the 128 allocated entries, leading to
an out-of-bounds memory write?
> +
> + /* Keep bootstrap message sizes for mandatory commands.
> + * Incompatible reports skip rebuilding, not channel creation.
> + */
> + if (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
[ ... ]
> + err = mana_hwc_establish_channel(gc, &q_depth_max,
> + &max_req_msg_size,
> + &max_resp_msg_size);
> + if (!err &&
> + (q_depth_max < hwc->num_inflight_msg ||
[Severity: Critical]
Does this validation check incorrectly allow the out-of-bounds access?
By verifying q_depth_max < hwc->num_inflight_msg, it permits the hardware
depth (q_depth_max) to be larger than the driver's allocated depth
(hwc->num_inflight_msg). Should this reject the configuration if the hardware
depth exceeds the allocated depth?
> + max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
> + max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE)) {
> + /* The rebuilt channel must support the allocated depth
> + * and message sizes.
> + */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908035201.402424-1-longli@microsoft.com?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path
2026-09-08 3:51 ` [PATCH net-next v5 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
2026-09-09 3:52 ` sashiko-bot
@ 2026-09-11 6:53 ` netdev-bot+sashiko
1 sibling, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 6:53 UTC (permalink / raw)
To: longli
Cc: longli, kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev,
jgg, leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms,
ernis, stephen, shirazsaleem, netdev, linux-rdma, linux-hyperv,
linux-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 3 (1) · Medium: 1 (1) · Low: 0
- [High] The new re-init and bootstrap-fallback blocks in
mana_hwc_create_channel() unpublish the CQ table in the wrong order:…
- [High] The new reinit_fallback block treats a failed DESTROY_HWC as
fatal (`if (mana_smc_teardown_hwc(&gc->shm_channel, false)) {…
- [High] mana_hwc_establish_channel() now sets `gc->hwc.doorbell =
INVALID_DOORBELL;` (UINT_MAX) at the top of every establish, after…
- [Medium] The commit message states "Advertise support in the driver-
version request", but GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH cannot…
Pre-existing issues:
- [High] The new guard in mana_hwc_establish_channel() validates the HWC
doorbell only against the INVALID_DOORBELL sentinel (UINT_MAX); it…
- [Medium] mana_hwc_destroy_cq() frees resources that the still-live HWC
EQ interrupt handler can reach before the EQ is deregistered:…
--
net: mana: add dynamic HWC queue depth with reinit path
This rebuilds the depth-one HWC bootstrap queues at the device-advertised
maximum depth, capped at 128, and keeps the 24-bit reported depth in a u32
with bounded DMA allocations. It adds a bootstrap fallback path that tears
down and rebuilds at depth one when the larger-depth teardown, rebuild or
re-establishment fails.
No Link: tags in the commit header.
A few questions below about the new reinit and fallback paths.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index eb88bae2b14d8..78424e1d884f1 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1260,15 +1260,17 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
> static void mana_gd_destroy_cq(struct gdma_context *gc,
> struct gdma_queue *queue)
> {
> + struct gdma_queue **cq_table = READ_ONCE(gc->cq_table);
> u32 id = queue->id;
>
> - if (id >= gc->max_num_cqs)
> + /* HWC re-establishment can fail before allocating the CQ table. */
> + if (!cq_table || id >= gc->max_num_cqs)
> return;
>
> - if (!gc->cq_table[id])
> + if (!cq_table[id])
> return;
>
> - gc->cq_table[id] = NULL;
> + cq_table[id] = NULL;
> }
[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index a4f7346d285f7..750ee8f729994 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -771,6 +780,15 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
> struct gdma_queue *cq = hwc->cq->gdma_cq;
> int err;
>
> + /* Do not reuse dimensions or routing IDs from a previous establish. */
> + hwc->hwc_init_q_depth_max = 0;
> + hwc->hwc_init_max_req_msg_size = 0;
> + hwc->hwc_init_max_resp_msg_size = 0;
> + gc->hwc.doorbell = INVALID_DOORBELL;
> + gc->hwc.pdid = INVALID_PDID;
> + hwc->pf_dest_vrq_id = 0;
> + hwc->pf_dest_vrcq_id = 0;
> +
[Severity: High]
Is it safe to set gc->hwc.doorbell to INVALID_DOORBELL here while the HWC
EQ is already registered on the shared msix 0 interrupt?
mana_hwc_init_queues() has already run mana_gd_create_eq() ->
mana_gd_register_irq() -> list_add_rcu(&queue->entry, &gic->eq_list), so
mana_gd_process_eq_events() can run at any point after
mana_smc_setup_hwc() writes the shared-memory request. At the end of every
batch it arms unconditionally:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eq_events() {
...
mana_gd_ring_doorbell(gc, eq->gdma_dev->doorbell, eq->type, eq->id,
head, SET_ARM_BIT);
...
}
and mana_gd_ring_doorbell() forms the MMIO address with no validity check:
void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index;
...
writeq(e.as_uint64, addr);
Only GDMA_EQE_HWC_INIT_EQ_ID_DB repopulates gd->doorbell in
mana_hwc_init_event_handler(). If the first delivered batch does not start
with that event, does the writeq() run with db_index == 0xFFFFFFFF, i.e.
roughly 17 TiB past the ioremapped BAR0 window?
The field is also written from hard IRQ context (gd->doorbell =
eq_db.doorbell;) while this store is a plain process-context store with no
WRITE_ONCE() or barrier. Since the reinit path calls
mana_hwc_establish_channel() two or three times per probe, does this
multiply the window rather than close it?
> init_completion(&hwc->hwc_init_eqe_comp);
>
> err = mana_smc_setup_hwc(&gc->shm_channel, false,
> @@ -789,6 +807,14 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
> *max_req_msg_size = hwc->hwc_init_max_req_msg_size;
> *max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>
> + /* Reject a missing doorbell before the channel test. This neither
> + * validates its BAR range nor protects earlier IRQ rearming.
> + */
> + if (gc->hwc.doorbell == INVALID_DOORBELL) {
> + dev_err(hwc->dev, "HWC: no doorbell in init data\n");
> + return -EPROTO;
> + }
> +
[Severity: High]
This isn't a bug introduced by this patch, but since a doorbell sanity
check is being added right here, could it also cover the BAR range?
The value comes from a 16-bit device-controlled field and is stored with no
validation:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_handler() {
...
eq_db.as_uint32 = event->details[0];
hwc->cq->gdma_eq->id = eq_db.eq_id;
gd->doorbell = eq_db.doorbell;
...
}
It is then consumed unchecked by mana_gd_ring_doorbell() from the EQ rearm,
from mana_gd_ring_cq() on the HWC CQ and from mana_gd_wq_ring_doorbell() on
the HWC SQ/RQ. The driver already has the corresponding check for the
PF-reported db_id:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_device() {
...
if (gc->db_page_off + gc->db_page_size * ((u64)resp.db_id + 1) > gc->bar0_size) {
dev_err(gc->dev, "Doorbell ID %u out of range\n", resp.db_id);
return -EPROTO;
}
...
}
With a 4 KiB doorbell page, a reported index of 0x8000 gives a 128 MiB
offset, well outside a typical VF BAR0. Can the writeq() then land outside
the ioremapped region at a device-chosen offset? The commit message notes
the gap ("does not cover BAR bounds"), so was leaving it open intentional?
> /* Both were set in mana_hwc_init_event_handler(). */
> if (WARN_ON(cq->id >= gc->max_num_cqs))
> return -EPROTO;
[ ... ]
> @@ -846,13 +878,44 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
> return err;
> }
>
> +/* Bring-up only: requires no senders or concurrent lifecycle operations.
> + * This helper does not unpublish the HWC or drain senders.
> + */
> +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
> +{
> + struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> +
> + /* The CQ helper deregisters the HWC EQ before returning. */
> + if (hwc->cq) {
> + mana_hwc_destroy_cq(gc, hwc->cq);
> + hwc->cq = NULL;
> + }
[Severity: Medium]
This is a pre-existing ordering issue rather than something this patch
introduces, but the comment says the CQ helper deregisters the EQ before
returning, and that only happens as the last step:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_cq() {
kfree(hwc_cq->comp_buf);
if (hwc_cq->gdma_cq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
if (hwc_cq->gdma_eq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
...
}
The list_del_rcu()/synchronize_rcu() pair that actually drains in-flight EQ
users lives in mana_gd_deregister_irq(), reached only from the final
gdma_eq destroy. During the first two frees the EQ is still on
gic->eq_list.
Can an interrupt in that window reach mana_hwc_comp_event() and use the
already freed comp_buf?
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_comp_event() {
...
completions = hwc_cq->comp_buf;
comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
...
}
An HWC init EQE in the same window would instead reach
mana_hwc_init_event_handler(), which writes hwc->cq->gdma_eq->id and
hwc->cq->gdma_cq->id while those objects are being freed, since hwc->cq is
only cleared after mana_hwc_destroy_queues() returns. Would destroying the
EQ first, before comp_buf and the gdma_cq, close this?
> +
> + kfree(hwc->caller_ctx);
> + hwc->caller_ctx = NULL;
> +
> + if (hwc->txq) {
> + mana_hwc_destroy_wq(hwc, hwc->txq);
> + hwc->txq = NULL;
> + }
> +
> + if (hwc->rxq) {
> + mana_hwc_destroy_wq(hwc, hwc->rxq);
> + hwc->rxq = NULL;
> + }
> +
> + mana_gd_free_res_map(&hwc->inflight_msg_res);
> + hwc->num_inflight_msg = 0;
> +}
> +
[ ... ]
> @@ -896,8 +959,132 @@ int mana_hwc_create_channel(struct gdma_context *gc)
> goto out;
> }
>
> + if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
[ ... ]
> + err = mana_smc_teardown_hwc(&gc->shm_channel, false);
> + if (err) {
> + dev_err(hwc->dev,
> + "Failed to teardown HWC for reinit: %d\n",
> + err);
> + goto reinit_fallback;
> + }
> +
> + hwc->setup_active = false;
> +
> + /* Unpublish the CQ and drain its EQ before freeing the table. */
> + mana_hwc_destroy_queues(hwc);
> +
> + old_cq_table = gc->cq_table;
> + gc->cq_table = NULL;
> + gc->max_num_cqs = 0;
> + synchronize_rcu();
> + vfree(old_cq_table);
[Severity: High]
Is the store order here inverted with respect to what the readers check?
The pre-existing teardown in mana_hwc_destroy_channel() clears the bound
first and only drops the pointer at the very end (gc->max_num_cqs = 0; ...
vfree(gc->cq_table); gc->cq_table = NULL;). Here the pointer goes away
first, leaving a state where the bound check passes but the pointer is
NULL.
The surviving hard IRQ consumer validates only the bound:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eqe() {
...
cq_id = eqe->details[0] & 0xFFFFFF;
if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
break;
cq = gc->cq_table[cq_id];
...
}
That is exactly the case the patch guards against in mana_gd_destroy_cq()
with READ_ONCE() plus a NULL check, but mana_gd_process_eqe() has neither.
Beyond the adjacent-store window, is there a much longer one? The
subsequent mana_hwc_init_queues() re-registers the HWC EQ on msix 0 before
mana_hwc_establish_channel() runs, and the hard IRQ init handler sets
gd->gdma_context->max_num_cqs = val from HWC_INIT_DATA_MAX_NUM_CQS while
gc->cq_table is still NULL. The vcalloc() that repopulates it only happens
after the up-to-60 s wait_for_completion_timeout() returns. Would a
GDMA_EQE_COMPLETION delivered in that window pass the bound check and
dereference NULL plus cq_id * 8, then call through the loaded value as
cq->cq.callback?
Also, since the reclaim now relies on synchronize_rcu(), should these be
WRITE_ONCE()/rcu_assign_pointer() stores rather than plain ones?
> +
> + err = mana_hwc_init_queues(hwc, q_depth_max,
> + max_req_msg_size,
> + max_resp_msg_size);
> + if (err) {
> + dev_err(hwc->dev, "Failed to reinit HWC: %d\n", err);
> + goto reinit_fallback;
> + }
[ ... ]
> + }
> +
> + goto skip_reinit;
> +
> +reinit_fallback:
> + /* A failed handshake leaves queue ownership uncertain. */
> + if (hwc->setup_active) {
> + if (mana_smc_teardown_hwc(&gc->shm_channel, false)) {
> + dev_err(hwc->dev,
> + "Failed to tear down HWC before bootstrap fallback\n");
> + goto out;
> + }
> + hwc->setup_active = false;
> + }
[Severity: High]
Does this abort actually avoid freeing memory the device may still own?
The goto out lands in mana_hwc_destroy_channel(), which retries the
teardown once and then continues regardless of the result:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
...
if (hwc->setup_active) {
if (!mana_smc_teardown_hwc(&gc->shm_channel, false))
hwc->setup_active = false;
else
dev_err(hwc->dev, "Failed to tear down HWC\n");
}
gc->max_num_cqs = 0;
/* Deregister the HWC EQ before freeing the work queues. */
if (hwc->cq)
mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
...
}
So a third failure is logged and then mana_hwc_destroy_cq() and
mana_hwc_destroy_wq() run mana_gd_destroy_queue() ->
mana_gd_free_memory() -> dma_free_coherent(), plus vfree(gc->cq_table) and
kfree(hwc), with hwc->setup_active still true.
mana_smc_teardown_hwc() states the requirement explicitly:
drivers/net/ethernet/microsoft/mana/shm_channel.c:mana_smc_teardown_hwc() {
...
/* Read shmem response (polling for VF possession) and validate.
* For teardown, waiting for response is required to ensure hardware
* invalidates MST entries before software frees memory.
*/
...
}
Can the coherent EQ/CQ/SQ/RQ rings and HWC message buffers then be returned
to the page allocator while the hardware MST entries still map them, and
with hwc freed so no later safe teardown is possible? The ignore-the-failure
behaviour in mana_hwc_destroy_channel() predates this series, but this abort
path is newly reachable, and the commit message says the intent is to
"never test queues after an indeterminate destroy result".
[ ... ]
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 571a533e62e64..22dd7c9cecc8a 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -692,6 +692,9 @@ enum {
> /* Driver supports dynamic interrupt moderation - DIM */
> #define GDMA_DRV_CAP_FLAG_1_DYN_INTERRUPT_MODERATION BIT(28)
>
> +/* Driver supports dynamic queue depth for HWC */
> +#define GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH BIT(29)
> +
[Severity: Medium]
The commit message says "Advertise support in the driver-version request."
Can this flag gate the new behaviour at all?
GDMA_DRV_CAP_FLAGS1 is only transmitted by mana_gd_verify_vf_version(),
which runs after the HWC has been created:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_setup() {
...
err = mana_hwc_create_channel(gc);
if (err)
goto remove_irq;
err = mana_gd_verify_vf_version(pdev);
...
}
By then mana_hwc_create_channel() has already consumed
HWC_INIT_DATA_QUEUE_DEPTH and performed the DESTROY_HWC plus second
ESTABLISH_HWC cycle, and the verify request itself travels over the rebuilt
channel. So the rebuild in mana_hwc_create_channel() is taken purely from
the device-reported depth:
if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
Unlike other capabilities in this driver, there is no PF acknowledgment
check (GDMA_PF_CAP_FLAG_1_* in gc->pf_cap_flags1) before acting.
In the baseline, hwc_init_q_depth_max was stored but never acted on, since
mana_hwc_test_channel() was always called with
HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH. Does existing hardware that already
reports a depth above one now perform an extra shared-memory
teardown/establish per probe, each with mana_smc_poll_register() polling and
a 60 * HZ wait_for_completion_timeout()? And can probe now fail through the
reinit_fallback goto out paths where it previously succeeded at depth one?
Could the commit message describe the flag as informational only, and
mention the behaviour change on PF firmware that never saw the new bit?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com
^ permalink raw reply [flat|nested] 13+ messages in thread