Re: [PATCH v11 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 3 potential issue(s) to consider:
- [High] Rolling back mqprio configuration after an ndo_open() failure programs hardware shapers using invalid/freed SMQ indices (SMQ 0).
- [Medium] Early return in otx2_teardown_tc_mqprio() on restore failure leaves stale replace state, hijacking future qdisc operations.
- [Medium] Hardware failure during mqprio teardown abandons software state, causing permanent desynchronization and feature denial.
--

commit 6413eab5a10115c63a46dacd6bbe4921e9bd1678
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers

This commit adds TC_SETUP_QDISC_MQPRIO offload for channel-mode mqprio
with TC_MQPRIO_SHAPER_BW_RATE. It rebuilds the TX scheduler hierarchy
by bouncing the netdev on mqprio add, replace, and delete, and includes
rollback logic using snapshots to recover from setup failures.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 039fd47ebf522..5efa0b4d27d53 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
[ ... ]
> +static bool otx2_mqprio_mdq_allocated(struct otx2_nic *pfvf)
> +{
> +	return pfvf->hw.txschq_cnt[NIX_TXSCH_LVL_MDQ] != 0;
> +}
> +
> +static int otx2_mqprio_restore_old(struct otx2_nic *pfvf)
> +{
> +	struct mq_offload_snap *snap = pfvf->old_mq_snap;
> +	struct net_device *netdev = pfvf->netdev;
> +	u16 num_txq = pfvf->hw.non_qos_queues;
> +	int tc, txq, err;
[ ... ]
> +	if (otx2_mqprio_mdq_allocated(pfvf)) {
> +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> +		if (err)
> +			return err;
> +	}

[Severity: High]
When ndo_open() fails and we fall back to otx2_mqprio_restore_old(), will
otx2_mqprio_mdq_allocated() falsely return true here?

It looks like otx2_txschq_stop() frees schedulers and zeroes txschq_list,
but does not zero txschq_cnt. If that happens,
otx2_nix_tm_clear_queue_shaper() will iterate over the non-zero count,
reading smq = 0 from the zeroed txschq_list, and send an invalid mailbox
command for SMQ 0. This could abort the configuration rollback or corrupt
hardware state.

[ ... ]
> +int otx2_mqprio_down(struct otx2_nic *pfvf)
> +{
> +	int err = 0;
> +
> +	if (!pfvf->mqprio.rate_limit)
> +		return 0;
> +
> +	if (netif_running(pfvf->netdev) &&
> +	    otx2_mqprio_mdq_allocated(pfvf))
> +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> +
> +	if (err) {
> +		netdev_err(pfvf->netdev,
> +			   "mqprio: failed to clear hardware shapers: %d; some TX queues may retain bandwidth limits\n",
> +			   err);
> +		return err;
> +	}

[Severity: Medium]
If clearing the hardware shapers fails (e.g. due to a transient mailbox
timeout), this early return skips the call to otx2_mqprio_clear_sw(pfvf).

Does this abandon the software state and cause a permanent desynchronization?
The driver will retain pfvf->mqprio.rate_limit = true while the kernel has
already deleted the qdisc, permanently blocking features like XDP or PFC.

[ ... ]
> +static int otx2_teardown_tc_mqprio(struct otx2_nic *pfvf,
> +				   struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	bool had_mqprio = pfvf->mqprio.rate_limit;
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	struct net_device *netdev = pfvf->netdev;
> +	bool if_up = netif_running(netdev);
> +	int err;
> +
> +	qopt->hw = 0;
> +
> +	/* tc qdisc replace runs setup on the new mqprio before destroying the
> +	 * old one. replace_setup_done and TC_ROOT_GRAFT distinguish stale
> +	 * old-instance teardown from graft failure after setup.
> +	 */
> +	if (pfvf->mqprio.replace_setup_done && pfvf->cur_mq_snap) {
> +		if (pfvf->mqprio.replace_graft_done) {
> +			otx2_mqprio_snap_commit(pfvf);
> +		} else {
> +			err = otx2_mqprio_restore_old(pfvf);
> +			if (err)
> +				return err;

[Severity: Medium]
If otx2_mqprio_restore_old() returns an error (such as the SMQ 0 failure
noted above), this early return skips clearing the replace state flags
via otx2_mqprio_clear_replace_state().

Could this leave a stale replace state that permanently hijacks future
qdisc operations and prevents the offload from ever being cleanly torn
down?

> +		}
> +		otx2_mqprio_clear_replace_state(pfvf);
> +		return 0;
> +	}
> +
> +	/* Skip the netdev restart when mqprio offload was not active. */
> +	if (!had_mqprio)
> +		return 0;
> +
> +	if (if_up) {
> +		int down_err, err;
> +
> +		down_err = otx2_mqprio_down(pfvf);
> +		err = otx2_mqprio_restart_netdev(netdev, false);
> +		if (err)
> +			return err;
> +		return down_err;
> +	}

[Severity: Medium]
Following up on the issue in otx2_mqprio_down() above, if that function
fails and doesn't clear the software state, we then call
otx2_mqprio_restart_netdev(netdev, false).

Since the rate_limit flag is still true in the uncleared software state,
won't this netdev restart accidentally restore the hardware offload that
was just torn down by the kernel?

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