Re: [PATCH v20 net-next 2/2] octeontx2: add mqprio bandwidth offload for NIX TX schedulers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net/octeontx2: incomplete mqprio teardown on shaper clear failure
--

commit eaf43a97d16b9d31a19d30a97316304bd2d66c74
Author: Ratheesh Kannoth <rkannoth@marvell.com>
Subject: octeontx2: add mqprio bandwidth offload for NIX TX schedulers

This commit adds TC_SETUP_QDISC_MQPRIO offload with TC_MQPRIO_SHAPER_BW_RATE
on PF and VF netdevs. It programs per-queue MDQ CIR/PIR via the NIX TX
scheduler mailbox and handles live reprogramming by bouncing the netdev.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 8877af348a090..b34a86259e786 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
[ ... ]
> @@ -1886,6 +1886,22 @@ static void otx2_mqprio_clear_sw(struct otx2_nic *pfvf)
> +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);
> +
> +	/* The core cannot veto qdisc teardown, so always release software
> +	 * state.  The netdev restart that follows reallocates the TX
> +	 * scheduler, which drops any shaper left programmed.
> +	 */
> +	if (err)
> +		netdev_warn(pfvf->netdev,
> +			    "mqprio: failed to clear hardware shapers: %d; releasing offload state\n",
> +			    err);
> +
> +	otx2_mqprio_clear_sw(pfvf);
> +
> +	return err;
> +}
[ ... ]
> @@ -2012,6 +2013,17 @@ static int otx2_teardown_tc_mqprio(struct otx2_nic *pfvf,
> +	if (if_up) {
> +		err = otx2_mqprio_down(pfvf);
> +		if (err) {
> +			if (extack)
> +				NL_SET_ERR_MSG_FMT_MOD(extack,
> +						       "mqprio: teardown incomplete (%d)",
> +						       err);
> +			netdev_err(netdev,
> +				   "mqprio: offload teardown incomplete (%d); driver state may not match removed qdisc\n",
> +				   err);
> +			return err;

[Severity: Medium]
Will this early return on error bypass the required netdev restart?

If otx2_nix_tm_clear_queue_shaper() fails inside otx2_mqprio_down(), the
software state is cleared via otx2_mqprio_clear_sw(), but returning
here skips otx2_mqprio_restart_netdev(). 

This seems to leave the hardware and software states permanently
desynchronized. The hardware would continue applying mqprio topology and
bandwidth shaping while the software state considers it disabled. Since
the comment in otx2_mqprio_down() states that the netdev restart that
follows will reallocate the tx scheduler and drop any shaper left
programmed, skipping the restart appears to contradict the recovery
strategy.

Would it be safer to proceed with the netdev restart even if the hardware
shaper clearing fails?

> +		}
> +
> +		err = otx2_mqprio_restart_netdev(netdev, false);
> +		if (err) {

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