Re: [PATCH net-next v6 2/6] net: mana: Query device capabilities and configure MSI-X sharing for EQs

Simon Horman
2026-05-02T15:31:57Z

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

I apologise for any overlap with this and the review of this
patchset available at sashiko.dev. They both seem relevant and
I'm unsure how to cleanly pass them both on.

Both Sashiko instances also flagged concerns in the remaining
patches of this patchset. But I am not forwarding them as they
seem to be lower priority, and I think I've already created
enough noise here.

---
net: mana: Query device capabilities and configure MSI-X sharing for EQs

This patch queries device capabilities early in GDMA probe and chooses
between dedicated MSI-X per vPort or a shared-MSI-X mode based on the
available vector count, introducing gc->max_num_queues_vport,
gc->msi_sharing and gc->msi_bitmap, and renaming mana_query_device_cfg()
to mana_gd_query_device_cfg() so it can be called from the GDMA layer.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 098fbda0d128..b96859e0aec9 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -149,6 +149,9 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
> struct gdma_context *gc = pci_get_drvdata(pdev);
> struct gdma_query_max_resources_resp resp = {};
> struct gdma_general_req req = {};
> + unsigned int max_num_queues;
> + u8 bm_hostmode;
> + u16 num_ports;
> int err;
>
> mana_gd_init_req_hdr(&req.hdr, GDMA_QUERY_MAX_RESOURCES,
> @@ -194,6 +197,40 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
> if (gc->max_num_queues > gc->num_msix_usable - 1)
> gc->max_num_queues = gc->num_msix_usable - 1;
>
> + err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
> + MANA_MICRO_VERSION, &num_ports, &bm_hostmode);
> + if (err)
> + return err;

Is this the same MANA_QUERY_DEV_CONFIG that mana_probe() issues shortly
after? mana_probe() still contains:

err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION, MANA_MINOR_VERSION,
MANA_MICRO_VERSION, &num_ports, &bm_hostmode);

so the same HWC round-trip runs twice on every probe with identical
arguments, both populating gc->adapter_mtu.

The commit message frames this rename as consolidation ("used at GDMA
device probe time"), but the mana_probe() call-site is not removed. Was
one of the two call-sites meant to go away?

The relocation of debugfs_create_u16("adapter-MTU", ...) from
mana_gd_query_device_cfg() into mana_probe() only makes sense if the
function is now invoked more than once, which hints at the same
duplication.

> +
> + if (!num_ports)
> + return -EINVAL;
> +
> + /*
> + * Adjust gc->max_num_queues returned from the SOC to allow dedicated
> + * MSIx for each vPort. Clamp to no less than MANA_DEF_NUM_QUEUES.
> + */
> + max_num_queues = (gc->num_msix_usable - 1) / num_ports;
> + max_num_queues = roundup_pow_of_two(max(max_num_queues, 1U));
> + if (max_num_queues < MANA_DEF_NUM_QUEUES)
> + max_num_queues = MANA_DEF_NUM_QUEUES;
> +
> + /*
> + * Use dedicated MSIx for EQs whenever possible, use MSIx sharing for
> + * Ethernet EQs when (max_num_queues * num_ports > num_msix_usable - 1)
> + */
> + max_num_queues = min(gc->max_num_queues, max_num_queues);
> + if (max_num_queues * num_ports > gc->num_msix_usable - 1)
> + gc->msi_sharing = true;

Is gc->msi_sharing ever reset to false? The only two writers are this
line and mana_gd_setup_hwc_irqs() (the !pci_msix_can_alloc_dyn branch),
and both only set it to true. mana_gd_remove_irqs() frees msi_bitmap
and zeros max_num_msix / num_msix_usable, but does not clear
msi_sharing, and the gdma_context survives mana_gd_suspend() /
mana_gd_resume().

Once true is latched in any setup cycle, mana_gd_setup() will always
take the sharing branch on resume even if the recomputed
max_num_queues * num_ports fits within num_msix_usable - 1. Is that
consistent with the commit message saying sharing "is only enabled when
there are not enough MSI-X vectors for dedicated allocation"?

> +
> + /* If MSI is shared, use max allowed value */
> + if (gc->msi_sharing)
> + gc->max_num_queues_vport = min(gc->num_msix_usable - 1, gc->max_num_queues);
> + else
> + gc->max_num_queues_vport = max_num_queues;
> +
> + dev_info(gc->dev, "MSI sharing mode %d max queues %d\n",
> + gc->msi_sharing, gc->max_num_queues);

Should this print gc->max_num_queues_vport rather than
gc->max_num_queues? The block immediately above computes
gc->max_num_queues_vport and leaves gc->max_num_queues unchanged, and
mana_probe_port() sizes alloc_etherdev_mq() and apc->max_queues from
gc->max_num_queues_vport:

ndev = alloc_etherdev_mq(sizeof(struct mana_port_context),
gc->max_num_queues_vport);
...
apc->max_queues = gc->max_num_queues_vport;
apc->num_queues = min(gc->max_num_queues_vport, MANA_DEF_NUM_QUEUES);

so the logged value does not match the per-vPort queue count the driver
actually exposes.

> +
> return 0;
> }
>
> @@ -1856,6 +1893,7 @@ static int mana_gd_setup_hwc_irqs(struct pci_dev *pdev)

[ ... ]

> @@ -1968,20 +2008,30 @@ static int mana_gd_setup(struct pci_dev *pdev)
> if (err)
> goto destroy_hwc;
>
> - err = mana_gd_query_max_resources(pdev);
> + err = mana_gd_detect_devices(pdev);
> if (err)
> goto destroy_hwc;
>
> - err = mana_gd_setup_remaining_irqs(pdev);
> - if (err) {
> - dev_err(gc->dev, "Failed to setup remaining IRQs: %d", err);
> - goto destroy_hwc;
> - }
> -
> - err = mana_gd_detect_devices(pdev);
> + err = mana_gd_query_max_resources(pdev);
> if (err)
> goto destroy_hwc;
>
> + if (!gc->msi_sharing) {
> + gc->msi_bitmap = bitmap_zalloc(gc->num_msix_usable, GFP_KERNEL);
> + if (!gc->msi_bitmap) {
> + err = -ENOMEM;
> + goto destroy_hwc;
> + }
> + /* Set bit for HWC */
> + set_bit(0, gc->msi_bitmap);
> + } else {
> + err = mana_gd_setup_remaining_irqs(pdev);
> + if (err) {
> + dev_err(gc->dev, "Failed to setup remaining IRQs: %d", err);
> + goto destroy_hwc;
> + }
> + }
> +

Can the driver bring up any vPort after this change when the !msi_sharing
branch is taken?

In the dedicated branch, only gc->msi_bitmap is allocated and bit 0 is
set for HWC. mana_gd_setup_remaining_irqs() is skipped, so no
gdma_irq_context is inserted into gc->irq_contexts for indices 1..
num_msix_usable-1.

Later, mana_create_eq() still assigns

spec.eq.msix_index = (i + 1) % gc->num_msix_usable;

and mana_gd_register_irq() does:

gic = xa_load(&gc->irq_contexts, msi_index);
if (WARN_ON(!gic))
return -EINVAL;

On a typical cloud SKU with, for example, num_msix_usable=32,
num_ports=1 and num_online_cpus=16, the new math keeps msi_sharing=false
(16 * 1 <= 31), so every EQ-create goes down this path and hits the
WARN_ON. Doesn't that make every vPort open and every resume fail for
the common dedicated-MSI-X case?

The msi_bitmap allocated here is not consumed anywhere in this commit;
the on-demand allocation via mana_gd_get_gic() appears in the later
commit "net: mana: Allocate interrupt context for each EQ when creating
vPort" (dbbdf40a8974). Should the bitmap and the new branch be
introduced in the same commit that actually uses them, so each commit in
the series is independently bootable?

> dev_dbg(&pdev->dev, "mana gdma setup successful\n");
> return 0;
>