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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 6 · Low: 0

- [High] The new mqprio replace protocol depends on the core delivering
  TC_SETUP_ROOT_QDISC/TC_ROOT_GRAFT, but qdisc_offload_graft_root() ->…
- [Medium] otx2_setup_tc_mqprio()'s `cleanup:` rollback for a failed `tc
  qdisc replace` is silently discarded by the qdisc core.
- [Medium] A failed ndo_open() during the mqprio netdev bounce leaves the
  interface administratively DOWN: otx2_mqprio_restart_netdev() sets…
- [Medium] The advertised mqprio replacement rollback does not restore
  the priority-to-TC map.
- [Medium] Replacing between software mqprio and bandwidth-offloaded
  mqprio clears the newly installed traffic-class mapping in both…
- [Medium] otx2_mqprio_restore_old() aborts the rollback if
  otx2_nix_tm_clear_queue_shaper() fails, returning before…
- [Medium] otx2_mqprio_restore_old() calls otx2_mqprio_alloc_cache(pfvf,
  false), which unconditionally frees the *live* per-queue rate arrays…

--

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

This adds TC_SETUP_QDISC_MQPRIO offload with TC_MQPRIO_SHAPER_BW_RATE,
programming per-queue MDQ CIR/PIR through the NIX TX scheduler mailbox and
bouncing the netdev through ndo_stop()/ndo_open() to rebuild the scheduler
hierarchy.  Replace handling stages a new snapshot and rolls back to the
previous one on failure, using TC_ROOT_GRAFT to detect a successful graft.

A few questions below about the replace/rollback protocol and the netdev
bounce.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 175992188c18a..a519e0a97a2b7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

[ ... ]

> @@ -779,6 +919,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;

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index eecee612b7b2c..ede7f1113b714 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> @@ -483,6 +484,23 @@ struct pf_irq_data {
>  	int mdevs;
>  };
>  
> +struct mq_offload_snap {
> +	u64 min_rate[TC_QOPT_MAX_QUEUE];
> +	u64 max_rate[TC_QOPT_MAX_QUEUE];
> +	__u8 num_tc;
> +	__u16 count[TC_QOPT_MAX_QUEUE];
> +	__u16 offset[TC_QOPT_MAX_QUEUE];
> +};

[Severity: Medium]
Should the snapshot also record the priority-to-TC map?  The snapshot keeps
num_tc, count[], offset[] and the rates, and otx2_mqprio_restore_old()
restores exactly those, but netdev->prio_tc_map is never saved or restored.

mqprio_init() installs the replacement map unconditionally once the driver
offload call has succeeded:

net/sched/sch_mqprio.c:mqprio_init() {
	/* Always use supplied priority mappings */
	for (i = 0; i < TC_BITMASK + 1; i++)
		netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
	...
	sch->flags |= TCQ_F_MQROOT;
}

If qdisc_create() then fails after init, for instance because TCA_RATE was
supplied, which is always rejected for a TCQ_F_MQROOT qdisc:

net/sched/sch_api.c:qdisc_create() {
	if (tca[TCA_RATE]) {
		err = -EOPNOTSUPP;
		if (sch->flags & TCQ_F_MQROOT) {
			NL_SET_ERR_MSG(extack, "Cannot attach rate estimator to a multi-queue root qdisc");
			goto err_out4;
	...
}

the old qdisc stays installed and the driver rolls the rates and the queue
layout back, but the device is left with the rejected configuration's
prio_tc_map.  Would packets then be classified into traffic classes that do
not match the queue ranges and rates that are actually in effect?

[ ... ]

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

[ ... ]

> @@ -1600,14 +1609,714 @@ static int otx2_setup_tc_block(struct net_device *netdev,
> +static int otx2_mqprio_alloc_cache(struct otx2_nic *pfvf, bool replacing)
> +{
> +	u16 num_txq = pfvf->hw.non_qos_queues;
> +
> +	if (replacing && pfvf->mqprio.min_rate && pfvf->mqprio.max_rate) {
> +		memset(pfvf->mqprio.min_rate, 0,
> +		       num_txq * sizeof(*pfvf->mqprio.min_rate));
> +		memset(pfvf->mqprio.max_rate, 0,
> +		       num_txq * sizeof(*pfvf->mqprio.max_rate));
> +		pfvf->mqprio.flags = 0;
> +		return 0;
> +	}
> +
> +	otx2_mqprio_free_cache(pfvf);
> +
> +	pfvf->mqprio.min_rate = devm_kcalloc(pfvf->dev, num_txq,
> +					     sizeof(*pfvf->mqprio.min_rate),
> +					     GFP_KERNEL);
> +	pfvf->mqprio.max_rate = devm_kcalloc(pfvf->dev, num_txq,
> +					     sizeof(*pfvf->mqprio.max_rate),
> +					     GFP_KERNEL);
> +	if (!pfvf->mqprio.min_rate || !pfvf->mqprio.max_rate) {
> +		otx2_mqprio_free_cache(pfvf);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Can this lose the live rate cache?  otx2_mqprio_restore_old() calls
otx2_mqprio_alloc_cache(pfvf, false), which frees the existing min_rate and
max_rate arrays before attempting the new devm_kcalloc().  If either
allocation fails, the only copy of the cached rates is already gone and
-ENOMEM is returned early, so:

  - pfvf->mqprio.rate_limit stays true (set earlier by
    otx2_mqprio_restart_netdev()), so the offload still counts as active and
    ethtool channel changes and XDP remain blocked,
  - otx2_mqprio_up() then always bails out:

	if (!pfvf->mqprio.min_rate || !pfvf->mqprio.max_rate)
		return 0;

    so the shapers can no longer be restored on any later ndo_open(),
  - pfvf->cur_mq_snap is left allocated because the trailing
    otx2_mqprio_snap_free() is skipped.

Since non_qos_queues cannot change while rate_limit is set, could the restore
path pass replacing=true and just memset the existing arrays instead of
freeing them first?

> +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)
> +		return err;

[ ... ]

> +	netdev_set_num_tc(netdev, snap->num_tc);
> +	for (tc = 0; tc < snap->num_tc; tc++)
> +		netdev_set_tc_queue(netdev, tc, snap->count[tc],
> +				    snap->offset[tc]);
> +

[Severity: Medium]
Does this TC layout restore survive the return to the qdisc core?  When the
replacement setup fails, otx2_setup_tc_mqprio() calls this from the cleanup:
label and then returns a non-zero error.  The failed instance never had
priv->hw_offload set, since mqprio_enable_offload() only assigns it after
ndo_setup_tc() succeeds, and destroy runs anyway:

net/sched/sch_api.c:qdisc_create() {
err_out4:
	/* Even if ops->init() failed, we call ops->destroy()
	 * like qdisc_create_dflt().
	 */
	if (ops->destroy)
		ops->destroy(sch);
}

net/sched/sch_mqprio.c:mqprio_destroy() {
	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);
	else
		netdev_set_num_tc(dev, 0);
}

So the still-installed old mqprio qdisc ends up with dev->num_tc == 0 while
pfvf->mqprio.rate_limit stays true and the restored per-queue shapers remain
programmed.  Would traffic then spread over all TX queues, some of which
still carry the restored limits?  The commit message says "failed setup
restores the old snapshot" - does that hold given the core clears num_tc
afterwards?

> +	if (otx2_mqprio_mdq_allocated(pfvf)) {
> +		err = otx2_nix_tm_clear_queue_shaper(pfvf);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* otx2_mqprio_restart_netdev() clears rate_limit when ndo_open() fails.
> +	 * 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 (pfvf->hw.txschq_cnt[NIX_TXSCH_LVL_SMQ]) {
> +		err = otx2_mqprio_up(pfvf);
> +		if (err)
> +			return err;
> +	}
> +
> +	otx2_mqprio_snap_free(pfvf, &pfvf->cur_mq_snap);
> +
> +	return 0;
> +}

[Severity: Medium]
What state is left behind if otx2_nix_tm_clear_queue_shaper() fails here?  It
issues a mailbox transaction through otx2_nix_tmq_reg_write() ->
otx2_sync_mbox_msg(), which can time out or be rejected by the AF.  On that
early return the software cache and the netdev TC mapping have already been
rewritten to the old configuration, but:

  - pfvf->mqprio.rate_limit = true is not reached,
  - otx2_mqprio_up() does not run, so the MDQ shapers keep the partially
    applied new rates,
  - pfvf->cur_mq_snap is not freed.

The caller only logs "mqprio: replace failed and prior configuration rollback
failed", so is there anything that re-synchronises hardware with the cached
rates before the next successful ndo_open()?

> +static void otx2_mqprio_clear_sw(struct otx2_nic *pfvf)
> +{
> +	struct net_device *netdev = pfvf->netdev;
> +
> +	pfvf->mqprio.rate_limit = false;
> +	otx2_mqprio_clear_replace_state(pfvf);
> +	netdev_set_num_tc(netdev, 0);
> +	otx2_mqprio_free_cache(pfvf);
> +}

[Severity: Medium]
Can this unconditional netdev_set_num_tc(netdev, 0) wipe a mapping that the
replacement qdisc already installed?

Consider an active bandwidth offload followed by

  tc qdisc replace dev X root mqprio num_tc 2 map ... queues ... hw 0

The new qdisc has qopt->hw == 0, so the driver is never called during
mqprio_init(), which programs the mapping itself:

net/sched/sch_mqprio.c:mqprio_init() {
	} else {
		netdev_set_num_tc(dev, qopt->num_tc);
		for (i = 0; i < qopt->num_tc; i++)
			netdev_set_tc_queue(dev, i, qopt->count[i], qopt->offset[i]);
	}
}

replace_setup_done therefore stays false, and the old offloaded instance is
destroyed afterwards, reaching otx2_teardown_tc_mqprio() with had_mqprio ==
true, which calls otx2_mqprio_down() and otx2_mqprio_restart_netdev(netdev,
false) - both of which end in otx2_mqprio_clear_sw() and this
netdev_set_num_tc(netdev, 0).

The reverse direction, software mqprio replaced by an offloaded one, ends the
same way through mqprio_destroy()'s "else netdev_set_num_tc(dev, 0)" branch
after the driver has installed its mapping.

In both cases the tc command reports success while dev->num_tc ends up 0.  Is
that intended?

[ ... ]

> +static int otx2_mqprio_restart_netdev(struct net_device *netdev, bool rate_limit)
> +{

[ ... ]

> +	err = ops->ndo_open(netdev);
> +	if (err) {
> +		netdev_err(netdev,
> +			   "Failed to restart device after mqprio change: %d\n",
> +			   err);
> +		/* ndo_open() already freed the TX schedulers on failure while
> +		 * netif_running() may still be true; drop mqprio software state
> +		 * only instead of sending shaper clears to freed queues.
> +		 */
> +		otx2_mqprio_clear_sw(pfvf);
> +		/* ndo_open() rolls back on failure; mark the interface down so
> +		 * netif_close() does not invoke ndo_stop() on freed NAPI/queue
> +		 * state. Caller holds RTNL; dev_close() would deadlock.
> +		 */
> +		pfvf->flags |= OTX2_FLAG_INTF_DOWN;
> +		/* visible to otx2_stop() on other cpus */
> +		smp_wmb();
> +		netif_close(netdev);
> +	}
> +
> +	return err;
> +}

[Severity: Medium]
Should the commit message mention that a tc command can take the link down?
netif_close() here clears IFF_UP and __LINK_STATE_START, so the interface is
left administratively down and needs a manual "ip link set up".

ndo_open() failure is newly resource dependent, because otx2_txsch_alloc()
now asks for hw.non_qos_queues SMQs instead of one:

	if (pfvf->mqprio.rate_limit)
		req->schq[NIX_TXSCH_LVL_SMQ] = pfvf->hw.non_qos_queues;

which the AF can reject when that many MDQ/SMQ nodes are not free, depending
on what other PFs and VFs have allocated.

On the teardown path this also happens with no error visible to user space,
since the return value is dropped:

net/sched/sch_mqprio.c:mqprio_disable_offload() is static void and does

	dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, &mqprio);

so "tc qdisc del dev X root" reports success while the link is now down.  Is
that acceptable?

[ ... ]

> +static int otx2_teardown_tc_mqprio(struct otx2_nic *pfvf,
> +				   struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	bool had_mqprio = pfvf->mqprio.rate_limit;
> +	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) {
> +		err = 0;
> +		if (pfvf->mqprio.replace_graft_done)
> +			otx2_mqprio_snap_commit(pfvf);
> +		else
> +			err = otx2_mqprio_restore_old(pfvf);
> +		otx2_mqprio_clear_replace_state(pfvf);
> +		return err;
> +	}

[ ... ]

> +static int otx2_setup_tc_root(struct otx2_nic *pfvf,
> +			      struct tc_root_qopt_offload *root)
> +{
> +	switch (root->command) {
> +	case TC_ROOT_GRAFT:
> +		if (pfvf->mqprio.replace_setup_done)
> +			pfvf->mqprio.replace_graft_done = true;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

[Severity: High]
Is TC_ROOT_GRAFT guaranteed to be delivered to this driver?  The graft
notification is gated on tc_can_offload(), while the mqprio setup and
teardown calls are not:

net/sched/sch_api.c:qdisc_offload_graft_helper() {
	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
		return;
}

net/sched/sch_mqprio.c:mqprio_enable_offload()/mqprio_disable_offload() {
	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, &mqprio);
}

tc_can_offload() tests dev->features & NETIF_F_HW_TC, and in this driver that
bit is added to hw_features after features have already been copied:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c {
	netdev->features |= netdev->hw_features;

	/* HW supports tc offload but mutually exclusive with n-tuple filters */
	if (pf->flags & OTX2_FLAG_TC_FLOWER_SUPPORT)
		netdev->hw_features |= NETIF_F_HW_TC;
}

so hw-tc-offload is off in dev->features by default (and can be turned off
with ethtool at any time).  In that state the mqprio bandwidth offload is
still accepted, but replace_graft_done stays false, and the sequence

  tc qdisc add dev X root mqprio ... hw 1 shaper bw_rlimit
  tc qdisc replace dev X root mqprio ... hw 1 shaper bw_rlimit

reports success while the old instance's teardown takes the
!replace_graft_done branch above and calls otx2_mqprio_restore_old(),
re-applying the previous rates and TC layout.  Does the new configuration end
up silently discarded here?

Separately, otx2_setup_tc_root() ignores root->ingress and root->handle.
Would a clsact or ingress root graft be taken as confirmation of the egress
mqprio graft?

[ ... ]

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