Re: [PATCH v20 net-next 06/10] net/nebula-matrix: add intr resource implementation

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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://sashiko.dev/
---
net/nebula-matrix: add intr resource implementation

This patch adds the nbl_interrupt module for management of driver-private
MSI-X indices. It adds hw_ops callbacks to program the function MSI-X mapping
registers, configure MSI-X info, and enable mailbox interrupts.

> 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..53a657aef9ca0
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c
> @@ -0,0 +1,267 @@
[ ... ]
> +/*
> + * This function tears down the MSI-X mapping for a function by clearing the
> + * interrupt bitmaps, disabling MSI-X via HW callbacks, and resetting the
> + * bookkeeping state. The DMA buffer and interrupts[] array are intentionally
> + * left allocated for potential reuse on reset/hot-add paths. Caller must
> + * ensure this is only invoked when the HW cannot issue in-flight DMA to
> + * the region (IRQs already freed or not yet configured).
> + */
> +int nbl_res_intr_destroy_msix_map(struct nbl_resource_mgt *res_mgt,
> +				  u16 func_id)
> +{
[ ... ]
> +	/* use ctrl dev bdf */
> +	hw_ops->configure_msix_map(res_mgt->hw_ops_tbl->priv, func_id, false, 0,
> +				   0, 0, 0);
> +
> +	intr_mgt->func_intr_res[func_id].interrupts = NULL;
> +	intr_mgt->func_intr_res[func_id].num_interrupts = 0;
> +	intr_mgt->func_intr_res[func_id].msix_map_table.base_addr = NULL;
> +	intr_mgt->func_intr_res[func_id].msix_map_table.size = 0;
> +	hw_ops->flush_write(res_mgt->hw_ops_tbl->priv);
> +
> +	return 0;
> +}

[Severity: High]
Does this code leak the devm-managed resources? The comment mentions that
the DMA buffer and interrupts array are left allocated for potential reuse,
but the pointers are set to NULL here. This causes the pointers to be lost
so they cannot be reused or explicitly freed later.

Because nbl_res_intr_configure_msix_map() unconditionally allocates new devres
memory on every call, won't this cause the devres list to accumulate leaked
allocations (dmam_alloc_coherent and devm_kcalloc) each time the device is
reset or a VF is reconfigured?

[ ... ]
> +int nbl_res_intr_configure_msix_map(struct nbl_resource_mgt *res_mgt,
> +				    u16 func_id, u16 num_net_msix,
> +				    u16 num_others_msix,
> +				    bool net_msix_mask_en)
> +{
[ ... ]
> +	nbl_res_intr_destroy_msix_map(res_mgt, func_id);
> +
> +	msix_map_table = &intr_mgt->func_intr_res[func_id].msix_map_table;
> +	WARN_ON(msix_map_table->base_addr);
> +	msix_map_table->size =
> +		sizeof(struct nbl_msix_map) * NBL_MSIX_MAP_TABLE_MAX_ENTRIES;
> +	/* HW indexes the MSI-X map table with a fixed stride of 1024 entries,
> +	 * so we must allocate the full table even if fewer vectors are used.
> +	 */
> +	msix_map_table->base_addr = dmam_alloc_coherent(dev,
> +							msix_map_table->size,
> +							&msix_map_table->dma,
> +							GFP_KERNEL);
> +	if (!msix_map_table->base_addr) {
> +		dev_err(dev, "Allocate DMA memory for function msix map table failed\n");
> +		msix_map_table->size = 0;
> +		return -ENOMEM;
> +	}
> +
> +	interrupts = devm_kcalloc(dev, requested, sizeof(interrupts[0]),
> +				  GFP_KERNEL);
> +	if (!interrupts) {
> +		ret = -ENOMEM;
> +		goto alloc_interrupts_err;
> +	}

[Severity: High]
If devm_kcalloc() fails, the goto alloc_interrupts_err path bypasses
clearing msix_map_table->base_addr and size. 

While nbl_res_intr_destroy_msix_map() clears it unconditionally on the next
call, doesn't this leave the internal state inconsistent by keeping base_addr
populated when interrupts is NULL?

[ ... ]
> +get_interrupt_err:
> +	while (i--) {
> +		intr_index = interrupts[i];
> +		if (intr_index >= NBL_NET_INTR_BASE)
> +			clear_bit(intr_index - NBL_NET_INTR_BASE,
> +				  intr_mgt->interrupt_net_bitmap);
> +		else
> +			clear_bit(intr_index,
> +				  intr_mgt->interrupt_others_bitmap);
> +	}
> +
> +	intr_mgt->func_intr_res[func_id].num_interrupts = 0;
> +	intr_mgt->func_intr_res[func_id].interrupts = NULL;
> +	intr_mgt->func_intr_res[func_id].num_net_interrupts = 0;
> +	intr_mgt->func_intr_res[func_id].msix_map_table.base_addr = NULL;
> +	intr_mgt->func_intr_res[func_id].msix_map_table.size = 0;
> +alloc_interrupts_err:
> +	return ret;
> +}
-- 
This is an AI-generated review.