* [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF
2026-09-01 20:00 [PATCH net-next v4 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
@ 2026-09-01 20:00 ` Long Li
2026-09-02 20:01 ` sashiko-bot
2026-09-01 20:00 ` [PATCH net-next v4 2/4] net: mana: give each HWC message slot its own completion state Long Li
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Long Li @ 2026-09-01 20:00 UTC (permalink / raw)
To: 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
mana_hwc_destroy_channel() decides whether to tear the channel down by
looking at gc->max_num_cqs, which is only set when the device delivers an
HWC_INIT_DATA_MAX_NUM_CQS bootstrap event. That is a proxy for "the
device is using the queues", and it is a poor one: the event arrives well
after mana_smc_setup_hwc() has already handed the queue addresses to the
PF, so a setup that fails in between leaves no reliable signal.
Record the handover directly instead. mana_smc_setup_hwc() clears the
new setup_active flag on entry and sets it immediately before it writes
the ESTABLISH_HWC message, so the flag is exact in both directions: the
three exits before that write leave the device untouched and clear the
flag, while everything after it -- including a failed response -- leaves
it set, because the PF has the queue addresses either way.
No functional change is intended for the current single-establish flow.
This is preparation for the reinit path added later in this series, which
tears the channel down and rebuilds it at a larger queue depth and needs
to know whether a given attempt left the device holding the queues.
Signed-off-by: Long Li <longli@microsoft.com>
---
.../net/ethernet/microsoft/mana/hw_channel.c | 19 +++++++++++++------
.../net/ethernet/microsoft/mana/shm_channel.c | 11 ++++++++++-
include/net/mana/hw_channel.h | 10 ++++++++++
include/net/mana/shm_channel.h | 2 +-
4 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 263e7c4e2934186af037be4c80350a6e322b6771..75fdccdc8c48201b011a243fd0bd41b84c477076 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,20 @@ 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.
+ /* Tear down only if setup_hwc() handed the queues to the PF. Until
+ * then the device never saw them, so there is nothing to undo.
*/
- if (gc->max_num_cqs > 0) {
- mana_smc_teardown_hwc(&gc->shm_channel, false);
- gc->max_num_cqs = 0;
+ if (hwc->setup_active) {
+ /* Only a successful teardown invalidates the MST entries. If
+ * it fails the device may still be using the queues, so leave
+ * the flag set rather than record a clean teardown.
+ */
+ 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..1ec8e3661721dc8663975477b7ed09fcda72e2ab 100644
--- a/drivers/net/ethernet/microsoft/mana/shm_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/shm_channel.c
@@ -129,9 +129,15 @@ void mana_smc_init(struct shm_channel *sc, struct device *dev,
sc->base = base;
}
+/* Clear *submitted on entry and set it once the ESTABLISH_HWC message has
+ * been handed to the PF, i.e. once the device may start using the queues.
+ * The caller uses it to decide whether a failure still needs
+ * mana_smc_teardown_hwc(): the exits below leave the device untouched, so
+ * they must not require one.
+ */
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 +150,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 +237,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..237608b488e5ee773d7889d3c1d291ca92ebbeb9 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -199,6 +199,16 @@ struct hw_channel_context {
u32 pf_dest_vrcq_id;
u32 hwc_timeout;
+ /* True once mana_smc_setup_hwc() has handed the ESTABLISH_HWC message
+ * to the PF, so the device may DMA into the HWC buffers. That
+ * function clears it on entry and sets it at the handover, so only a
+ * failure before the handover leaves it false; a failure after it --
+ * including one reported by mana_hwc_establish_channel() -- leaves it
+ * set, which is what makes teardown attempt DESTROY_HWC. Cleared
+ * again once that teardown succeeds.
+ */
+ 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] 9+ messages in thread* Re: [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF
2026-09-01 20:00 ` [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF Long Li
@ 2026-09-02 20:01 ` sashiko-bot
2026-09-02 20:04 ` [EXTERNAL] " Long Li
0 siblings, 1 reply; 9+ messages in thread
From: sashiko-bot @ 2026-09-02 20:01 UTC (permalink / raw)
To: Long Li; +Cc: linux-hyperv
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] If `mana_smc_teardown_hwc()` fails, the driver proceeds to unconditionally destroy the hardware queues and free their backing DMA memory, which can lead to a DMA use-after-free by the device.
--
commit 0af150088ed8994a134d0a328a359e4cea4bbce5
Author: Long Li <longli@microsoft.com>
net: mana: track when the HWC has been handed to the PF
This commit introduces a new setup_active flag to reliably track whether the
hardware queues have been handed to the physical function, addressing a gap
where the previous queue depth check was insufficient. This ensures accurate
state tracking during teardown to prepare for a future reinitialization path.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 263e7c4e29341..75fdccdc8c482 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -815,13 +815,20 @@ 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.
> + /* Tear down only if setup_hwc() handed the queues to the PF. Until
> + * then the device never saw them, so there is nothing to undo.
> */
> - if (gc->max_num_cqs > 0) {
> - mana_smc_teardown_hwc(&gc->shm_channel, false);
> - gc->max_num_cqs = 0;
> + if (hwc->setup_active) {
> + /* Only a successful teardown invalidates the MST entries. If
> + * it fails the device may still be using the queues, so leave
> + * the flag set rather than record a clean teardown.
> + */
> + 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);
[Severity: High]
This is a pre-existing issue, but if mana_smc_teardown_hwc() fails, does the
driver proceed to unconditionally destroy the hardware queues and free their
backing DMA memory?
As the newly added comment acknowledges that "the device may still be using
the queues", calling mana_hwc_destroy_wq() unconditionally unmaps and frees
the DMA memory. If the physical function hardware is still active, could this
result in a DMA use-after-free leading to memory corruption or IOMMU faults?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901200018.3194525-1-longli@microsoft.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [EXTERNAL] Re: [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF
2026-09-02 20:01 ` sashiko-bot
@ 2026-09-02 20:04 ` Long Li
0 siblings, 0 replies; 9+ messages in thread
From: Long Li @ 2026-09-02 20:04 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev; +Cc: linux-hyperv@vger.kernel.org
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
>
> Pre-existing issues:
> - [High] If `mana_smc_teardown_hwc()` fails, the driver proceeds to
> unconditionally destroy the hardware queues and free their backing DMA
> memory, which can lead to a DMA use-after-free by the device.
> --
>
> commit 0af150088ed8994a134d0a328a359e4cea4bbce5
> Author: Long Li <longli@microsoft.com>
>
> net: mana: track when the HWC has been handed to the PF
>
> This commit introduces a new setup_active flag to reliably track whether the
> hardware queues have been handed to the physical function, addressing a
> gap where the previous queue depth check was insufficient. This ensures
> accurate state tracking during teardown to prepare for a future reinitialization
> path.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 263e7c4e29341..75fdccdc8c482 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> [ ... ]
> > @@ -815,13 +815,20 @@ 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.
> > + /* Tear down only if setup_hwc() handed the queues to the PF. Until
> > + * then the device never saw them, so there is nothing to undo.
> > */
> > - if (gc->max_num_cqs > 0) {
> > - mana_smc_teardown_hwc(&gc->shm_channel, false);
> > - gc->max_num_cqs = 0;
> > + if (hwc->setup_active) {
> > + /* Only a successful teardown invalidates the MST entries. If
> > + * it fails the device may still be using the queues, so leave
> > + * the flag set rather than record a clean teardown.
> > + */
> > + 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);
>
> [Severity: High]
> This is a pre-existing issue, but if mana_smc_teardown_hwc() fails, does the
> driver proceed to unconditionally destroy the hardware queues and free
> their backing DMA memory?
>
> As the newly added comment acknowledges that "the device may still be
> using the queues", calling mana_hwc_destroy_wq() unconditionally unmaps
> and frees the DMA memory. If the physical function hardware is still active,
> could this result in a DMA use-after-free leading to memory corruption or
> IOMMU faults?
>
This is a pre-existing issue. The patch set implements multi queue depth support and doesn't attempt to fix the existing issue.
If needed, I will submit a separate patch to fix the existing issue. The patch is not related to this patch series.
Long
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v4 2/4] net: mana: give each HWC message slot its own completion state
2026-09-01 20:00 [PATCH net-next v4 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
2026-09-01 20:00 ` [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF Long Li
@ 2026-09-01 20:00 ` Long Li
2026-09-01 20:00 ` [PATCH net-next v4 3/4] net: mana: support concurrent HWC requests Long Li
2026-09-01 20:00 ` [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
3 siblings, 0 replies; 9+ messages in thread
From: Long Li @ 2026-09-01 20:00 UTC (permalink / raw)
To: 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
An HWC message slot is currently owned jointly by the sender and the
response handler with nothing arbitrating between them: the sender
publishes ctx->output_buf, waits, and on timeout releases the slot, while
mana_hwc_handle_resp() writes through that pointer from the CQ interrupt.
At the bootstrap queue depth of one this is survivable because there is
never more than one command outstanding.
Give each slot the state it needs to be owned independently:
- a per-slot spinlock, so the sender's timeout path and the response
handler serialise on the slot rather than on the channel;
- a refcount holding one reference for the sender and one for the
response handler, with the last put releasing the bitmap slot, so
neither side can retire a slot the other is still using;
- a responded flag, set by whichever side gets there first, so a
second response for the same request is dropped rather than applied
twice.
ctx->output_buf becomes the sender's ownership marker: the handler
honours a response only while it is published and the slot has not
already been answered. ctx->error also changes from u32 to int, since it
carries a negative errno.
A response that arrives after its slot has already been released and
reused is still applied to the new owner; the slot index is the only
thing correlating a response to a request, so nothing here can tell the
two apart. The next patch stops the slot being released at all while a
response may still arrive.
No functional change is intended at the current queue depth of one. This
is preparation for allowing several commands to be in flight at once,
which is what makes independent per-slot ownership necessary.
Signed-off-by: Long Li <longli@microsoft.com>
---
.../net/ethernet/microsoft/mana/gdma_main.c | 8 +-
.../net/ethernet/microsoft/mana/hw_channel.c | 167 +++++++++++++++---
include/net/mana/hw_channel.h | 18 +-
3 files changed, 165 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..a023d3e1a95deeea3d15860f6fe7cab24f0b64e5 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -331,7 +331,13 @@ 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;
+ /* Zero is the driver's own "do not wait, do not log" sentinel, set by
+ * mana_serv_reset() when the HWC has stopped responding. A zero from
+ * the device would enter that state instead: ignore it and keep the
+ * caller's positive value.
+ */
+ 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 75fdccdc8c48201b011a243fd0bd41b84c477076..0056bdd8c53f5bf6b6b5f318f21faf7d9e14de53 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;
@@ -867,13 +908,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];
@@ -885,8 +932,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)
@@ -902,43 +952,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);
+
+ 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;
- /* Reduce further waiting if HWC no response */
+ /* 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 237608b488e5ee773d7889d3c1d291ca92ebbeb9..daff051472a0dd9f57ee0b9fb3d71029fca909ad 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] 9+ messages in thread* [PATCH net-next v4 3/4] net: mana: support concurrent HWC requests
2026-09-01 20:00 [PATCH net-next v4 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
2026-09-01 20:00 ` [PATCH net-next v4 1/4] net: mana: track when the HWC has been handed to the PF Long Li
2026-09-01 20:00 ` [PATCH net-next v4 2/4] net: mana: give each HWC message slot its own completion state Long Li
@ 2026-09-01 20:00 ` Long Li
2026-09-01 20:00 ` [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
3 siblings, 0 replies; 9+ messages in thread
From: Long Li @ 2026-09-01 20:00 UTC (permalink / raw)
To: 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
The HWC now manages message slots with a refcounted, per-slot-locked
bitmap (see the previous patch) but still runs at the bootstrap queue
depth of 1. Prepare the channel for multiple in-flight requests and
make teardown safe against them.
- Add a per-queue lock to hwc_wq; mana_gd_post_and_ring() is not safe
to call concurrently on the same queue.
- Bound the wait for a message slot. A sender blocked in down() could
only be woken by up(), which teardown never issued, so it slept with
no way to observe that the channel was going away. Keep the
counting semaphore -- its count is exactly the number of free slots
-- but acquire with down_timeout(), so a caller expires instead of
blocking for ever, and re-check channel_up under the bitmap lock
once a permit is held, returning the permit if the channel is on its
way down. A slot whose request timed out is never posted back (see
below), so the count falls permanently until the response that owns
it arrives or teardown reclaims it; that is what makes the bounded
wait terminate rather than spin. mlx5 gates its command slots the
same way (down_timeout() on cmd->vars.sem, -EBUSY on expiry).
- Add a channel_up flag, set once the channel is established and
cleared under the bitmap lock in destroy_channel(), so
mana_hwc_get_msg_index() rejects new senders during teardown. It is
read under the same lock that publishes a slot, so a sender that
already holds a permit cannot miss the clear: it either publishes
before teardown observes the bitmap, or finds the flag clear and
posts the permit back. Each released waiter posting its permit back
releases the next, so the force-completion in destroy_channel() --
which returns every in-flight slot, including the ones a timed-out
request was holding -- drains the whole queue.
- destroy_channel() must not free the HWC while senders are still in
flight. Count active senders in a gc->hwc_lock-protected counter;
destroy_channel() force-completes the in-flight slots (-ENODEV), then
drains the counter to zero with wait_event_lock_irq() before freeing.
The counter changes only under hwc_lock and the last sender's
wake_up() runs under that lock, so evaluating the drain condition
under hwc_lock guarantees the waking sender has already dropped the
lock -- finished touching gc -- before the drain returns; it cannot
race the later free of gc. The waitqueue itself lives on
gdma_context, not hwc, so the wake never dereferences freed hwc.
- The sender looks up the channel via gc->hwc.driver_data, which
destroy_channel() clears and then frees. Without serialization the
lookup and the reference can straddle the free:
CPU A (mana_gd_send_request) CPU B (destroy_channel)
------------------------------ ------------------------------
hwc = gc->hwc.driver_data; // ok
driver_data = NULL;
wait active_senders == 0; // 0!
kfree(hwc);
hwc->active_senders++; // use-after-free
Guard driver_data with a new gc->hwc_lock spinlock, taken by the
readers (mana_gd_send_request, mana_need_log, mana_serv_reset) and by
the publish/clear, so "load the pointer + take a sender reference" is
atomic against the clear. After the clear a sender either already
holds a reference (and is waited for) or observes NULL and returns
-ENODEV. These are all control-plane paths (HWC commands sleep,
reset runs on a workqueue), so a plain spinlock -- not RCU -- is
sufficient.
With more than one slot in use, a timed-out command also stops being
harmless to retire. The previous patch releases its slot while the
device may still answer, so the next request can take that slot and be
completed with the previous request's response -- responses are
correlated only by the slot index. So do not release it: a slot whose
request reached the hardware stays taken until the response arrives, and
only that response frees it. A request that never reached the hardware
cannot be answered, so its slot is still released immediately.
Slots held this way are counted, so a channel that is merely busy can be
told from one where nothing will ever free a slot again. Once every slot
is held by an unanswered command, mana_hwc_get_msg_index() fails with
-ETIMEDOUT rather than sleeping on a queue no one can wake, which would
otherwise deadlock teardown and reset against a device that has stopped
responding.
The next patch raises the depth to the device-reported maximum.
Signed-off-by: Long Li <longli@microsoft.com>
---
.../net/ethernet/microsoft/mana/gdma_main.c | 55 ++-
.../net/ethernet/microsoft/mana/hw_channel.c | 330 +++++++++++++++---
include/net/mana/gdma.h | 15 +
include/net/mana/hw_channel.h | 27 ++
4 files changed, 384 insertions(+), 43 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index a023d3e1a95deeea3d15860f6fe7cab24f0b64e5..9e7efce669ae0686e352cf953c051de160d74a0d 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)
@@ -393,9 +397,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);
- return 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 err;
}
EXPORT_SYMBOL_NS(mana_gd_send_request, "NET_MANA");
@@ -716,6 +738,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) {
@@ -725,14 +748,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");
@@ -1339,6 +1365,16 @@ static int mana_gd_create_dma_region(struct gdma_dev *gd,
if (gmi->nr_pages == 0 && !MANA_PAGE_ALIGNED(gmi->virt_addr))
return -EINVAL;
+ /* No RCU needed: this runs only on the data-path queue-creation
+ * path (mana_gd_create_mana_eq/mana_gd_create_mana_wq_cq, called
+ * by mana_en under RTNL and by mana_ib RDMA verbs, or during
+ * init). Every teardown path — mana_gd_remove, mana_gd_suspend,
+ * and the HWC reset/service path (which goes through
+ * mana_gd_suspend) — drains those consumers via mana_rdma_remove()
+ * + mana_remove() before mana_hwc_destroy_channel() clears
+ * gc->hwc.driver_data, so no concurrent destroy can race with
+ * this dereference.
+ */
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)
@@ -1544,7 +1580,17 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
struct hw_channel_context *hwc;
int err;
+ /* No RCU needed: this runs only inside mana_gd_setup, on the
+ * probe and resume paths. The PCI/PM core holds device_lock
+ * across .probe/.resume and .remove/.suspend, so setup cannot
+ * overlap teardown of the same device. The HWC reset/service
+ * path is additionally serialized by GC_IN_SERVICE and runs
+ * suspend (destroy) then resume (this) sequentially in one work
+ * item. driver_data was just set by mana_hwc_create_channel
+ * earlier in this same setup call, so it is live here.
+ */
hwc = gc->hwc.driver_data;
+
mana_gd_init_req_hdr(&req.hdr, GDMA_VERIFY_VF_DRIVER_VERSION,
sizeof(req), sizeof(resp));
@@ -2538,6 +2584,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 0056bdd8c53f5bf6b6b5f318f21faf7d9e14de53..91fcf7c092113133d392f0e43f2eda62ca5cb919 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -6,7 +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. */
+/* Acquire a free message slot from the inflight bitmap, waiting for one if
+ * all are in use. Returns -ENODEV if the channel is being torn down, or
+ * -ETIMEDOUT if a prior HWC command has timed out (preserving the error
+ * code callers expect).
+ */
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 +18,32 @@ static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16 *msg_id)
unsigned long flags;
u32 index;
- down(&hwc->sema);
+ /* Bounded wait for a slot. A timed-out request keeps its slot until
+ * the device answers for it, so the semaphore is never posted back
+ * for that slot and a caller expires here rather than blocking on a
+ * release that is not coming. Teardown reclaims those slots, which
+ * posts the semaphore and releases anyone waiting below.
+ */
+ 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 +52,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 +126,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,15 +139,32 @@ 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) {
+ 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.
+ */
+ 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;
err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
@@ -138,7 +181,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 +637,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 +655,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 +694,11 @@ 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;
+ /* Serialize WQE posting — multiple senders may call concurrently. */
+ 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;
@@ -660,6 +709,9 @@ static int mana_hwc_init_inflight_msg(struct hw_channel_context *hwc,
{
int err;
+ /* One permit per slot; a permit is returned only when the slot is
+ * released, so the count always mirrors the free slots.
+ */
sema_init(&hwc->sema, num_msg);
err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
@@ -675,6 +727,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 +750,34 @@ 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);
+ /* channel_up must be set before the test EQ request, because
+ * the request goes through mana_hwc_get_msg_index() which
+ * checks channel_up. caller_ctx is allocated above, so
+ * concurrent access to a NULL caller_ctx is not possible.
+ *
+ * Publish it under the bitmap lock, the same one the waiters and
+ * mana_hwc_destroy_channel() use, so the flag is never stored
+ * concurrently with the teardown that clears it.
+ */
+ 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) {
+ /* Clear channel_up under the bitmap lock, mirroring
+ * mana_hwc_destroy_channel(). Any sender already waiting on
+ * the semaphore is released by the slot holder that posts it,
+ * or by the teardown the caller runs on this error; each
+ * released waiter finds the flag clear and posts the permit
+ * straight back, so one permit walks the whole queue.
+ */
+ 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 +877,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 +886,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 +899,15 @@ int mana_hwc_create_channel(struct gdma_context *gc)
gd->pdid = INVALID_PDID;
gd->doorbell = INVALID_DOORBELL;
+ /* Publish driver_data last, under hwc_lock: the lock orders the hwc
+ * initialisation above before the pointer becomes visible and
+ * serialises the publish against the control-plane readers in
+ * mana_gd_send_request(), mana_need_log() and mana_serv_reset().
+ */
+ 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 +942,120 @@ int mana_hwc_create_channel(struct gdma_context *gc)
void mana_hwc_destroy_channel(struct gdma_context *gc)
{
+ /* This is the only destroy entry point. driver_data is read
+ * plainly here (teardown is serialised against other teardown);
+ * it is cleared under hwc_lock below before hwc is freed.
+ */
struct hw_channel_context *hwc = gc->hwc.driver_data;
+ unsigned long flags;
if (!hwc)
return;
+ /* Prevent new requests from starting. Clear channel_up under the
+ * bitmap lock so get_msg_index() cannot acquire a slot and increment
+ * active_senders after this point. Senders already waiting on the
+ * semaphore are released by the force-completion loop below, which
+ * returns every in-flight slot -- including the ones a timed-out
+ * request was holding; each released waiter sees the flag clear and
+ * posts its permit straight back, so they drain in turn.
+ *
+ * Gate on the bitmap rather than on channel_up: reading the flag
+ * unlocked and only then taking the lock would let a concurrent
+ * setup publish it in between and leave the channel up. A zero
+ * num_inflight_msg means mana_gd_alloc_res_map() never ran, so the
+ * lock is not initialised yet -- and no sender can exist either.
+ */
+ 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);
+ }
+
+ /* Clear the pointer under hwc_lock so new callers in
+ * mana_gd_send_request() see NULL and return -ENODEV. The lock
+ * makes the readers' "load driver_data + active_senders++"
+ * atomic against this store, so once it returns no new sender can
+ * take a reference; the active_senders drain below waits out those
+ * that already did, before their hwc is freed.
+ */
+ spin_lock_irqsave(&gc->hwc_lock, flags);
+ gc->hwc.driver_data = NULL;
+ spin_unlock_irqrestore(&gc->hwc_lock, flags);
+
+ /* Force-complete any in-flight senders so they observe -ENODEV,
+ * return, and drop their references. This runs before the HWC
+ * hardware teardown below, so a live interrupt may still deliver
+ * a real response via handle_resp() concurrently — that is safe
+ * because the per-slot refcount model tolerates a concurrent
+ * complete() and both paths (handle_resp and this loop) drop
+ * their refs without double-releasing the slot.
+ */
+ 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];
+
+ /* Wake senders blocked on wait_for_completion.
+ * Set error under lock to avoid racing with
+ * handle_resp() which writes error under the
+ * same lock. The sender NULLs output_buf
+ * after waking — doing it here would race
+ * with a sender that hasn't set output_buf yet.
+ *
+ * Latch ->responded so that a response still in
+ * flight cannot overwrite -ENODEV and report
+ * success for a request the channel is abandoning.
+ * handle_resp() then drops that response without
+ * touching the refcount, so release the
+ * response-side reference here instead.
+ */
+ spin_lock_irqsave(&ctx->lock, flags);
+ /* Do not clobber a response mana_hwc_handle_resp() has
+ * already delivered: its payload is in the caller's
+ * buffer and the command really did complete, so
+ * reporting -ENODEV would make the caller treat a
+ * hardware object it now owns as never created.
+ */
+ 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);
+ }
+ }
+
+ /* Wait for all sender threads to finish and drop their refs
+ * before touching the hardware or freeing anything, so no
+ * in-flight sender is still running when this function returns;
+ * otherwise a stranded sender would dereference gc->hwc_lock /
+ * gc->hwc_drain_waitq after the caller frees gc.
+ * After this, only slots held by timed-out senders whose
+ * handle_resp() never ran remain in the bitmap.
+ *
+ * active_senders is only ever modified under hwc_lock, and the
+ * last sender's wake_up() runs under that lock before it is
+ * released. Evaluating the condition under hwc_lock therefore
+ * guarantees that once we observe 0 the waking sender has
+ * already dropped the lock -- i.e. finished touching gc -- so it
+ * cannot race the caller freeing gc after this 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);
+
/* Tear down only if setup_hwc() handed the queues to the PF. Until
* then the device never saw them, so there is nothing to undo.
*/
@@ -871,14 +1071,36 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
}
gc->max_num_cqs = 0;
+ /* Destroy the HWC CQ object before the TXQ and RQ. The
+ * active_senders drain above already guarantees no sender is
+ * still reaching the CQ through txq->hwc_cq.
+ */
+ 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);
+ /* Safety net: the force-complete loop above dropped the
+ * response-side reference of every occupied slot and the sender
+ * drain released the matching sender references, so nothing should
+ * still be set here. Release anything that is, rather than leak it.
+ */
+ 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;
@@ -893,7 +1115,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);
@@ -910,6 +1131,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;
@@ -955,7 +1178,25 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
/* 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);
+ /* Submit under the slot lock, so mana_hwc_destroy_channel() cannot
+ * cancel this request between the check and the doorbell: it takes
+ * the same lock, so it either cancels before this runs -- and the
+ * request is never handed to the device -- or after, when the
+ * request is genuinely in flight. Posting is a WQE write plus a
+ * doorbell, so it does not sleep.
+ */
+ spin_lock_irqsave(&ctx->lock, flags);
+ cancelled = ctx->responded;
+ if (cancelled)
+ err = ctx->error;
+ 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;
@@ -972,9 +1213,23 @@ 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) {
+ /* Give up on this request in the same critical section
+ * that clears output_buf, so a response can never
+ * observe the slot as "sender has not published yet"
+ * and be discarded as premature -- that would strand
+ * the slot, because only a response frees it.
+ *
+ * Keep the response-side reference: the device may
+ * still answer, so the slot stays taken until it does
+ * and must not be handed to another request.
+ */
+ ctx->responded = true;
+ abandoned = true;
+ }
spin_unlock_irqrestore(&ctx->lock, flags);
- if (err != -EINPROGRESS) {
+ if (!abandoned) {
/* 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
@@ -986,29 +1241,25 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
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
@@ -1041,14 +1292,15 @@ 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.
+ /* Only reached before the request reached the hardware, so no
+ * response can ever arrive for it: latch ->responded and drop both
+ * the response-side and the sender's reference, freeing the slot.
*/
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..f97f63f8fee418f4ca36f88019690c096e6f2e6a 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -468,6 +468,21 @@ struct gdma_context {
/* Hardware communication channel (HWC) */
struct gdma_dev hwc;
+ /* destroy_channel() waits here for all HWC senders to exit.
+ * Lives on gc (not hwc) so wake_up() after the last sender's
+ * atomic_dec doesn't dereference freed hwc memory.
+ */
+ wait_queue_head_t hwc_drain_waitq;
+
+ /* Serializes hwc.driver_data (the hw_channel_context pointer)
+ * between the control-plane readers in mana_gd_send_request(),
+ * mana_need_log() and mana_serv_reset() and the publish/clear in
+ * mana_hwc_create_channel()/mana_hwc_destroy_channel(). All users
+ * are control-plane (HWC commands sleep; reset runs on a workqueue),
+ * so a plain spinlock -- not RCU -- is sufficient.
+ */
+ 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 daff051472a0dd9f57ee0b9fb3d71029fca909ad..5bddc87e8e78900097974968cefb6fb2d9415132 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 concurrent mana_gd_post_and_ring() calls. */
+ spinlock_t lock;
};
struct hwc_caller_ctx {
@@ -189,6 +192,12 @@ struct hwc_caller_ctx {
* so a later or duplicate response is dropped.
*/
bool responded;
+
+ /* True while the response-side reference is still held, i.e. while a
+ * response for this acquisition may still arrive. Dropped exactly
+ * once, by whoever establishes that no further response is coming.
+ */
+ bool resp_pending;
};
struct hw_channel_context {
@@ -196,6 +205,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 +218,12 @@ struct hw_channel_context {
struct hwc_wq *txq;
struct hwc_cq *cq;
+ /* Counts the message slots that are free to acquire. A slot held by
+ * a timed-out request is never posted back, so the count falls
+ * permanently until the response that owns it arrives or teardown
+ * reclaims it; a sender then expires in down_timeout() instead of
+ * blocking on a slot nothing will release.
+ */
struct semaphore sema;
struct gdma_resource inflight_msg_res;
@@ -215,6 +231,11 @@ struct hw_channel_context {
u32 pf_dest_vrcq_id;
u32 hwc_timeout;
+ /* Set after channel is fully established; cleared on teardown to
+ * abort waiters in mana_hwc_get_msg_index() and reject new sends.
+ */
+ bool channel_up;
+
/* True once mana_smc_setup_hwc() has handed the ESTABLISH_HWC message
* to the PF, so the device may DMA into the HWC buffers. That
* function clears it on entry and sets it at the handover, so only a
@@ -225,6 +246,12 @@ struct hw_channel_context {
*/
bool setup_active;
+ /* Count of in-flight mana_gd_send_request() callers. Protected
+ * by gc->hwc_lock; the last sender to drop it to zero wakes
+ * gc->hwc_drain_waitq for the mana_hwc_destroy_channel() drain.
+ */
+ unsigned int active_senders;
+
struct hwc_caller_ctx *caller_ctx;
};
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path
2026-09-01 20:00 [PATCH net-next v4 0/4] net: mana: concurrent HWC requests and dynamic queue depth Long Li
` (2 preceding siblings ...)
2026-09-01 20:00 ` [PATCH net-next v4 3/4] net: mana: support concurrent HWC requests Long Li
@ 2026-09-01 20:00 ` Long Li
2026-09-02 20:01 ` sashiko-bot
3 siblings, 1 reply; 9+ messages in thread
From: Long Li @ 2026-09-01 20:00 UTC (permalink / raw)
To: 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
The HWC is first established at a bootstrap queue depth of 1. Query the
device's maximum supported depth and, if larger, tear down and rebuild
the HWC queues at that depth before re-establishing the channel, so more
management commands can be in flight. Advertise
GDMA_DRV_CAP_FLAG_1_DYN_HWC_QUEUE_DEPTH so the firmware knows the driver
supports a non-bootstrap depth. That capability is sent by
mana_gd_verify_vf_version(), which is itself an HWC command and so
necessarily runs after the channel exists; the depth chosen here comes
from the device's own reported maximum.
mana_hwc_destroy_queues() tears down the CQ first, which deregisters the
EQ IRQ (mana_gd_deregister_irq() + synchronize_rcu()) so no interrupt
handler can touch the queues, then the TXQ, RXQ and inflight resources.
Validate the device-reported dimensions before they size DMA
allocations: take only the depth from the device and require the
negotiated message sizes to match the bootstrap ones, since mandatory
commands such as GDMA_VERIFY_VF_DRIVER_VERSION are much larger than the
protocol header minimum and mana_hwc_send_request() bounds a request
only against the slot it lands in; bound the depth by
HW_CHANNEL_MAX_QUEUE_DEPTH so the two coherent message buffers stay a
sane size; ensure
q_depth * max_msg_size plus alignment fits in u32, and cap CQ depth to
U16_MAX/2. Round the message-buffer length up to a power of two, which
mana_gd_alloc_memory() requires and which the EQ and CQ rings already
did, so a device reporting dimensions whose product is not a power of
two still gets the larger depth instead of falling back. Carry the
depth as u32 -- the device field is 24-bit, so
truncating to u16 on receipt could wrap a large value to a small depth
and silently pass these checks.
If the bootstrap channel cannot be torn down, keep using it at depth 1.
If the rebuild then fails before the new queue addresses were handed to
the PF, restore the bootstrap-depth channel: the device never saw them,
so the memory can be reused. If they were already handed over the
device may still DMA into those queues, so tear the channel down once
more before reusing them -- only a successful DESTROY_HWC invalidates
the MST entries. That matters because mana_smc_setup_hwc() marks the
handover before it reads the response, so a device that refuses the
larger depth still leaves the queues marked as handed over; retrying the
teardown lets such a device fall back to a working bootstrap channel
instead of failing probe outright. If that teardown also fails, nothing
has established that the device is finished with the queues, so give up
rather than rebuild over them.
Reject a device-reported message size above the driver maximum, and
reset the negotiated init values before each establish so a firmware
that omits an HWC_INIT_DATA_* item cannot reuse stale dimensions or
drive an oversized DMA allocation. The routing identities -- doorbell,
PDID and the PF destination queues -- are reset with them, since the
rebuilt queues are not the ones those described. Because that reset
runs on every establish, a firmware that supplied the doorbell on the
first one but omits it on the second would leave INVALID_DOORBELL
behind, so refuse the channel in that case rather than let
mana_gd_ring_doorbell() turn it into an unchecked write far outside the
BAR. The same write is already reachable without this patch when the
very first establish omits the doorbell; that is pre-existing and not
addressed here. Anything else left unset either fails the dimension
checks above or leaves the queues unable to complete, which
mana_hwc_test_channel() already catches. Finally, refuse a
rebuilt channel whose report contradicts the one its queues were built
from -- a shallower queue or smaller message limits -- rather than
oversubscribe what the device now admits to.
Signed-off-by: Long Li <longli@microsoft.com>
---
.../net/ethernet/microsoft/mana/hw_channel.c | 297 +++++++++++++++++-
include/net/mana/gdma.h | 4 +
include/net/mana/hw_channel.h | 9 +-
3 files changed, 304 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 91fcf7c092113133d392f0e43f2eda62ca5cb919..83df4f194ee3bb41affb2110553d502322057b98 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -223,7 +223,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:
@@ -553,7 +558,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);
@@ -780,7 +789,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)
{
@@ -797,6 +806,21 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
struct gdma_queue *cq = hwc->cq->gdma_cq;
int err;
+ /* Clear the values a previous establish left behind so a firmware
+ * that omits an HWC_INIT_DATA_* item on this cycle cannot silently
+ * reuse stale dimensions from the last one. The same applies to the
+ * routing identities: the queues are rebuilt from scratch, so a
+ * doorbell, PDID or PF destination left over from the previous
+ * channel does not describe them.
+ */
+ 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,
@@ -815,6 +839,20 @@ 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;
+ /* The doorbell was cleared before the handshake, so a firmware that
+ * signals INIT_DONE without sending GDMA_EQE_HWC_INIT_EQ_ID_DB
+ * leaves INVALID_DOORBELL behind. mana_gd_ring_doorbell() turns
+ * that into gc->db_page_base + gc->db_page_size * 0xffffffff, an
+ * unchecked MMIO write far outside the mapped BAR, and the channel
+ * test below rings it. Everything else the device reports either
+ * fails the dimension checks in mana_hwc_create_channel() or leaves
+ * the queues unable to complete, which that test already catches.
+ */
+ 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;
@@ -833,6 +871,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;
@@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
return err;
}
+/* Tear down all HWC queues and free associated resources. Used on
+ * the reinit-with-higher-queue-depth path and reinit fallback.
+ *
+ * PRECONDITION: must be called only during channel bring-up in
+ * mana_hwc_create_channel(), before the channel carries traffic:
+ * channel_up is still false, caller_ctx is not yet allocated, the
+ * data path is not probed yet, and active_senders is 0 — so no
+ * request or response user can reach these queues. That is why this
+ * skips the hwc_lock-protected driver_data clear + active_senders
+ * drain that mana_hwc_destroy_channel() needs for the runtime
+ * teardown race; only the CQ-first ordering below (to fence off a
+ * pending interrupt) is required. Bring-up itself runs under the
+ * PCI/PM device_lock, or under GC_IN_SERVICE on the service path;
+ * those two do not exclude each other, so a service reset racing a PM
+ * transition is not serialized — but that is pre-existing and applies
+ * equally to mana_hwc_destroy_channel(), which frees the same
+ * objects. Calling this on a live, published channel would be a
+ * use-after-free.
+ */
+static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
+{
+ struct gdma_context *gc = hwc->gdma_dev->gdma_context;
+
+ /* Destroy CQ first to deregister the EQ from the interrupt
+ * handler list before freeing caller_ctx, TXQ, or RXQ memory.
+ * A pending interrupt handler could still reach handle_resp()
+ * which dereferences caller_ctx.
+ */
+ 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);
@@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct gdma_context *gc)
goto out;
}
+ /* The channel was bootstrapped at a minimal queue depth. If the
+ * device reports a higher maximum, tear down and rebuild with
+ * the larger depth so more HWC commands can be in flight.
+ */
+ if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
+ /* q_depth_max now carries the full device-reported value
+ * (HWC_INIT_DATA_QUEUE_DEPTH is 24-bit). Clamp it before
+ * the overflow check below, so an over-large but otherwise
+ * valid depth is reduced instead of wrapping or being
+ * rejected. The bound also keeps the two coherent DMA
+ * buffers, which scale with the depth, to a sane size.
+ */
+ if (q_depth_max > HW_CHANNEL_MAX_QUEUE_DEPTH)
+ q_depth_max = HW_CHANNEL_MAX_QUEUE_DEPTH;
+
+ /* Sanity-check device-reported values before using them to
+ * size DMA allocations. Only the depth is taken from the
+ * device: the rebuilt queues must keep the bootstrap
+ * message sizes, because the rest of the driver already
+ * assumes it can send any request up to
+ * HW_CHANNEL_MAX_REQUEST_SIZE -- mandatory commands such as
+ * GDMA_VERIFY_VF_DRIVER_VERSION are far larger than the
+ * protocol header minimum, and mana_hwc_send_request() only
+ * bounds a request against the slot it lands in. Also check
+ * that q_depth * max_msg_size plus alignment headroom fits
+ * in u32 (for mana_hwc_alloc_dma_buf's MANA_PAGE_ALIGN).
+ */
+ 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) {
+ /* Keep using the bootstrap-depth channel. The
+ * destroy request may already have been written to
+ * the PF before the response failed, so the PF may
+ * have invalidated the MST entries; nothing is freed
+ * here, and mana_hwc_test_channel() below fails the
+ * channel creation if the queues are no longer
+ * usable.
+ */
+ dev_err(hwc->dev,
+ "Failed to teardown HWC for reinit: %d\n",
+ err);
+ q_depth_max = HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH;
+ goto skip_reinit;
+ }
+
+ hwc->setup_active = false;
+
+ /* Destroy the queues before the CQ table they refer to:
+ * mana_hwc_destroy_queues() releases the HWC CQ, so it must
+ * run while cq_table is still valid.
+ */
+ mana_hwc_destroy_queues(hwc);
+
+ old_cq_table = gc->cq_table;
+ gc->cq_table = NULL;
+ /* Clear the bound with the table: mana_gd_destroy_cq() gates
+ * on max_num_cqs before indexing cq_table, so leaving a stale
+ * bound behind would let it dereference the NULL table.
+ */
+ 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 contradicts the report its own
+ * queues were built from: a shallower queue would be
+ * oversubscribed by the slots already allocated, and
+ * smaller message limits would be exceeded by every
+ * command sized for the buffers already allocated.
+ * The queues are handed to the PF by now, so give up
+ * rather than run past what the device admits to.
+ */
+ 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);
+ /* setup_active tells us whether this attempt got
+ * as far as handing the queue addresses to the PF.
+ * If it did not, the device never saw them and the
+ * bootstrap fallback can safely reuse the memory.
+ *
+ * If it did, the mappings are live and rebuilding
+ * over them would let the device DMA into queues
+ * this path is about to free. Tear the channel down
+ * once more first: only a successful DESTROY_HWC
+ * invalidates the MST entries, and that is what makes
+ * the fallback safe again -- so a device that refuses
+ * the larger depth still ends up on a working
+ * bootstrap channel rather than failing probe
+ * outright. If that teardown also fails, nothing has
+ * established that the device is finished with the
+ * queues, so give up rather than reuse them.
+ */
+ if (hwc->setup_active) {
+ if (mana_smc_teardown_hwc(&gc->shm_channel,
+ false)) {
+ dev_err(hwc->dev,
+ "Failed to tear down HWC after failed reinit\n");
+ goto out;
+ }
+ hwc->setup_active = false;
+ }
+ goto reinit_fallback;
+ }
+ }
+
+ goto skip_reinit;
+
+reinit_fallback:
+ /* Restore bootstrap-depth channel so the device remains functional.
+ * Free cq_table if it was allocated by a partially successful
+ * establish attempt.
+ */
+ 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;
+ /* Clear the bound with the table, as above. */
+ 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 queues above were rebuilt with the bootstrap sizes, so
+ * a handshake that now reports different limits describes
+ * queues that do not exist. Commands are only bounded by the
+ * slots they land in, so they would be sized for the
+ * bootstrap limits and could exceed what the device accepts.
+ */
+ 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:
+
+ /* No RCU needed: still in mana_hwc_create_channel, the
+ * pointer has not been published to concurrent senders yet.
+ */
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 f97f63f8fee418f4ca36f88019690c096e6f2e6a..b435b2039c1f9d469f0f054ed37625cdfd1e1212 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -698,6 +698,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)
@@ -716,6 +719,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 5bddc87e8e78900097974968cefb6fb2d9415132..c2954f1f613736e7107c54d316bb12a804e83817 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
@@ -208,7 +215,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] 9+ messages in thread* Re: [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path
2026-09-01 20:00 ` [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path Long Li
@ 2026-09-02 20:01 ` sashiko-bot
2026-09-02 20:04 ` [EXTERNAL] " Long Li
0 siblings, 1 reply; 9+ messages in thread
From: sashiko-bot @ 2026-09-02 20:01 UTC (permalink / raw)
To: Long Li; +Cc: linux-hyperv
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Use-after-free of `hwc_cq->comp_buf` during queue teardown because memory is freed before the EQ IRQ is deregistered.
- [High] NULL pointer dereference on `gc->cq_table` during HWC reinit fallback on error paths because `gc->max_num_cqs` is updated asynchronously before table allocation.
--
commit b3057022f465825e22d893f907cd5cd5dd291b2f
Author: Long Li <longli@microsoft.com>
net: mana: add dynamic HWC queue depth with reinit path
This patch dynamically adjusts the hardware channel (HWC) queue depth by
tearing down the bootstrap channel and rebuilding it at a device-supported
maximum depth. This allows more management commands to be in flight.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
[ ... ]
> +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
> +{
> + struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> +
> + /* Destroy CQ first to deregister the EQ from the interrupt
> + * handler list before freeing caller_ctx, TXQ, or RXQ memory.
> + * A pending interrupt handler could still reach handle_resp()
> + * which dereferences caller_ctx.
> + */
> + if (hwc->cq) {
> + mana_hwc_destroy_cq(gc, hwc->cq);
[Severity: Critical]
This is a pre-existing issue, but since this patch dynamically tears down and
reconstructs the active HWC queues on the main probe path, does it turn a
teardown race into a reachable use-after-free regression?
Inside mana_hwc_destroy_cq(), the memory for hwc_cq->comp_buf is freed
before the EQ interrupt is deregistered:
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);
...
If an EQ interrupt fires concurrently (or a stray interrupt occurs), the
interrupt handler mana_gd_intr() calls the completion callback
mana_hwc_comp_event(). That callback will read the freed comp_buf pointer
and pass it to mana_gd_poll_cq(), causing out-of-bounds DMA completions to
be written into the freed memory.
> + hwc->cq = NULL;
> + }
[ ... ]
> @@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct gdma_context *gc)
[ ... ]
> +reinit_fallback:
> + /* Restore bootstrap-depth channel so the device remains functional.
> + * Free cq_table if it was allocated by a partially successful
> + * establish attempt.
> + */
> + dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap depth\n");
> +
> + mana_hwc_destroy_queues(hwc);
[Severity: High]
This is a pre-existing issue, but does this fallback path expose a null
pointer dereference on gc->cq_table?
If mana_hwc_establish_channel() fails after the firmware updates
gc->max_num_cqs (via an EQ event) but before gc->cq_table is allocated
(for example, if vcalloc fails or the doorbell is missing), we jump to
reinit_fallback.
In this path, mana_hwc_destroy_queues() is called before gc->max_num_cqs is
cleared. This calls down into mana_gd_destroy_cq(), which assumes
gc->cq_table is safely allocated if id < max_num_cqs:
mana_gd_destroy_cq() {
if (id >= gc->max_num_cqs)
return;
if (!gc->cq_table[id])
return;
...
This will dereference the null gc->cq_table pointer.
> +
> + old_cq_table = gc->cq_table;
> + gc->cq_table = NULL;
> + /* Clear the bound with the table, as above. */
> + gc->max_num_cqs = 0;
> + synchronize_rcu();
> + vfree(old_cq_table);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901200018.3194525-1-longli@microsoft.com?part=4
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [EXTERNAL] Re: [PATCH net-next v4 4/4] net: mana: add dynamic HWC queue depth with reinit path
2026-09-02 20:01 ` sashiko-bot
@ 2026-09-02 20:04 ` Long Li
0 siblings, 0 replies; 9+ messages in thread
From: Long Li @ 2026-09-02 20:04 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev; +Cc: linux-hyperv@vger.kernel.org
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Wednesday, September 2, 2026 1:01 PM
> To: Long Li <longli@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH net-next v4 4/4] net: mana: add dynamic
> HWC queue depth with reinit path
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> Pre-existing issues:
> - [Critical] Use-after-free of `hwc_cq->comp_buf` during queue teardown
> because memory is freed before the EQ IRQ is deregistered.
> - [High] NULL pointer dereference on `gc->cq_table` during HWC reinit
> fallback on error paths because `gc->max_num_cqs` is updated
> asynchronously before table allocation.
> --
>
> commit b3057022f465825e22d893f907cd5cd5dd291b2f
> Author: Long Li <longli@microsoft.com>
>
> net: mana: add dynamic HWC queue depth with reinit path
>
> This patch dynamically adjusts the hardware channel (HWC) queue depth by
> tearing down the bootstrap channel and rebuilding it at a device-supported
> maximum depth. This allows more management commands to be in flight.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
>
> [ ... ]
>
> > @@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct
> > hw_channel_context *hwc, u16 q_depth,
> [ ... ]
> > +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc) {
> > + struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> > +
> > + /* Destroy CQ first to deregister the EQ from the interrupt
> > + * handler list before freeing caller_ctx, TXQ, or RXQ memory.
> > + * A pending interrupt handler could still reach handle_resp()
> > + * which dereferences caller_ctx.
> > + */
> > + if (hwc->cq) {
> > + mana_hwc_destroy_cq(gc, hwc->cq);
>
> [Severity: Critical]
> This is a pre-existing issue, but since this patch dynamically tears down and
> reconstructs the active HWC queues on the main probe path, does it turn a
> teardown race into a reachable use-after-free regression?
>
> Inside mana_hwc_destroy_cq(), the memory for hwc_cq->comp_buf is freed
> before the EQ interrupt is deregistered:
>
> 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); ...
>
> If an EQ interrupt fires concurrently (or a stray interrupt occurs), the interrupt
> handler mana_gd_intr() calls the completion callback
> mana_hwc_comp_event(). That callback will read the freed comp_buf pointer
> and pass it to mana_gd_poll_cq(), causing out-of-bounds DMA completions
> to be written into the freed memory.
>
> > + hwc->cq = NULL;
> > + }
>
> [ ... ]
>
> > @@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct
> gdma_context
> > *gc)
> [ ... ]
> > +reinit_fallback:
> > + /* Restore bootstrap-depth channel so the device remains
> functional.
> > + * Free cq_table if it was allocated by a partially successful
> > + * establish attempt.
> > + */
> > + dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap
> > +depth\n");
> > +
> > + mana_hwc_destroy_queues(hwc);
>
> [Severity: High]
> This is a pre-existing issue, but does this fallback path expose a null pointer
> dereference on gc->cq_table?
>
> If mana_hwc_establish_channel() fails after the firmware updates
> gc->max_num_cqs (via an EQ event) but before gc->cq_table is allocated
> (for example, if vcalloc fails or the doorbell is missing), we jump to
> reinit_fallback.
>
> In this path, mana_hwc_destroy_queues() is called before gc->max_num_cqs
> is cleared. This calls down into mana_gd_destroy_cq(), which assumes
> gc->cq_table is safely allocated if id < max_num_cqs:
>
> mana_gd_destroy_cq() {
> if (id >= gc->max_num_cqs)
> return;
> if (!gc->cq_table[id])
> return;
> ...
>
> This will dereference the null gc->cq_table pointer.
>
> > +
> > + old_cq_table = gc->cq_table;
> > + gc->cq_table = NULL;
> > + /* Clear the bound with the table, as above. */
> > + gc->max_num_cqs = 0;
> > + synchronize_rcu();
> > + vfree(old_cq_table);
>
> --
Those are both pre-existing issues. The patch set implements multi queue depth support and doesn't attempt to fix the existing issues.
If needed, I will submit a separate patch to fix the existing issue. The patch is not related to this patch series.
Long
^ permalink raw reply [flat|nested] 9+ messages in thread