Re: [PATCH net v6 1/2] octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

This consolidates the per-file CONFIG_DCB ifdef blocks that pick the BPID
index for CQ and aura contexts into a new otx2_get_bpid_idx() helper in
otx2_common.h, and drops the equivalent cn20k_aura_bpid_idx().

>     Previously, BPID assignment under CONFIG_DCB assumed
>     `queue_to_pfc_map` was always initialized. For SDP VFs this leads to
>     invalid memory access as it was not initialized.
>
>     This patch adds a NULL check for `queue_to_pfc_map` before
>     dereferencing it. Also, simplifies the logic by always assigning a
>     default BPID first, then conditionally overriding it if CONFIG_DCB is
>     enabled and the map exists.

[Severity: High]
Does the patch match this description?  I do not see a NULL check for
queue_to_pfc_map anywhere in the diff, and no place where a default BPID is
assigned first and then conditionally overridden.  The new helper appears to
dereference the map unconditionally when CONFIG_DCB=y, which looks
byte-for-byte equivalent to the ifdef blocks it replaces.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index eecee612b7b2c..2a63d22cbd8d0 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> @@ -1029,6 +1029,14 @@ static inline int otx2_tc_flower_rule_cnt(struct otx2_nic *pfvf)
>  	return pfvf->flow_cfg->nr_flows;
>  }
>  
> +static inline u8 otx2_get_bpid_idx(struct otx2_nic *pfvf, int qidx)
> +{
> +#ifdef CONFIG_DCB
> +	return pfvf->queue_to_pfc_map[qidx];
> +#endif
> +	return 0;
> +}
> +

[Severity: High]
With CONFIG_DCB=y, isn't the trailing return 0 dead code here, leaving
pfvf->queue_to_pfc_map[qidx] as an unguarded dereference?

queue_to_pfc_map is allocated only in otx2_dcbnl_set_ops():

drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c:otx2_dcbnl_set_ops() {
	pfvf->queue_to_pfc_map = devm_kzalloc(pfvf->dev, pfvf->hw.rx_queues,
					      GFP_KERNEL);
}

and since the Fixes: commit, otx2vf_probe() skips that call for SDP and LBK
VFs:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c:otx2vf_probe() {
#ifdef CONFIG_DCB
	/* Priority flow control is not supported for LBK and SDP vf(s) */
	if (!(is_otx2_lbkvf(vf->pdev) || is_otx2_sdp_rep(vf->pdev))) {
		err = otx2_dcbnl_set_ops(netdev);
#endif
}

so for an SDP VF the pointer stays NULL for the lifetime of the device.

Would something like this be what was intended?

	static inline u8 otx2_get_bpid_idx(struct otx2_nic *pfvf, int qidx)
	{
	#ifdef CONFIG_DCB
		if (pfvf->queue_to_pfc_map)
			return pfvf->queue_to_pfc_map[qidx];
	#endif
		return 0;
	}

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 175992188c18a..0f3f405d9f1b7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

[ ... ]

> @@ -1132,11 +1133,8 @@ int otx2_cq_init(struct otx2_nic *pfvf, u16 qidx)
>  		if (!is_otx2_lbkvf(pfvf->pdev)) {
>  			/* Enable receive CQ backpressure */
>  			aq->cq.bp_ena = 1;
> -#ifdef CONFIG_DCB
> -			aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
> -#else
> -			aq->cq.bpid = pfvf->bpid[0];
> -#endif
> +			bpid_idx = otx2_get_bpid_idx(pfvf, qidx);
> +			aq->cq.bpid = pfvf->bpid[bpid_idx];

[Severity: High]
Does this call site still oops on an SDP VF with CONFIG_DCB=y?  The only
guard here is !is_otx2_lbkvf(), which matches PCI_DEVID_OCTEONTX2_RVU_AFVF
and PCI_DEVID_RVU_REP but not PCI_DEVID_OCTEONTX2_SDP_REP:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h {
	static inline bool is_otx2_lbkvf(struct pci_dev *pdev)
	{
		return (pdev->device == PCI_DEVID_OCTEONTX2_RVU_AFVF) ||
			(pdev->device == PCI_DEVID_RVU_REP);
	}
}

so interface bring up on an SDP VF reaches otx2_get_bpid_idx() with a NULL
map:

ndo_open -> otx2_init_hw_resources() -> otx2_cq_init() ->
otx2_get_bpid_idx()

The same holds for the other two converted sites, which carry identical
guards:

otx2_init_hw_resources() -> otx2_rq_aura_pool_init() ->
otx2_aura_aq_init() -> otx2_get_bpid_idx()

and cn20k_aura_aq_init() -> otx2_get_bpid_idx(), where the removed
cn20k_aura_bpid_idx() had the same shape.

Given the Fixes: tag will pull this into stable, could the helper be
respun with the guard the changelog describes, so that SDP VFs fall back to
pfvf->bpid[0]?

[ ... ]
-- 
pw-bot: cr