Re: [PATCH net-next 4/4] net: mana: Add support for CDX device ID 0x00C2

[Manish Awasthi <mawasthi@linux.microsoft.com>]

thanks for review. Comments inline.

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The CDX transport installs `.dev_reset = mana_cdx_dev_reset`
>   (-> `cdx_dev_reset()`) but has no way to recover afterwards.
> - [Low] `mana_cdx_setup_hwc_irqs()` skips IRQ affinity hints.
> - [Low] The comment in `mana_gd_query_max_resources()` (gdma_main.c)
>   says: "Only reset on platforms that support dynamic MSI-X allocation;…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the shared HWC
>   bootstrap path that the new CDX transport now reaches trusts a device-…
> 
> --
> 
> net: mana: Add support for CDX device ID 0x00C2
> 
> This adds a CDX bus transport for MANA, built as mana_cdx.ko on top of
> the shared gdma_core.ko, for device ID 0x00C2. It allocates the whole MSI
> pool at probe time, limits the queue count to the available vectors,
> restricts DMA to 32 bits, and adds a per-bus auxiliary device name prefix
> so the CDX auxiliary devices stay separate from the PCI ones.
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_cdx.c b/drivers/net/ethernet/microsoft/mana/gdma_cdx.c
> > new file mode 100644
> > index 0000000000000..0d01e3d2f0d96
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_cdx.c
> > @@ -0,0 +1,340 @@
> 
> [ ... ]
> 
> > +static int mana_cdx_dev_reset(struct gdma_context *gc)
> > +{
> > +	return cdx_dev_reset(gc->dev);
> > +}
> 
> [Severity: Medium]
> Can this reset leave the whole function unusable, with no recovery short
> of a manual rebind?
> 
> mana_dealloc_queues() calls it when a TX queue still has pending_sends
> after the drain timeout:
> 
> mana_tx_timeout()
>   -> queue_reset_work
>     -> mana_per_port_queue_reset_work_handler()
>       -> mana_dealloc_queues()
>         -> mana_gd_dev_reset()
>           -> mana_cdx_dev_reset()
>             -> cdx_dev_reset()
> 
This version of the driver doesn't support recovery after mana_cdx_dev_reset.

> The detach paths can reach mana_dealloc_queues() too.
> 
> drivers/cdx/controller/mc_cdx_pcol.h documents MC_CMD_CDX_DEVICE_RESET
> like this:
> 
>  * After this call completes, device DMA and interrupts are quiesced, devices
>  * logic is reset in a hardware-specific way and DMA bus mastering is disabled.
> 
> That resets the HWC, all EQs, the other vPorts and RDMA, but the driver
> keeps treating all of them as live. There is no re-init path on CDX:
> 
> - Bus mastering is only turned on by cdx_set_master() in
>   mana_cdx_gd_probe().
> - MSI is only enabled in mana_cdx_setup_hwc_irqs().
> - gdma_cdx_driver has no reset_prepare or reset_done callbacks.
> - mana_cdx_bus_ops has no schedule_serv_work.
> 
> So would the HWC requests that follow (mana_disable_vport_rx(), vPort
> object teardown, re-attach) all time out? And when
> mana_gf_stats_work_handler() detects the HWC timeout, would its servicing
> request just be rejected with -EOPNOTSUPP?
> 
The first HWC request after reset call will wait for default timeout and subsequent commands will fail after 1ms.
A future patch will change the tx timeout recovery to reset the timeout to 0 after dev_reset for both pci and cdx.

> On PCI, the same reset through pcie_flr() is backed by HWC-timeout
> recovery through mana_pci_schedule_serv_work().
> 
> CDX also still advertises stall-SQ recovery to firmware through the
> common GDMA_DRV_CAP_FLAGS1:
> 
> 	 GDMA_DRV_CAP_FLAG_1_HANDLE_STALL_SQ_RECOVERY | \
> 
> Without a dev_reset op, mana_gd_dev_reset() would return -EOPNOTSUPP, and
> a stall in one queue would not reset the whole function.
This flag represents the capability of the driver to support ndo_tx_timeout for cdx.
> 
> There is a second trigger. reset_store() in drivers/cdx/cdx.c calls
> cdx_dev_reset() on a bound device, and
> Documentation/ABI/testing/sysfs-bus-cdx says the driver gets notified.
> With no reset_prepare/reset_done here, wouldn't a sysfs reset leave the
> device in the same state?
> 
> [ ... ]
> 
> > +	xa_init(&gc->irq_contexts);
> > +
> > +	/* No IRQ affinity hint is applied here. The PCI transport spreads its
> > +	 * vectors over the NUMA node local to the device; the CDX platforms
> > +	 * this driver targets are single-node, so there is nothing to spread
> > +	 * over. Affinity management can be added when a multi-node CDX host
> > +	 * exists to validate it against.
> > +	 */
> 
The support for reset_prepare/reset_done will be added in future.

> [Severity: Low]
> Is the reason given in this comment accurate? The PCI transport does more
> than spread over the local node. mana_irq_setup_numa_aware() skips SMT
> siblings and puts each vector on a different physical core:
> 
> 	cpumask_andnot(cpus, cpus, topology_sibling_cpumask(cpu));
> 	...
> 	irq_set_affinity_and_hint(*irqs++, topology_sibling_cpumask(cpu));
> 
> mana_irq_setup_linear() also pins vector i to CPU i.
> 
> A single-node multi-core SoC still has cores to spread over. On CDX, the
> EQ interrupts keep the default affinity and get no per-queue CPU
> placement.
> 
> Could the comment, and the commit message, describe this difference from
> PCI accurately?
> 
> [ ... ]
> 
> > +static int mana_cdx_setup_remaining_irqs(struct gdma_context *gc)
> > +{
> > +	unsigned int max_queues_vport;
> > +
> > +	/* mana_gd_query_max_resources() may turn MSI sharing back on and round
> > +	 * the per-vPort queue count up to MANA_DEF_NUM_QUEUES. CDX sizes its
> > +	 * vector pool at probe time and cannot grow it later, and the HWC
> > +	 * vector must stay private, so give every vPort a private slice of the
> > +	 * remaining vectors instead. gc->max_num_queues is already capped at
> > +	 * num_msix_usable - 1 by the core.
> > +	 */
> > +	gc->msi_sharing = false;
> > +	max_queues_vport = (gc->num_msix_usable - 1) / gc->num_ports;
> 
> [ ... ]
> 
> > +	gc->max_num_queues_vport = min(gc->max_num_queues, max_queues_vport);
> 
> [Severity: Low]
> This isn't a bug, but after this patch the comment in
> mana_gd_query_max_resources() in gdma_main.c is no longer accurate:
> 
> 	 * not before. Only reset on platforms that support dynamic
> 	 * MSI-X allocation; on non-dyn platforms msi_sharing is
> 	 * unconditionally true (set in mana_gd_setup_hwc_irqs).
> 
> CDX leaves msix_can_alloc_dyn NULL, so it is a non-dyn bus. Even so,
> mana_cdx_setup_hwc_irqs() sets msi_sharing to false. The core may set it
> back to true, and then mana_cdx_setup_remaining_irqs() forces it to false
> again and overwrites max_num_queues_vport.
> 
> Should the core comment be updated? It may also help to document in the
> core that a transport is expected to override msi_sharing and
> max_num_queues_vport in setup_remaining_irqs.
> 
> [ ... ]
> 
> > +	/* gc->is_pf is intentionally left clear: this device uses the VF
> > +	 * register layout, and the vPort and MAC filter registration that
> > +	 * is_pf selects is not required on this bus.
> > +	 */
> > +
> > +	err = mana_gd_setup(gc);
> > +	if (err)
> > +		goto unmap_bar;
> 
> [Severity: High]
> This issue was not introduced by this patch, but the shared HWC bootstrap
> path that CDX now enters here uses a doorbell index supplied by the device
> without checking its bounds.
> 
> mana_hwc_init_event_handler() in hw_channel.c stores the 16-bit doorbell
> directly from the EQE:
> 
> 	case GDMA_EQE_HWC_INIT_EQ_ID_DB:
> 		eq_db.as_uint32 = event->details[0];
> 		hwc->cq->gdma_eq->id = eq_db.eq_id;
> 		gd->doorbell = eq_db.doorbell;
> 
> mana_hwc_establish_channel() checks cq->id against max_num_cqs, but it
> never checks the doorbell. mana_gd_ring_doorbell() then computes the MMIO
> address:
> 
> 	void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index;
> 
> db_page_size is at least 4KB and the doorbell can be as large as 0xFFFF.
> Could the next mana_gd_ring_eq() or mana_gd_ring_cq() then write far past
> the ioremapped BAR0, into unmapped vmalloc space or another mapping?
> 
> mana_gd_register_device() already has this check for the regular
> doorbell:
> 
> 	if (gc->db_page_off + gc->db_page_size * ((u64)resp.db_id + 1) > gc->bar0_size) {
> 
> Should the HWC path apply the same check? This affects the PCI transport
> as well.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924173054.589291-1-mawasthi%40linux.microsoft.com