Re: [PATCH v33 net-next 3/8] net/nebula-matrix: add intr resource implementation

[netdev-bot+sashiko@kernel.org]

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

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

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 commit message and an in-code comment describe the
  reconfiguration path the opposite way from how the code behaves.

--

net/nebula-matrix: add intr resource implementation

This adds an nbl_interrupt module that manages the chip-internal MSI-X
vector index space for the control PF. It also programs the
FUNCTION_MSIX_MAP, HOST_MSIX_INFO and MSIX_FID tables, using a
clear-VALID, flush, delay sequence for teardown. The patch also moves
pci_set_master() to after nbl_core_init() and clears bus mastering
before teardown in nbl_remove().

> 1. cfg_msix_map: Allocate global hardware MSI-X vectors from the separate
>    net/other interrupt bitmaps.  Reuse the per-function coherent DMA map
>    table on reconfig, so a reconfiguration neither frees and reallocates
>    the table nor has to take the destroy-path quiesce window before the
>    old vectors are recycled.  The first configuration of a function still
>    takes one invalidate + flush + quiesce window before the table is
>    written; that window belongs to the first configuration only and is
>    not part of the reconfiguration path.

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c
> new file mode 100644
> index 0000000000000..3969392abdc64
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c
> @@ -0,0 +1,851 @@

[ ... ]

> +/**
> + * nbl_res_intr_cfg_msix_map - allocate & program MSI-X mapping table

[ ... ]

> + * Allocate interrupt vectors; MSIX coherent DMA table is allocated once
> + * per function on first configuration, entries are rewritten while the
> + * map is invalidated on subsequent reconfigurations. No free/realloc of
> + * DMA table on vector count changes. This removes the DMA table
> + * free/realloc cycle. On reconfiguration the map VALID bit is cleared
> + * and the hardware-DMA quiesce window is observed (lock held) before old
> + * vectors are recycled and the table is rewritten.

[ ... ]

> +	if (had_config) {
> +		old_interrupts =
> +			intr_mgt->func_intr_res[func_id].interrupts;
> +		old_num = intr_mgt->func_intr_res[func_id].num_interrupts;
> +
> +		ret = __nbl_res_intr_set_mailbox_irq(intr_mgt, res_mgt, func_id,
> +						     0, false);

[ ... ]

> +		for (j = 0; j < old_num; j++) {
> +			hw_ops->cfg_msix_info(res_mgt->hw_ops_tbl->priv,
> +					      func_id, false,
> +					      old_interrupts[j],
> +					      0, 0, 0, false);
> +		}
> +		hw_ops->cfg_msix_map(res_mgt->hw_ops_tbl->priv, func_id,
> +				     false, official_tbl->dma,
> +				     0, 0, 0);
> +		hw_ops->flush_write(res_mgt->hw_ops_tbl->priv);
> +
> +		nbl_intr_quiesce_wait();
> +
> +		nbl_intr_release_bitmap(intr_mgt, res_mgt, old_interrupts,
> +					old_num);

[ ... ]

> +	/*
> +	 * First configuration of this function.  Re-invalidate the entry
> +	 * and take a quiesce window before the table below is rewritten:
> +	 * while the entry is VALID the pcompleter could fetch a table
> +	 * that is being written and route stale gvecs.
> +	 *
> +	 * This is a second, real quiesce window rather than part of the
> +	 * reconfiguration path, and it costs one delay per function on the
> +	 * first configuration only.  Reusing the table removes the
> +	 * free/realloc cycle and the destroy-path quiesce window from
> +	 * every *re*configuration; it does not remove this one.
> +	 */
> +	if (!had_config) {
> +		hw_ops->cfg_msix_map(res_mgt->hw_ops_tbl->priv, func_id,
> +				     false, 0, 0, 0, 0);
> +		hw_ops->flush_write(res_mgt->hw_ops_tbl->priv);
> +		nbl_intr_quiesce_wait();
> +	}

[Severity: Medium]
The commit message says a reconfiguration does not "take the
destroy-path quiesce window before the old vectors are recycled". The
comment above the !had_config block in nbl_res_intr_cfg_msix_map() says
the same thing.

The had_config branch above seems to do the opposite, though. It runs the
destroy sequence:

  __nbl_res_intr_set_mailbox_irq(..., false)
  cfg_msix_info(false) for each old gvec
  cfg_msix_map(false, official_tbl->dma)
  flush_write()

It then calls nbl_intr_quiesce_wait(), the same usleep_range(1000, 1200)
the destroy path uses. Only after that does it call
nbl_intr_release_bitmap() on the old vectors.

The kerneldoc for nbl_res_intr_cfg_msix_map() matches the code: "On
reconfiguration the map VALID bit is cleared and the hardware-DMA quiesce
window is observed (lock held) before old vectors are recycled". So the
two in-file comments contradict each other. The only quiesce a
reconfiguration skips is the extra first-configuration one.

Could the commit message and the !had_config comment be updated to match
the code?

The !had_config comment also justifies its extra invalidate and quiesce
with "while the entry is VALID the pcompleter could fetch a table that is
being written". Can the entry be VALID when had_config is false?

- nbl_intr_setup_mgt() clears VALID on all NBL_MAX_PF entries at start.
- A failed fresh configuration exits before any hardware write.
- __nbl_res_intr_complete_destroy_msix_map() leaves VALID cleared.
- DESTROYING can't be seen here, because the single-function destroy
  holds the lock and nbl_intr_mgt_stop() sets stopping.

Separately, the commit message says:

  Everything between pci_set_master() and the scrub is host-side setup
  plus MMIO and does not wait for the device.

This patch moves pci_set_master() in nbl_probe() to after
nbl_core_init(), so it now runs after the scrub in nbl_intr_setup_mgt().
Does this sentence describe the old ordering?

nbl_interrupt.c is not changed by the later patches in the series, so
this mismatch is still there at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106045631.2224-1-illusion.wang%40nebula-matrix.com