Re: [PATCH v10 net-next 2/2] 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] otx2_nix_tm_set_queue_shaper and otx2_nix_tm_clear_queue_shaper set the mailbox level to NIX_TXSCH_LVL_MDQ while using SMQ indices, leading to guaranteed AF driver validation failures.
- [High] A failure during initial mqprio setup bypasses hardware error cleanup, leaving the device in an inconsistent state.
- [High] A successful `tc qdisc replace` with an identical handle is mistakenly identified as a failure rollback, reverting the hardware configuration.
- [Medium] Failed mqprio replace permanently disables bandwidth offload because the rollback path fails to restore `pfvf->mqprio.rate_limit`.
- [Medium] Memory allocation failure during qdisc deletion aborts teardown, leaking active hardware shapers.
--

commit 3ab815bb73b8a877ec97fa639742f0ab5d81b757
Author: Ratheesh Kannoth <rkannoth@marvell.com>

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

This commit adds TC_SETUP_QDISC_MQPRIO handling for channel-mode mqprio with
TC_MQPRIO_SHAPER_BW_RATE. It enables shaping each non-QoS transmit queue by
programming MDQ CIR/PIR on hw.non_qos_queues.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -615,6 +615,142 @@ void otx2_get_mac_from_af(struct net_device *netdev)
>  }
>  EXPORT_SYMBOL(otx2_get_mac_from_af);
>  
> +static int
> +otx2_nix_tmq_reg_write(struct otx2_nic *pfvf, int cnt,
> +		       u64 reg_addr[MAX_REGS_PER_MBOX_MSG],
> +		       u64 reg_val[MAX_REGS_PER_MBOX_MSG])
> +{
> +	struct mbox *mbox = &pfvf->mbox;
> +	struct nix_txschq_config *req;
> +	int i, err;
> +
> +	mutex_lock(&mbox->lock);
> +	req = otx2_mbox_alloc_msg_nix_txschq_cfg(mbox);
> +	if (!req) {
> +		mutex_unlock(&mbox->lock);
> +		return -ENOMEM;
> +	}
> +
> +	req->lvl = NIX_TXSCH_LVL_MDQ;

[Severity: High]
Will this cause the AF driver to reject the configuration mailbox messages?

In otx2_nix_tm_set_queue_shaper(), the MDQ register addresses are computed
using the SMQ index. The AF driver's validation function extracts the
index from the register address and validates it against the bounds of the
NIX_TXSCH_LVL_MDQ pool, which has a different size than the SMQ pool.

[ ... ]
> +int otx2_nix_tm_set_queue_shaper(struct otx2_nic *pfvf,
> +				 int txq, u64 minrate, u64 maxrate)
> +{
[ ... ]
> +	req->lvl = NIX_TXSCH_LVL_MDQ;

[Severity: High]
Similar to the concern above, setting the mailbox level to NIX_TXSCH_LVL_MDQ
inline here while relying on SMQ indices appears to guarantee AF driver
validation failures during any qdisc add operation.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
[ ... ]
> +static int otx2_mqprio_restore_old(struct otx2_nic *pfvf)
> +{
[ ... ]
> +	if (otx2_mqprio_mdq_allocated(pfvf)) {
> +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = otx2_mqprio_up(pfvf);
> +	if (err)
> +		return err;

[Severity: Medium]
Does this rollback path permanently disable bandwidth offload?

If an ndo_open() failure occurs during tc qdisc replace,
otx2_mqprio_restart_netdev() explicitly clears pfvf->mqprio.rate_limit to
false. Here in otx2_mqprio_restore_old(), the rate_limit flag is not set
back to true before calling otx2_mqprio_up(). As a result, otx2_mqprio_up()
will exit early without configuring the hardware shapers.

[ ... ]
> +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);
> +	bool rollback;
> +	int err;
> +
> +	qopt->hw = 0;
> +
> +	/* tc qdisc replace runs setup on the new mqprio before destroying the
> +	 * old one. Ignore teardown from the replaced instance once the new
> +	 * handle is committed in otx2_setup_tc_mqprio().
> +	 */
> +	if (mqprio->handle && mqprio->handle != pfvf->mqprio.handle) {
> +		if (pfvf->cur_mq_snap)
> +			otx2_mqprio_snap_commit(pfvf);
> +		return 0;
> +	}

[Severity: High]
If a fresh qdisc add fails halfway through (for example, during
otx2_nix_tm_set_queue_shaper), could this bypass hardware cleanup?

During a failed initial setup, pfvf->mqprio.handle would still be 0, while
the incoming mqprio->handle from the generic TC layer is non-zero. This
condition would evaluate to true, treating the failure as a post-replace
teardown and returning 0 without performing the actual cleanup.

> +
> +	if (mqprio->handle && mqprio->handle == pfvf->mqprio.handle &&
> +	    pfvf->old_mq_snap) {
> +		rollback = pfvf->cur_mq_snap;
> +
> +		err = otx2_mqprio_restore_old(pfvf);
> +		if (err)
> +			return err;

[Severity: High]
Could this inadvertently trigger a rollback during a successful tc qdisc
replace? 

If the user issues a replace command utilizing the exact same handle as the
existing qdisc, this condition evaluates to true and appears to call
otx2_mqprio_restore_old(), overriding the newly installed configuration with
the old one.

[Severity: Medium]
If memory allocation fails during a qdisc deletion, will this leak the active
hardware shapers?

When deleting a qdisc, the code falls into this block because the handles
match and old_mq_snap exists. It then calls otx2_mqprio_restore_old(), which
uses devm_kcalloc(). If that allocation fails, the function returns early
here, completely bypassing the actual hardware teardown in otx2_mqprio_down().

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