Re: [net-next,v2] octeontx2-pf: Add NIXLF error and poison interrupt handlers

[netdev-bot+sashiko]

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

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

- [Medium] The patch bundles undisclosed VF-side changes that the commit
  message (subject "octeontx2-pf: Add NIXLF error and poison interrupt…
- [Low] The rewritten comment in otx2_realloc_msix_vectors()
  (otx2_pf.c:3086-3090) claims the code "Skip[s] NPA vectors", but the…

--

octeontx2-pf: Add NIXLF error and poison interrupt handlers

This registers handlers for the NIX LF error (NIX_LF_ERR_INT) and poison/RAS
(NIX_LF_RAS) MSI-X vectors in otx2_open(), frees them in otx2_stop(), and
widens the MSI-X reservation in otx2_realloc_msix_vectors() and
otx2vf_realloc_msix_vectors() to cover the new vectors.

A couple of questions about the changes below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index c995f29008590..1f8cb78c19195 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c

[ ... ]

> @@ -3006,11 +3084,15 @@ int otx2_realloc_msix_vectors(struct otx2_nic *pf)
>  	struct otx2_hw *hw = &pf->hw;
>  	int num_vec, err;
>  
> -	/* NPA interrupts are inot registered, so alloc only
> -	 * upto NIX vector offset.
> +	/* Skip NPA vectors. Representors only use CINT vectors, so limit
> +	 * the budget to that range. For PF/VF, allocate the full NIX LF
> +	 * interrupt range (QINT, CINT, GINT, ERR and POISON vectors).
>  	 */

[Severity: Low]
Does the new comment in otx2_realloc_msix_vectors() describe what the code
does?  The count starts at hw->nix_msixoff and the allocation is:

	num_vec = hw->nix_msixoff;
	...
	err = pci_alloc_irq_vectors(hw->pdev, num_vec, num_vec, PCI_IRQ_MSIX);

pci_alloc_irq_vectors() enables the contiguous range starting at vector 0,
which includes everything below hw->nix_msixoff, and that is where the NPA
LF's vectors live.  rvu_mbox_handler_attach_resources() attaches BLKTYPE_NPA
before BLKTYPE_NIX, and rvu_set_msix_offset() hands out each LF's vectors
with rvu_alloc_rsrc_contig() from the function's MSI-X bitmap, so
npa_msixoff < nix_msixoff.

The NPA vectors are therefore still allocated, just left without handlers,
which is what the replaced comment said ("NPA interrupts are [not]
registered, so alloc only upto NIX vector offset").  Would it be clearer to
keep the original wording rather than saying the vectors are skipped?

>  	num_vec = hw->nix_msixoff;
> -	num_vec += NIX_LF_CINT_VEC_START + hw->max_queues;
> +	if (pf->flags & OTX2_FLAG_REP_MODE_ENABLED)
> +		num_vec += NIX_LF_CINT_VEC_START + hw->max_queues;
> +	else
> +		num_vec += NIX_LF_POISON_VEC + 1;
>  
>  	otx2_disable_mbox_intr(pf);
>  	pci_free_irq_vectors(hw->pdev);

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> index fcdf891f90b58..3b06da52d37e5 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> @@ -554,7 +554,7 @@ static int otx2vf_realloc_msix_vectors(struct otx2_nic *vf)
>  	int num_vec, err;
>  
>  	num_vec = hw->nix_msixoff;
> -	num_vec += NIX_LF_CINT_VEC_START + hw->max_queues;
> +	num_vec += NIX_LF_POISON_VEC + 1;
>  
>  	otx2vf_disable_mbox_intr(vf);
>  	pci_free_irq_vectors(hw->pdev);
> @@ -597,7 +597,7 @@ static int otx2vf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	pci_set_master(pdev);
>  
> -	qcount = num_online_cpus();
> +	qcount = min_t(int, num_online_cpus(), OTX2_MAX_CQ_CNT);

[Severity: Medium]
Should this queue count clamp in otx2vf_probe() be a separate patch, or at
least be described in the commit message?

The subject and body only talk about registering the two new NIX LF handlers,
but this line changes the VF datapath configuration: on a system with more
than 64 online CPUs every octeontx2 VF drops from num_online_cpus() rx/tx
queues to 64, since hw->rx_queues, hw->tx_queues, hw->max_queues,
hw->non_qos_queues and the alloc_etherdev_mqs() queue counts are all derived
from qcount.

It also looks like a fix for a pre-existing asymmetry rather than a
requirement of the new handlers.  otx2_probe() already had the same clamp:

	qcount = min_t(int, num_online_cpus(), OTX2_MAX_CQ_CNT);

while the VF did not, so with more than 64 CPUs the VF's cint_cnt (max3 of
rx_queues, tx_queues and tc_tx_queues) exceeded the 64 CINT slots available
between NIX_LF_CINT_VEC_START (0x40) and NIX_LF_GINT_VEC (0x80) in
otx2_common.h, and the CINT vector indices ran into GINT/ERR/POISON or past
the LF's vector count.

Would a standalone patch carrying

	Fixes: 3184fb5ba96e ("octeontx2-vf: Virtual function driver support")

be more appropriate for the clamp?  The "Changes in v2:" note that mentions
it sits below the --- separator and is dropped when the patch is applied, so
the recorded history contains no trace of this change.

The same applies to the otx2vf_realloc_msix_vectors() hunk above: the VF
reservation moves from the queue-count-scaled NIX_LF_CINT_VEC_START +
hw->max_queues to the fixed NIX_LF_POISON_VEC + 1, and VFs pick up the new
handlers only because otx2vf_open()/otx2vf_stop() call into the shared
otx2_open()/otx2_stop().  Should the commit message mention that this touches
VFs too, given the "octeontx2-pf:" subject prefix?

>  	qos_txqs = min_t(int, qcount, OTX2_QOS_MAX_LEAF_NODES);
>  	netdev = alloc_etherdev_mqs(sizeof(*vf), qcount + qos_txqs, qcount);
>  	if (!netdev)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831062600.839203-1-nshettyj%40marvell.com