[PATCH net-next] net: mana: Use the HWC destination queues reported by the hardware

[PATCH net-next] net: mana: Use the HWC destination queues reported by the hardware

[Manish Awasthi]

mana_hwc_send_request() gates the HWC destination queue IDs on
gc->is_pf, which the PCI probe derives from the device ID.

The gate is not needed. hwc->pf_dest_vrq_id and hwc->pf_dest_vrcq_id
are only ever assigned from the HWC_INIT_DATA_PF_DEST_RQ_ID and
HWC_INIT_DATA_PF_DEST_CQ_ID init events, and the hardware always
reports values that are valid for the function it sends them to.

Signed-off-by: Manish Awasthi <mawasthi@linux.microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/hw_channel.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 263e7c4e2934..5d75c2478a8a 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -855,13 +855,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
 int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 			  const void *req, u32 resp_len, void *resp)
 {
-	struct gdma_context *gc = hwc->gdma_dev->gdma_context;
 	struct hwc_work_request *tx_wr;
 	struct hwc_wq *txq = hwc->txq;
 	struct gdma_req_hdr *req_msg;
 	struct hwc_caller_ctx *ctx;
-	u32 dest_vrcq = 0;
-	u32 dest_vrq = 0;
+	u32 dest_vrcq;
+	u32 dest_vrq;
 	u32 command;
 	u16 msg_id;
 	int err;
@@ -890,10 +889,13 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
 	tx_wr->msg_size = req_len;
 	command = req_msg->req.msg_type;
 
-	if (gc->is_pf) {
-		dest_vrq = hwc->pf_dest_vrq_id;
-		dest_vrcq = hwc->pf_dest_vrcq_id;
-	}
+	/* The hardware reports the HWC destination queues through
+	 * HWC_INIT_DATA_PF_DEST_RQ_ID and HWC_INIT_DATA_PF_DEST_CQ_ID, and
+	 * always supplies values that are valid for this function, so no
+	 * PF-specific handling is needed here.
+	 */
+	dest_vrq = hwc->pf_dest_vrq_id;
+	dest_vrcq = hwc->pf_dest_vrcq_id;
 
 	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
 	if (err) {

base-commit: 1b78070aaef63512688aebfbc82365ef9d6660f1
-- 
2.54.0

RE: [PATCH net-next] net: mana: Use the HWC destination queues reported by the hardware

[Long Li]

> mana_hwc_send_request() gates the HWC destination queue IDs on
> gc->is_pf, which the PCI probe derives from the device ID.
> 
> The gate is not needed. hwc->pf_dest_vrq_id and hwc->pf_dest_vrcq_id are
> only ever assigned from the HWC_INIT_DATA_PF_DEST_RQ_ID and
> HWC_INIT_DATA_PF_DEST_CQ_ID init events, and the hardware always
> reports values that are valid for the function it sends them to.
> 
> Signed-off-by: Manish Awasthi <mawasthi@linux.microsoft.com>

Reviewed-by: Long Li <longli@microsoft.com>



> ---
>  drivers/net/ethernet/microsoft/mana/hw_channel.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 263e7c4e2934..5d75c2478a8a 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -855,13 +855,12 @@ void mana_hwc_destroy_channel(struct
> gdma_context *gc)  int mana_hwc_send_request(struct hw_channel_context
> *hwc, u32 req_len,
>  			  const void *req, u32 resp_len, void *resp)  {
> -	struct gdma_context *gc = hwc->gdma_dev->gdma_context;
>  	struct hwc_work_request *tx_wr;
>  	struct hwc_wq *txq = hwc->txq;
>  	struct gdma_req_hdr *req_msg;
>  	struct hwc_caller_ctx *ctx;
> -	u32 dest_vrcq = 0;
> -	u32 dest_vrq = 0;
> +	u32 dest_vrcq;
> +	u32 dest_vrq;
>  	u32 command;
>  	u16 msg_id;
>  	int err;
> @@ -890,10 +889,13 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
>  	tx_wr->msg_size = req_len;
>  	command = req_msg->req.msg_type;
> 
> -	if (gc->is_pf) {
> -		dest_vrq = hwc->pf_dest_vrq_id;
> -		dest_vrcq = hwc->pf_dest_vrcq_id;
> -	}
> +	/* The hardware reports the HWC destination queues through
> +	 * HWC_INIT_DATA_PF_DEST_RQ_ID and
> HWC_INIT_DATA_PF_DEST_CQ_ID, and
> +	 * always supplies values that are valid for this function, so no
> +	 * PF-specific handling is needed here.
> +	 */
> +	dest_vrq = hwc->pf_dest_vrq_id;
> +	dest_vrcq = hwc->pf_dest_vrcq_id;
> 
>  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
>  	if (err) {
> 
> base-commit: 1b78070aaef63512688aebfbc82365ef9d6660f1
> --
> 2.54.0

RE: [PATCH net-next] net: mana: Use the HWC destination queues reported by the hardware

[Haiyang Zhang]

> -----Original Message-----
> From: Manish Awasthi <mawasthi@linux.microsoft.com>
> Sent: Thursday, September 3, 2026 2:41 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <DECUI@microsoft.com>; Long Li <longli@microsoft.com>;
> andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com
> Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; horms@kernel.org; gargaditya@linux.microsoft.com;
> ernis@linux.microsoft.com; kees@kernel.org; Paul Rosswurm
> <paulros@microsoft.com>
> Subject: [PATCH net-next] net: mana: Use the HWC destination queues
> reported by the hardware
> 
> mana_hwc_send_request() gates the HWC destination queue IDs on
> gc->is_pf, which the PCI probe derives from the device ID.
> 
> The gate is not needed. hwc->pf_dest_vrq_id and hwc->pf_dest_vrcq_id
> are only ever assigned from the HWC_INIT_DATA_PF_DEST_RQ_ID and
> HWC_INIT_DATA_PF_DEST_CQ_ID init events, and the hardware always
> reports values that are valid for the function it sends them to.
> 
> Signed-off-by: Manish Awasthi <mawasthi@linux.microsoft.com>

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

Re: [PATCH net-next] net: mana: Use the HWC destination queues reported by the hardware

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 1 (1)

- [High] Removing the `if (gc->is_pf)` gate in `mana_hwc_send_request()`
  (drivers/net/ethernet/microsoft/mana/hw_channel.c:897-898) makes…
- [Medium] Commit message understates a functional change and carries no
  Fixes tag.
- [Low] The patch redefines the meaning of hardware-protocol fields but
  renames nothing, leaving an internally contradictory abstraction: the…

Pre-existing issues:
- [Low] `hwc->pf_dest_vrq_id` and `hwc->pf_dest_vrcq_id` are written from
  the HWC EQ callback `mana_hwc_init_event_handler()`…

--

net: mana: Use the HWC destination queues reported by the hardware

This removes the gc->is_pf gate in mana_hwc_send_request() so that the HWC
destination queue IDs learned from the HWC init events are used
unconditionally.  Previously non-PF functions posted every HWC request with
destination vRQ/vRCQ of 0.

No Link: tags were present in the commit header.

A few questions on the change below.

> net: mana: Use the HWC destination queues reported by the hardware
>
> mana_hwc_send_request() gates the HWC destination queue IDs on
> gc->is_pf, which the PCI probe derives from the device ID.
>
> The gate is not needed. hwc->pf_dest_vrq_id and hwc->pf_dest_vrcq_id
> are only ever assigned from the HWC_INIT_DATA_PF_DEST_RQ_ID and
> HWC_INIT_DATA_PF_DEST_CQ_ID init events, and the hardware always
> reports values that are valid for the function it sends them to.

[Severity: Medium]
Is "The gate is not needed" an accurate description of what changes here?

mana_is_pf() in gdma_main.c returns true only for MANA_PF_DEVICE_ID and
MANA_PF2_DEVICE_ID, so mana_gd_probe() leaves gc->is_pf false for every VF:

	gc->is_pf = mana_is_pf(pdev->device);
	gc->is_pf2 = (pdev->device == MANA_PF2_DEVICE_ID);

That means the removed branch was false exactly on VFs, and on those
functions mana_hwc_send_request() used to post with dest_vrq = dest_vrcq =
0 and now posts hwc->pf_dest_vrq_id / hwc->pf_dest_vrcq_id instead.

The two statements in the message seem to pull in opposite directions:
"The gate is not needed" reads as a no-op cleanup, while "the hardware
always reports values that are valid for the function it sends them to"
only carries meaning if the hardware does report these values to VFs, i.e.
if VF behaviour changes.

Could the changelog say which of the two it is, name the user-visible
symptom being fixed (HWC command timeout, probe failure, something else),
say which host/firmware versions are affected, and which of PF, PF2, VF
and bare-metal hostmode were tested?

Also, both the pf_dest_* fields and the gc->is_pf gate being deleted came
from commit 1566e7d6206f ("net: mana: Add the Linux MANA PF driver").
Should this carry a Fixes: tag naming it?

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 263e7c4e29341..5d75c2478a8a8 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -890,10 +889,13 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  	tx_wr->msg_size = req_len;
>  	command = req_msg->req.msg_type;
>  
> -	if (gc->is_pf) {
> -		dest_vrq = hwc->pf_dest_vrq_id;
> -		dest_vrcq = hwc->pf_dest_vrcq_id;
> -	}
> +	/* The hardware reports the HWC destination queues through
> +	 * HWC_INIT_DATA_PF_DEST_RQ_ID and HWC_INIT_DATA_PF_DEST_CQ_ID, and
> +	 * always supplies values that are valid for this function, so no
> +	 * PF-specific handling is needed here.
> +	 */
> +	dest_vrq = hwc->pf_dest_vrq_id;
> +	dest_vrcq = hwc->pf_dest_vrcq_id;

[Severity: High]
Where in the driver is the invariant stated by the new comment actually
established?

The only producers of these two fields are in mana_hwc_init_event_handler():

	case HWC_INIT_DATA_PF_DEST_RQ_ID:
		hwc->pf_dest_vrq_id = val;
		break;

	case HWC_INIT_DATA_PF_DEST_CQ_ID:
		hwc->pf_dest_vrcq_id = val;
		break;

There is no gc->is_pf / gc->is_pf2 test there, no range check against
gc->max_num_cqs, and no check that the reported queues belong to the
receiving function.  Compare mana_hwc_establish_channel(), which does at
least sanity-check the HWC CQ id it got from the same handler:

	/* Both were set in mana_hwc_init_event_handler(). */
	if (WARN_ON(cq->id >= gc->max_num_cqs))
		return -EPROTO;

So if a non-PF function ever receives those init events with non-zero
values, is every HWC request now addressed to a queue that function may not
own?

The values go straight into the hardware OOB in mana_hwc_post_tx_wqe():

	tx_oob->vrq_id = dest_virt_rq_id;
	tx_oob->dest_vfid = 0;
	tx_oob->vrcq_id = dest_virt_rcq_id;
	...
	tx_oob->dest_pf = dest_pf;

and this call site still passes dest_pf = false while dest_vfid is
hardcoded to 0, which does not look like a descriptor addressed at a PF
destination.

If a request is misdirected, does the channel recover?  In
mana_hwc_send_request() the completion never arrives, the
wait_for_completion_timeout(&ctx->comp_event, ...) expires with -ETIMEDOUT,
and the same path then does:

		/* Reduce further waiting if HWC no response */
		if (hwc->hwc_timeout > 1)
			hwc->hwc_timeout = 1;

after which every later command fails fast.  Since the first users of this
path run during mana_hwc_create_channel() and mana_hwc_test_channel() at
bring-up, would that show up as a probe failure and loss of the interface?

The ABI names (HWC_INIT_DATA_PF_DEST_RQ_ID, HWC_INIT_DATA_PF_DEST_CQ_ID,
pf_dest_vrq_id, pf_dest_vrcq_id) and the fact that the gate and the fields
were added together for the bare-metal PF path all read as evidence
against the premise.  Would it be safer to keep the function-type gate, or
to move the gate/validation into mana_hwc_init_event_handler() and document
the firmware contract there?

[Severity: Low]
This isn't a bug, but the patch redefines the meaning of these fields
without renaming anything.  The declarations stay as:

	u32 pf_dest_vrq_id;
	u32 pf_dest_vrcq_id;

the init-data constants stay as HWC_INIT_DATA_PF_DEST_RQ_ID and
HWC_INIT_DATA_PF_DEST_CQ_ID, mana_hwc_post_tx_wqe() is still called with
dest_pf = false, and the only record of the new semantics is free text at
this one call site.

A later reader seeing pf_dest_vrq_id consumed unconditionally right next to
dest_pf = false cannot tell whether that is deliberate.  If the premise
holds, could the fields and constants be renamed (dest_vrq_id,
HWC_INIT_DATA_DEST_RQ_ID) so the invariant lives at the definition?

[Severity: Low]
This is a pre-existing issue rather than something introduced here, but
the patch widens it from PF-only to all function types, so it seems worth
mentioning.

These two fields are written from the HWC EQ callback
mana_hwc_init_event_handler() and read here in process context with no
lock, no READ_ONCE(), and as two separate plain loads, even though
mana_hwc_post_tx_wqe() consumes them as a pair.

The initial publication is ordered correctly: the stores precede
GDMA_EQE_HWC_INIT_DONE -> complete(&hwc->hwc_init_eqe_comp), and
mana_hwc_establish_channel() waits on that completion before anything is
posted:

	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
		return -ETIMEDOUT;

The EQ callback stays registered for the life of the channel though, and
the GDMA_EQE_HWC_INIT_DATA switch has no guard against a further init-data
event while requests are in flight.  If one lands between these two reads,
can the WQE end up carrying a vrq_id from one generation and a vrcq_id
from another, so the completion is steered at a queue the driver is not
waiting on and the caller blocks for the full hwc_timeout?

I have no evidence any firmware re-delivers that event, and the same
handler would also rewrite hwc->cq->gdma_cq->id, the rxq/txq gdma_wq ids
and gpa_mkey unsynchronized, so this is a property of the whole init-data
handler rather than of the two fields touched here.

>  
>  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
>  	if (err) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903184104.3194819-1-mawasthi%40linux.microsoft.com

Re: [PATCH net-next] net: mana: Use the HWC destination queues reported by the hardware

[Manish Awasthi]

On Wed, 09 Sep 2026 01:38:27 +0000,
netdev-bot+sashiko@kernel.org wrote:

Thanks for the review.
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 1 (1)
> 
> - [High] Removing the `if (gc->is_pf)` gate in `mana_hwc_send_request()`
>   (drivers/net/ethernet/microsoft/mana/hw_channel.c:897-898) makes…
> - [Medium] Commit message understates a functional change and carries no
>   Fixes tag.
> - [Low] The patch redefines the meaning of hardware-protocol fields but
>   renames nothing, leaving an internally contradictory abstraction: the…
> 
> Pre-existing issues:
> - [Low] `hwc->pf_dest_vrq_id` and `hwc->pf_dest_vrcq_id` are written from
>   the HWC EQ callback `mana_hwc_init_event_handler()`…
> 
> --
> 
> net: mana: Use the HWC destination queues reported by the hardware
> 
> This removes the gc->is_pf gate in mana_hwc_send_request() so that the HWC
> destination queue IDs learned from the HWC init events are used
> unconditionally.  Previously non-PF functions posted every HWC request with
> destination vRQ/vRCQ of 0.
> 
> No Link: tags were present in the commit header.
> 
> A few questions on the change below.
> 
> > net: mana: Use the HWC destination queues reported by the hardware
> >
> > mana_hwc_send_request() gates the HWC destination queue IDs on
> > gc->is_pf, which the PCI probe derives from the device ID.
> >
> > The gate is not needed. hwc->pf_dest_vrq_id and hwc->pf_dest_vrcq_id
> > are only ever assigned from the HWC_INIT_DATA_PF_DEST_RQ_ID and
> > HWC_INIT_DATA_PF_DEST_CQ_ID init events, and the hardware always
> > reports values that are valid for the function it sends them to.
> 
> [Severity: Medium]
> Is "The gate is not needed" an accurate description of what changes here?
> 
I'll change the message to clarify that the check is not needed for any device.
> mana_is_pf() in gdma_main.c returns true only for MANA_PF_DEVICE_ID and
> MANA_PF2_DEVICE_ID, so mana_gd_probe() leaves gc->is_pf false for every VF:
> 
> 	gc->is_pf = mana_is_pf(pdev->device);
> 	gc->is_pf2 = (pdev->device == MANA_PF2_DEVICE_ID);
> 
> That means the removed branch was false exactly on VFs, and on those
> functions mana_hwc_send_request() used to post with dest_vrq = dest_vrcq =
> 0 and now posts hwc->pf_dest_vrq_id / hwc->pf_dest_vrcq_id instead.
> 
The removed branch was false on VF and PF2 before this change. Now the branch
is removed.

> The two statements in the message seem to pull in opposite directions:
> "The gate is not needed" reads as a no-op cleanup, while "the hardware
> always reports values that are valid for the function it sends them to"
> only carries meaning if the hardware does report these values to VFs, i.e.
> if VF behaviour changes.
> 
> Could the changelog say which of the two it is, name the user-visible
> symptom being fixed (HWC command timeout, probe failure, something else),
> say which host/firmware versions are affected, and which of PF, PF2, VF
> and bare-metal hostmode were tested?
The change applies to all supported device types. I will update the changelog.
> 
> Also, both the pf_dest_* fields and the gc->is_pf gate being deleted came
> from commit 1566e7d6206f ("net: mana: Add the Linux MANA PF driver").
> Should this carry a Fixes: tag naming it?
> 
This patch doesn't fix a bug or an issue introduced by commit 1566e7d6206f, so I don't think a Fixes tag is appropriate.
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 263e7c4e29341..5d75c2478a8a8 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> 
> [ ... ]
> 
> > @@ -890,10 +889,13 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> >  	tx_wr->msg_size = req_len;
> >  	command = req_msg->req.msg_type;
> >  
> > -	if (gc->is_pf) {
> > -		dest_vrq = hwc->pf_dest_vrq_id;
> > -		dest_vrcq = hwc->pf_dest_vrcq_id;
> > -	}
> > +	/* The hardware reports the HWC destination queues through
> > +	 * HWC_INIT_DATA_PF_DEST_RQ_ID and HWC_INIT_DATA_PF_DEST_CQ_ID, and
> > +	 * always supplies values that are valid for this function, so no
> > +	 * PF-specific handling is needed here.
> > +	 */
> > +	dest_vrq = hwc->pf_dest_vrq_id;
> > +	dest_vrcq = hwc->pf_dest_vrcq_id;
> 
> [Severity: High]
> Where in the driver is the invariant stated by the new comment actually
> established?
I'll rename the HWC_INIT_DATA_PF_DEST_RQ_ID and HWC_INIT_DATA_PF_DEST_CQ_ID to
HWC_INIT_DATA_DEST_CQ_ID and HWC_INIT_DATA_DEST_CQ_ID. The invariant is established during 
the HWC handshake regardless of whether its PF, PF2, VF or bare-metal.
> 
> The only producers of these two fields are in mana_hwc_init_event_handler():
> 
> 	case HWC_INIT_DATA_PF_DEST_RQ_ID:
> 		hwc->pf_dest_vrq_id = val;
> 		break;
> 
> 	case HWC_INIT_DATA_PF_DEST_CQ_ID:
> 		hwc->pf_dest_vrcq_id = val;
> 		break;
> 
> There is no gc->is_pf / gc->is_pf2 test there, no range check against
> gc->max_num_cqs, and no check that the reported queues belong to the
> receiving function.  Compare mana_hwc_establish_channel(), which does at
> least sanity-check the HWC CQ id it got from the same handler:
> 
> 	/* Both were set in mana_hwc_init_event_handler(). */
> 	if (WARN_ON(cq->id >= gc->max_num_cqs))
> 		return -EPROTO;
> 
> So if a non-PF function ever receives those init events with non-zero
> values, is every HWC request now addressed to a queue that function may not
> own?
Hardware sends the values for these fields as appropriate for the function it is sending them to.
A non zero value is not invalid.
> 
> The values go straight into the hardware OOB in mana_hwc_post_tx_wqe():
> 
> 	tx_oob->vrq_id = dest_virt_rq_id;
> 	tx_oob->dest_vfid = 0;
> 	tx_oob->vrcq_id = dest_virt_rcq_id;
> 	...
> 	tx_oob->dest_pf = dest_pf;
> 
> and this call site still passes dest_pf = false while dest_vfid is
> hardcoded to 0, which does not look like a descriptor addressed at a PF
> destination.
> 
> If a request is misdirected, does the channel recover?  In

This is an existing behavior and this patch doesn't change it. The recovery is already
implemented in the driver.

> mana_hwc_send_request() the completion never arrives, the
> wait_for_completion_timeout(&ctx->comp_event, ...) expires with -ETIMEDOUT,
> and the same path then does:
> 
> 		/* Reduce further waiting if HWC no response */
> 		if (hwc->hwc_timeout > 1)
> 			hwc->hwc_timeout = 1;
> 
> after which every later command fails fast.  Since the first users of this
> path run during mana_hwc_create_channel() and mana_hwc_test_channel() at
> bring-up, would that show up as a probe failure and loss of the interface?

There is recovery implemented in the driver on probe failure.
> 
> The ABI names (HWC_INIT_DATA_PF_DEST_RQ_ID, HWC_INIT_DATA_PF_DEST_CQ_ID,
> pf_dest_vrq_id, pf_dest_vrcq_id) and the fact that the gate and the fields
> were added together for the bare-metal PF path all read as evidence
> against the premise.  Would it be safer to keep the function-type gate, or
> to move the gate/validation into mana_hwc_init_event_handler() and document
> the firmware contract there?

I will send v2 with fields renamed.
> 
> [Severity: Low]
> This isn't a bug, but the patch redefines the meaning of these fields
> without renaming anything.  The declarations stay as:
> 
> 	u32 pf_dest_vrq_id;
> 	u32 pf_dest_vrcq_id;
> 
> the init-data constants stay as HWC_INIT_DATA_PF_DEST_RQ_ID and
> HWC_INIT_DATA_PF_DEST_CQ_ID, mana_hwc_post_tx_wqe() is still called with
> dest_pf = false, and the only record of the new semantics is free text at
> this one call site.
> 
> A later reader seeing pf_dest_vrq_id consumed unconditionally right next to
> dest_pf = false cannot tell whether that is deliberate.  If the premise
> holds, could the fields and constants be renamed (dest_vrq_id,
> HWC_INIT_DATA_DEST_RQ_ID) so the invariant lives at the definition?

Yes, I will send v2 with the fields and constants renamed.

> 
> [Severity: Low]
> This is a pre-existing issue rather than something introduced here, but
> the patch widens it from PF-only to all function types, so it seems worth
> mentioning.
> 
> These two fields are written from the HWC EQ callback
> mana_hwc_init_event_handler() and read here in process context with no
> lock, no READ_ONCE(), and as two separate plain loads, even though
> mana_hwc_post_tx_wqe() consumes them as a pair.
> 
> The initial publication is ordered correctly: the stores precede
> GDMA_EQE_HWC_INIT_DONE -> complete(&hwc->hwc_init_eqe_comp), and
> mana_hwc_establish_channel() waits on that completion before anything is
> posted:
> 
> 	if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
> 		return -ETIMEDOUT;
> 
> The EQ callback stays registered for the life of the channel though, and
> the GDMA_EQE_HWC_INIT_DATA switch has no guard against a further init-data
> event while requests are in flight.  If one lands between these two reads,
> can the WQE end up carrying a vrq_id from one generation and a vrcq_id
> from another, so the completion is steered at a queue the driver is not
> waiting on and the caller blocks for the full hwc_timeout?
> 
> I have no evidence any firmware re-delivers that event, and the same
> handler would also rewrite hwc->cq->gdma_cq->id, the rxq/txq gdma_wq ids
> and gpa_mkey unsynchronized, so this is a property of the whole init-data
> handler rather than of the two fields touched here.
> 
> >  
> >  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
> >  	if (err) {
> 
Thanks for reporting the issue, since it it pre-existing, we'll review and address it in a separate patch.
Manish
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903184104.3194819-1-mawasthi%40linux.microsoft.com
>