Re: [PATCH v8 net-next] octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] A failed `tc qdisc replace` operation destructively modifies the software rate cache and hardware shapers without rolling back, leaving the system in a corrupted, mismatched state.
- [High] Reusing the TL4[0] scheduler queue for MQPRIO after an HTB teardown inherits a stale topology configuration, permanently starving some TX queues.
- [High] The driver attempts to modify hardware queues that have already been freed when deleting the MQPRIO qdisc while the interface is down.
- [Medium] If clearing hardware shapers fails during MQPRIO teardown, the driver unconditionally clears its software state while the hardware silently retains the stale shapers.
- [Medium] A flawed allocation check silently skips clearing hardware shapers for single-queue configurations if HW index 0 is used.
--

--- Patch [1]: [PATCH v8 net-next] octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers ---
commit c00a385aa5cbe69512654ab461dd033c6039b7b7
Author: Ratheesh Kannoth <rkannoth@marvell.com>
Subject: octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers

This patch adds TC_SETUP_QDISC_MQPRIO handling to offload per-queue MDQ
CIR/PIR shaping for mqprio channel mode. It manages the setup and teardown
by restarting the netdev to rebuild the TX scheduler hierarchy. Various
checks and limits are also introduced.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index ca73a94db7943..b6d8ee7e2d4c2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

[ ... ]

> @@ -650,7 +786,20 @@ int otx2_txschq_config(struct otx2_nic *pfvf, int lvl, int prio, bool txschq_for
>  						(u64)hw->smq_link_type);
>  		req->num_regs++;
>  		/* MDQ config */
> -		parent = schq_list[NIX_TXSCH_LVL_TL4][prio];
> +		if (pfvf->mqprio.rate_limit) {
> +			/* TODO: Program NIX_AF_TL4X_TOPOLOGY on TL4[0]. MQPRIO
> +			 * parents every MDQ under the same TL4 with schedule
> +			 * priority 0, so sibling arbitration depends on the TL4
> +			 * topology register. Only the HTB path programs this
> +			 * today and the AF does not reset it on schq realloc,
> +			 * so a TL4 reused after HTB teardown can retain stale
> +			 * RR_PRIO/PRIO_ANCHOR and starve some TX queues.
> +			 */
> +			parent = schq_list[NIX_TXSCH_LVL_TL4][0];
> +		} else {
> +			parent = schq_list[NIX_TXSCH_LVL_TL4][prio];
> +		}
> +
>  		req->reg[1] = NIX_AF_MDQX_PARENT(schq);
>  		req->regval[1] = parent << 16;
>  		req->num_regs++;

[Severity: High]
Does reusing the TL4[0] scheduler queue for MQPRIO after an HTB teardown
inherit a stale topology configuration? By parenting all MDQs to TL4[0]
without explicitly reprogramming NIX_AF_TL4X_TOPOLOGY, this might permanently
starve some TX queues.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 039fd47ebf522..fc9677e340658 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c

[ ... ]

> +static bool otx2_txschq_is_allocated(struct otx2_nic *pfvf)
> +{
> +	int lvl, idx;
> +
> +	for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++) {
> +		for (idx = 0; idx < pfvf->hw.txschq_cnt[lvl]; idx++) {
> +			if (pfvf->hw.txschq_list[lvl][idx])
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}

[Severity: Medium]
Can this check silently skip clearing hardware shapers for single-queue
configurations? If the hardware assigned index 0 (which is a valid queue ID),
evaluating the queue ID as a boolean returns false, causing the single TX
queue to retain its stale PIR/CIR limits.

[Severity: High]
When deleting the MQPRIO qdisc while the interface is down, will this attempt
to modify hardware queues that have already been freed? The hardware
resources are freed on interface down, but hw.txschq_cnt and txschq_list
remain intact. This could cause the function to return true and trigger a
use-after-free of the hardware scheduler queues via mailbox commands.

[ ... ]

> +int otx2_mqprio_down(struct otx2_nic *pfvf)
> +{
> +	struct net_device *netdev = pfvf->netdev;
> +	int err = 0;
> +
> +	if (!pfvf->mqprio.rate_limit)
> +		return 0;
> +
> +	if (otx2_txschq_is_allocated(pfvf))
> +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> +
> +	/* TODO: mqprio_down() clears mqprio_rate_limit, netdev TC mappings,
> +	 * and the rate cache even when otx2_nix_tm_clear_queue_shaper() fails
> +	 * partway through the MDQ batch. Software then reports offload as
> +	 * inactive while some queues may retain programmed CIR/PIR, so a later
> +	 * mqprio_up() or re-setup can shape with the wrong effective rates.
> +	 */
> +	pfvf->mqprio.rate_limit = false;
> +	pfvf->mqprio_replace_pending = false;
> +	pfvf->mqprio_skip_teardown = false;
> +	netdev_set_num_tc(netdev, 0);
> +	otx2_mqprio_free_cache(pfvf);
> +
> +	return err;
> +}

[Severity: Medium]
If clearing the hardware shapers fails during MQPRIO teardown (for example,
due to a mailbox communication error in otx2_nix_tm_clear_queue_shaper),
does this unconditionally clear the software state? The hardware would
silently retain the stale shapers and continue to enforce bandwidth limits,
resulting in invisible throttling that is out of sync with the software
tracking flags.

[ ... ]

> +cleanup:
> +	/* Clear hardware offload on this rejected request so a failed replace
> +	 * does not leave tc reporting offload for driver state that was torn
> +	 * down while the previous mqprio remains grafted.
> +	 */
> +	qopt->hw = 0;
> +	if (replacing) {
> +		otx2_mqprio_replace_failed_msg(netdev, extack);
> +		pfvf->mqprio_replace_pending = false;
> +		return err ? err : -EIO;
> +	}

[Severity: High]
If a tc qdisc replace operation fails (for instance, out of memory in
otx2_mqprio_alloc_cache or a mailbox timeout), does the cleanup path
abandon the destructively modified rate cache and partially applied
hardware shapers? 

Since the software rate cache is wiped early in otx2_mqprio_alloc_cache,
returning the error here without rolling back the netdev TC mappings could
leave the active qdisc and driver state permanently mismatched.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813082754.1165329-1-rkannoth@marvell.com?part=1