On Wed, 09 Sep 2026 01:38:27 +0000,
[email protected] 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
>
