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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 7 (1) · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When hw-tc-offload is off, otx2_teardown_tc_mqprio() can
  commit a replace that the qdisc core rejected.
- [Medium] A failed shaper clear during mqprio delete leaves the hardware
  rate-limiting with no qdisc behind it.
- [Medium] otx2_mqprio_restore_old() returns early on errors, and neither
  caller repairs or quiesces the device afterwards.
- [Medium] If the restart inside a rollback fails, the committed offload
  state is lost for good.
- [Medium] The new err_free_hw unwind in otx2_open() leaks
  pf->refill_wrk.
- [Medium] otx2_mqprio_restart_netdev() hand-rolls a close/open:
  clear_bit(__LINK_STATE_START), then ops->ndo_stop()/ndo_open().
- [Medium] In mqprio mode, the scheduler topology for the shared TL4 is
  never programmed.
- [Low] After a failed replace, the netdev TC layout is restored only
  later, from a work item, so there is a window with num_tc == 0.
- [Low] The new comment in otx2_config_sched_shaping() (qos.c) says 65536
  is 'the kernel-side default burst when HTB does not supply an explicit…
- [Low] Two offload restrictions are missing from the commit message's
  list of rejections.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  otx2_reset_task() checks netif_running() before rtnl_lock() and then…

--

octeontx2: add mqprio bandwidth offload for NIX TX schedulers

This adds TC_SETUP_QDISC_MQPRIO offload with the bw_rlimit shaper on
octeontx2 PF and VF netdevs. It programs per-queue MDQ CIR/PIR through the
AF mailbox and allocates one SMQ per non-QoS queue under TL4[0]. Every
mqprio add, replace and delete bounces the netdev through ndo_stop() and
ndo_open(). Rates are cached in software and staged in mq_offload_snap
snapshots that are committed on TC_ROOT_GRAFT or on teardown of the
replaced qdisc.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 836601fc10d8e..c95177602bf95 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

[ ... ]

> @@ -651,7 +784,11 @@ 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)
> +			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++;

[ ... ]

> @@ -777,6 +914,9 @@ int otx2_txsch_alloc(struct otx2_nic *pfvf)
>  		req->schq[NIX_TXSCH_LVL_TL4] = chan_cnt;
>  	}
>  
> +	if (pfvf->mqprio.rate_limit)
> +		req->schq[NIX_TXSCH_LVL_SMQ] = pfvf->hw.non_qos_queues;
> +
>  	rc = otx2_sync_mbox_msg(&pfvf->mbox);
>  	if (rc)
>  		return rc;

[Severity: Medium]
With rate_limit set, this requests non_qos_queues SMQs but still only one
TL4. otx2_txschq_config() then points every MDQ's NIX_AF_MDQX_PARENT at
schq_list[NIX_TXSCH_LVL_TL4][0]. The only thing it writes to
NIX_AF_MDQX_SCHEDULE is the DWRR quantum, so every MDQ gets PRIO 0.

Where is NIX_AF_TL4X_TOPOLOGY (PRIO_ANCHOR / RR_PRIO) programmed for that
TL4? The only TOPOLOGY writes I can find are the TL1 defaults in
otx2_txschq_config() and the HTB paths in qos.c, for example
otx2_qos_txschq_set_parent_topology():

	cfg->reg[0] = NIX_AF_TL4X_TOPOLOGY(parent->schq);
	...
	cfg->regval[0] = (u64)parent->prio_anchor << 32;

On the AF side, nix_reset_tx_schedule() resets only PARENT and SCHEDULE for
TL4, so a reused TL4 could keep a stale RR_PRIO / PRIO_ANCHOR. Would the
N PRIO 0 children still be arbitrated as a single DWRR group?

The netdev_warn_once() in otx2_setup_tc_mqprio() says the offload "uses
TL4[0] without explicit topology programming", yet the driver still reports
TC_MQPRIO_HW_OFFLOAD_TCS. Should the TL4 topology be programmed explicitly
before offload is reported as successful?

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index 777e7156badb7..f51161e8b5949 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -2008,6 +2008,15 @@ int otx2_open(struct net_device *netdev)
>  	if (err)
>  		goto err_free_mem;
>  
> +	/* Fail closed: abort open if cached mqprio shapers cannot be restored. */
> +	err = otx2_mqprio_up(pf);
> +	if (err) {
> +		netdev_err(pf->netdev,
> +			   "mqprio: failed to restore shapers during open: %d\n",
> +			   err);
> +		goto err_free_hw;
> +	}
> +
>  	/* Register NAPI handler */
>  	for (qidx = 0; qidx < pf->hw.cint_cnt; qidx++) {
>  		cq_poll = &qset->napi[qidx];

[ ... ]

> @@ -2206,6 +2215,7 @@ int otx2_open(struct net_device *netdev)
>  	free_irq(vec, pf);
>  err_disable_napi:
>  	otx2_disable_napi(pf);
> +err_free_hw:
>  	otx2_free_hw_resources(pf);
>  err_free_mem:
>  	otx2_free_queue_mem(qset);

[Severity: Medium]
Does this new unwind leak pf->refill_wrk? It is allocated on every open via
otx2_init_hw_resources()->otx2_config_nix_queues():

	pfvf->refill_wrk = devm_kcalloc(pfvf->dev, pfvf->qset.cq_cnt,
					sizeof(struct refill_work), GFP_KERNEL);

Neither otx2_free_hw_resources() nor otx2_free_queue_mem() frees it. The
only devm_kfree(pf->refill_wrk) is in otx2_stop(), and otx2_stop() returns
early afterwards because OTX2_FLAG_INTF_DOWN is set. The next open then
overwrites the pointer, and each leaked array stays on devres until unbind.

The older labels after otx2_init_hw_resources() have the same omission. This
new label, though, is reached whenever a shaper mailbox write fails during
any open while mqprio offload is active. That includes the mqprio bounces
and the MTU, channel and reset reopen paths.

[ ... ]

> 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

[ ... ]

> +static void otx2_mqprio_defer_netdev_tc_restore(struct otx2_nic *pfvf)
> +{
> +	if (READ_ONCE(pfvf->mqprio.defer_tc_work))
> +		return;
> +	schedule_work(&pfvf->mqprio.netdev_tc_work);
> +}

[Severity: Low]
After a failed replace, both the fail_validate and cleanup paths in
otx2_setup_tc_mqprio() call this to re-apply old_mq_snap later. Before the
work can take RTNL, mqprio_destroy() runs for the rejected new qdisc (with
hw_offload unset) and does:

	else
		netdev_set_num_tc(dev, 0);

The old offloaded mqprio remains root and the MDQ shapers stay programmed.
Until the work runs, netdev_pick_tx() / skb_tx_hash() see num_tc == 0. They
can then hash traffic onto queues shaped for other TCs.

Is this window intended? Could the layout be restored without going
through a work item?

> +
> +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 (!snap)
> +		return 0;
> +
> +	err = otx2_mqprio_alloc_cache(pfvf, false);
> +	if (err) {
> +		netdev_err(netdev,
> +			   "mqprio: rollback failed to allocate rate cache: %d\n",
> +			   err);
> +		return err;
> +	}

[ ... ]

> +	otx2_mqprio_apply_snap_netdev(netdev, snap);
> +
> +	if (otx2_mqprio_mdq_allocated(pfvf)) {
> +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> +		if (err) {
> +			netdev_err(netdev,
> +				   "mqprio: rollback shaper clear failed: %d; hardware limits may not match qdisc\n",
> +				   err);
> +			return err;
> +		}
> +	}

[Severity: Medium]
What state is the device left in when either of these early returns is
taken?

If otx2_mqprio_alloc_cache() fails, nothing has been restored yet. The
hardware and the caches still hold the rejected replacement, and rate_limit
stays true.

If otx2_nix_tm_clear_queue_shaper() fails, the cache has already been
rebuilt from old_mq_snap, but neither the restart nor otx2_mqprio_up()
runs. The MDQs then hold a mix of new and cleared CIR/PIR values.

Neither caller repairs this. otx2_teardown_tc_mqprio() only clears the
replace flags and returns the error, which mqprio_disable_offload() ignores.
The cleanup label in otx2_setup_tc_mqprio() only logs.

Should a failed rollback force the device back to a known state?

> +
> +	/* Rebuild the TX scheduler via netdev restart when running; otx2_mqprio_up()
> +	 * alone is insufficient after a failed replace that already bounced the
> +	 * interface. If open failed, TX schedulers were freed; defer shaper restore
> +	 * to the next successful ndo_open() via otx2_mqprio_up().
> +	 */
> +	pfvf->mqprio.rate_limit = true;
> +
> +	if (netif_running(netdev)) {
> +		err = otx2_mqprio_restart_netdev(netdev, true);
> +		if (err) {
> +			netdev_err(netdev,
> +				   "mqprio: rollback netdev restart failed: %d; qdisc rates may not be enforced\n",
> +				   err);
> +			return err;
> +		}

[Severity: Medium]
The comment above says shaper restore is deferred to the next successful
ndo_open(). Does that still hold if ndo_open() fails inside this restart?

On that failure, otx2_mqprio_restart_netdev() calls otx2_mqprio_clear_sw()
(rate_limit = false, rate caches freed), sets OTX2_FLAG_INTF_DOWN and calls
netif_close(). This function then returns without reinstating rate_limit or
the cache. The old offloaded mqprio qdisc stays installed.

On the next ip link set up, otx2_txsch_alloc() allocates a single SMQ and
otx2_mqprio_up() returns immediately:

	if (!pfvf->mqprio.rate_limit)
		return 0;

Wouldn't the interface then run without any bandwidth limits while the
qdisc still reports hardware-offloaded rates?

[ ... ]

> +static int otx2_mqprio_restart_netdev(struct net_device *netdev, bool rate_limit)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(netdev);
> +	const struct net_device_ops *ops = netdev->netdev_ops;
> +	bool running = netif_running(netdev);
> +	int err;

[ ... ]

> +	if (running) {
> +		clear_bit(__LINK_STATE_START, &netdev->state);
> +		smp_mb__after_atomic(); /* Commit netif_running(). */
> +	}
> +
> +	err = ops->ndo_stop(netdev);

[Severity: Medium]
This open-codes a close/open but skips the netpoll exclusion that
__dev_close_many() takes before ndo_stop():

	/* Temporarily disable netpoll until the interface is down */
	netpoll_poll_disable(dev);

netpoll_poll_dev() checks netif_running() only once, after
down_trylock(&ni->dev_lock). A poll that passed that check before the
clear_bit() could still be in poll_one_napi()->otx2_napi_handler() while
otx2_stop() runs.

In otx2_stop(), napi_synchronize() waits for NAPI_STATE_SCHED but not for
NAPI_STATE_NPSVC. otx2_free_hw_resources() also frees the CQ/SQ resources
before otx2_disable_napi() runs. With netconsole active, could this be a
use-after-free of the CQ rings?

otx2_change_mtu(), otx2_set_channels() and otx2_xdp_setup() already have
the same gap. This helper adds a new trigger on every mqprio add, replace,
delete and rollback. During the bounce, unlocked netif_running() readers
also see false while IFF_UP stays set.

[ ... ]

> +	err = ops->ndo_open(netdev);
> +	if (!err && running) {
> +		set_bit(__LINK_STATE_START, &netdev->state);
> +	} else if (err) {

[ ... ]

> +		otx2_mqprio_clear_sw(pfvf);
> +		/* ndo_open() rolls back on failure; mark the interface down so
> +		 * otx2_stop() returns early when netif_close() runs.  Caller
> +		 * holds RTNL; netif_close() is used since it needs no extra
> +		 * locking (dev_close() would also take the netdev ops lock).
> +		 */
> +		otx2_set_flag(pfvf, OTX2_FLAG_INTF_DOWN);
> +		/* visible to otx2_stop() on other cpus */
> +		smp_wmb();
> +		netif_close(netdev);
> +	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
otx2_reset_task() checks netif_running() before it takes RTNL:

	if (!netif_running(pf->netdev))
		return;

	rtnl_lock();
	otx2_stop(pf->netdev);
	pf->reset_count++;
	otx2_open(pf->netdev);

A reset task that is waiting on RTNL can reopen a device that was closed in
the meantime. This netif_close() becomes one more such close. otx2_stop()
returns early because of OTX2_FLAG_INTF_DOWN, and otx2_open() allocates
hardware, NAPI and IRQs while IFF_UP is clear.

Would a later ip link set up then open the device a second time? The same
race exists with an ordinary dev_close().

[ ... ]

> +static bool otx2_tc_can_offload(struct net_device *netdev)
> +{
> +	return !!(netdev->hw_features & NETIF_F_HW_TC);
> +}

[Severity: Low]
On the PF, otx2_probe() adds NETIF_F_HW_TC to hw_features only when
OTX2_FLAG_TC_FLOWER_SUPPORT is set, and that flag depends on MCAM entries
being allocated:

	if (otx2_test_flag(pf, OTX2_FLAG_TC_FLOWER_SUPPORT))
		netdev->hw_features |= NETIF_F_HW_TC;

So PFs without flower MCAM entries reject mqprio bandwidth offload with
-EOPNOTSUPP, even though MDQ shaping has nothing to do with flower. Is that
intended?

Because this tests hw_features rather than features, turning hw-tc-offload
off does not disable mqprio offload. That is inconsistent with the
tc_can_offload() check in otx2_teardown_tc_mqprio().

The commit message lists the rejected configurations:

  "Reject offload with PFC, XDP, HTB, SDP representors, per-TC rates on
  multi-queue classes, and ethtool channel changes while active."

That list leaves out this restriction. It also leaves out the rejection, in
otx2_setup_tc_mqprio(), of replacing a software mqprio root with an
offloaded one ("delete existing mqprio before re-enabling hw/sw offload").
Could both be documented?

> +
> +static int otx2_teardown_tc_mqprio(struct otx2_nic *pfvf,
> +				   struct tc_mqprio_qopt_offload *mqprio)
> +{

[ ... ]

> +	/* 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.
> +	 * qdisc_offload_graft_helper() skips TC_ROOT_GRAFT unless hw-tc-offload
> +	 * is enabled in netdev->features; commit without graft in that case.
> +	 */
> +	if (pfvf->mqprio.replace_setup_done && pfvf->cur_mq_snap) {
> +		err = 0;
> +		if (pfvf->mqprio.replace_graft_done || !tc_can_offload(netdev)) {
> +			otx2_mqprio_snap_commit(pfvf);
> +		} else {

[Severity: Medium]
Can this commit a replace that the qdisc core rejected?

In otx2_probe() and otx2vf_probe(), NETIF_F_HW_TC is added to hw_features
after netdev->features |= netdev->hw_features. So by default
tc_can_offload() is false, while otx2_tc_can_offload() still accepts mqprio
offload.

Take a tc qdisc replace ... mqprio ... hw 1 shaper bw_rlimit ... with an
estimator, issued over an active offloaded mqprio. mqprio_init() succeeds,
the driver reprograms the hardware, and TCQ_F_MQROOT is set. Then
qdisc_create() rejects the qdisc:

	if (tca[TCA_RATE]) {
		err = -EOPNOTSUPP;
		if (sch->flags & TCQ_F_MQROOT) {
			...
			goto err_out4;

err_out4 calls ops->destroy(). mqprio_destroy() has hw_offload set, so it
calls back into the driver and we end up here. replace_setup_done and
cur_mq_snap are set and !tc_can_offload() is true. The rejected config is
committed into old_mq_snap and left in hardware, while the old qdisc is
still root.

In this configuration TC_ROOT_GRAFT is never delivered, because
qdisc_offload_graft_helper() returns early:

	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
		return;

That means this branch cannot tell a successful graft from a failed
create. Later rollbacks would also restore this wrong snapshot.

[ ... ]

> +	/* Skip the netdev restart when mqprio offload was not active. */
> +	if (!had_mqprio)
> +		return 0;
> +
> +	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;
> +		}
> +
> +		err = otx2_mqprio_restart_netdev(netdev, false);

[Severity: Medium]
Doesn't this early return skip the step that otx2_mqprio_down() relies on?
If otx2_nix_tm_clear_queue_shaper() fails there, otx2_mqprio_clear_sw() has
already released the software state, and the comment in otx2_mqprio_down()
says:

	/* 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.
	 */

mqprio_disable_offload() ignores the return value of ndo_setup_tc(), so the
qdisc is removed anyway. Until some unrelated stop/open, the MDQs keep their
PIR/CIR values and the multi-SMQ topology under TL4[0].

Should the restart still run when otx2_mqprio_down() fails?

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> index f160b1618efa2..9ef55a6db50b7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> @@ -118,6 +118,9 @@ static void otx2_config_sched_shaping(struct otx2_nic *pfvf,
>  	/* configure PIR */
>  	maxrate = (node->rate > node->ceil) ? node->rate : node->ceil;
>  
> +	/* 65536 is the kernel-side default burst when HTB does not supply an
> +	 * explicit value, not the NIX hardware maximum (CN10K_MAX_BURST_SIZE).
> +	 */
>  	cfg->regval[*num_regs] =
>  		otx2_get_txschq_rate_regval(pfvf, maxrate, 65536);
>  	(*num_regs)++;

[Severity: Low]
This isn't a bug, but is this comment accurate? struct tc_htb_qopt_offload
has no burst or cburst field, so HTB offload never supplies an explicit
burst and the driver always uses this fixed constant.

The same wording appears in otx2_nix_tm_set_queue_shaper() ("Use the same
65536 byte default as the HTB shaper path").

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014023431.1775238-1-rkannoth%40marvell.com