Re: [PATCH v21 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 11 potential
issue(s) to consider.

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

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] Replacing an offloaded mqprio with a software taprio stops TX
  on the interface.
- [Medium] With mqprio rate limiting active, otx2_txsch_alloc() requests
  hw.non_qos_queues SMQs but still only one TL4.
- [Medium] Setup and teardown use different notions of 'offload enabled'.
- [Medium] When otx2_mqprio_down() fails, otx2_teardown_tc_mqprio()
  returns immediately and skips otx2_mqprio_restart_netdev(netdev,…
- [Medium] The new otx2_mqprio_up() failure branch in otx2_open() jumps
  to the new err_free_hw label.
- [Medium] octeontx2-pf: premature otx2_mcam_flow_del() in otx2_probe()
  error path
- [Low] The commit message leaves out several changes that reach code
  paths unrelated to mqprio.
- [Low] otx2_mqprio_restart_netdev(), called from ndo_setup_tc, clears
  and sets __LINK_STATE_START on netdev->state directly.
- [Low] The new comment in otx2_config_sched_shaping() calls 65536 'the
  kernel-side default burst when HTB does not supply an explicit value'.
- [Low] The comment on otx2_mqprio_keep_netdev_tc() says it detects a new
  software mqprio that installed its TC layout before the old offloaded…
- [Low] In otx2_setup_tc_mqprio(), the cleanup path for a failed replace
  is entered mid-loop when otx2_nix_tm_set_queue_shaper() fails.

--

octeontx2: add mqprio bandwidth offload for NIX TX schedulers

This commit adds TC_SETUP_QDISC_MQPRIO offload with the bw_rlimit shaper
for octeontx2 PF and VF netdevs. It programs per-queue MDQ CIR/PIR through
the NIX TX scheduler mailbox.

The TX scheduler cannot be reprogrammed live, so every mqprio add, replace
and delete bounces the netdev through ndo_stop()/ndo_open(). Cached rates
are restored from otx2_mqprio_up(), and replace operations are staged in
snapshots.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index f4047b0d5d963..b4a738cf9a8b7 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++;

[Severity: Medium]
With rate_limit set, otx2_txsch_alloc() asks for hw.non_qos_queues SMQs
but still only one TL4. Every MDQ then becomes a child of TL4[0] with
MDQ_SCHEDULE = dwrr_val (priority 0).

Is NIX_AF_TL4X_TOPOLOGY for TL4[0] programmed anywhere on this path?

As far as I can tell, TL4X_TOPOLOGY is only written from nic/qos.c. The AF
reset clears only PARENT and SCHEDULE:

drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:nix_reset_tx_schedule() {
	case NIX_TXSCH_LVL_TL4:
		tlx_parent   = NIX_AF_TL4X_PARENT(schq);
		tlx_schedule = NIX_AF_TL4X_SCHEDULE(schq);
}

The HTB teardown in otx2_qos_free_hw_schq() does reset the topology, but
only best-effort. If the mailbox fails it warns and frees the schq anyway.

TL4[0] may have been last used by another function, or by an HTB teardown
whose reset failed. Can it then keep a non-zero RR_PRIO/PRIO_ANCHOR? If
so, the priority-0 MDQ children would not form a single DWRR group, and
per-queue shaping could behave unexpectedly.

The patch acknowledges this later with:

	netdev_warn_once(netdev,
			 "mqprio: bandwidth offload uses TL4[0] without explicit topology programming; verify scheduling if limits look wrong\n");

Would it make sense to program TL4[0] TOPOLOGY with an RR_PRIO that
matches the MDQ priority when several MDQs are attached?

[ ... ]

> 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;
> +	}

[Severity: Medium]
Does this new error path leak pf->refill_wrk?

otx2_init_hw_resources()->otx2_config_nix_queues() has just allocated it:

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

err_free_hw only runs otx2_free_hw_resources() and otx2_free_queue_mem().
Neither of them frees refill_wrk. The only devm_kfree() of it is in
otx2_stop().

When the open fails from otx2_mqprio_restart_netdev(), OTX2_FLAG_INTF_DOWN
is set before netif_close(). otx2_stop() then returns early, so the
allocation is never released.

With rate_limit active, a persistent shaper mailbox failure would leak
cq_cnt * sizeof(struct refill_work) on every admin up attempt until the
driver is unbound.

The older error labels after otx2_init_hw_resources() have the same gap.
This patch adds a new way to reach it from userspace.

[ ... ]

> @@ -2281,6 +2291,7 @@ int otx2_stop(struct net_device *netdev)
>  	for (qidx = 0; qidx < netdev->num_tx_queues; qidx++)
>  		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, qidx));
>  
> +	synchronize_net();
>  	otx2_free_queue_mem(qset);

[Severity: Low]
This isn't a bug, but several changes here affect paths outside mqprio,
and the commit message does not mention them.

synchronize_net() now runs on every ndo_stop. That covers admin down,
otx2_change_mtu(), ethtool channel and ring changes, XDP attach/detach,
and the reset task. Under RTNL it becomes synchronize_rcu_expedited().

What race does it close? If it fixes an existing problem with
otx2_free_queue_mem(), could it go in a separate patch with a Fixes: tag?

otx2_txschq_stop() now zeroes hw.txschq_cnt[] for every caller. After a
stop, otx2_get_smq_idx() returns 0 instead of the last SMQ. Existing users
such as otx2_pfc_update_sq_smq_mapping() and otx2_qos_disable_sq() will
see this.

The PF and VF probe/remove ordering also changes:

  - otx2_init_tc() and otx2vf_mcam_flow_init() now run before
    register_netdev()
  - otx2_shutdown_tc() moves to right after unregister_netdev()
  - the error labels are rewired

There are also comment-only edits in qos.c and otx2_get_egress_burst_cfg().

Could the commit message describe these changes, or could they be split
out?

[ ... ]

> 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 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;
> +	}

[Severity: Low]
The replace path in otx2_setup_tc_mqprio() can reach cleanup because
otx2_nix_tm_set_queue_shaper() failed partway through the loop. At that
point pfvf->mqprio.min_rate[]/max_rate[] and pfvf->mqprio.flags already
hold part of the new config, and rate_limit is true.

If otx2_mqprio_alloc_cache() fails here, we return before refilling the
cache from old_mq_snap. Does that leave the partially-new rates cached
with rate_limit still set, while the old qdisc stays root?

The same applies if otx2_nix_tm_clear_queue_shaper() fails further down.
The cache is restored, but the hardware keeps the partially-new shapers.

On the restore_err branch in otx2_setup_tc_mqprio(), no deferred TC
restore is scheduled. num_tc therefore stays 0 after mqprio_destroy() of
the failed qdisc. From then on, otx2_mqprio_up() would re-apply the mixed
rate set on every ndo_open().

[ ... ]

> +/* Offloaded mqprio replaced by software mqprio installs netdev TC layout in
> + * mqprio_init() before the old offload instance is destroyed during graft.
> + */
> +static bool otx2_mqprio_keep_netdev_tc(struct otx2_nic *pfvf)
> +{
> +	struct Qdisc *qdisc = rtnl_dereference(pfvf->netdev->qdisc);
> +
> +	return qdisc && qdisc->ops && !strcmp(qdisc->ops->id, "mqprio");
> +}

[Severity: Low]
Can this match the offloaded mqprio that is being destroyed, and not only
a new software mqprio?

dev_shutdown() puts the root qdisc while dev->qdisc still points at it:

net/sched/sch_generic.c:dev_shutdown() {
	qdisc_put(rtnl_dereference(dev->qdisc));
	rcu_assign_pointer(dev->qdisc, &noop_qdisc);
}

When offload is active, mqprio_destroy() leaves num_tc to the driver:

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);
}

This happens on unregister, and in __dev_change_net_namespace(), which
calls netif_close() and then dev_shutdown(). In both cases
otx2_mqprio_clear_sw() would see "mqprio" and skip
netdev_set_num_tc(netdev, 0).

After a netns move, the stale TC layout would stay while only the default
mq is attached. skb_tx_hash() would keep limiting queue selection to the
old TC offset/count until a new mqprio is installed.

> +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);
> +	if (!otx2_mqprio_keep_netdev_tc(pfvf)) {
> +		struct Qdisc *root = rtnl_dereference(netdev->qdisc);
> +
> +		if (root && root->ops && !strcmp(root->ops->id, "taprio"))
> +			netdev_warn(netdev,
> +				    "mqprio: offload teardown cleared the TC layout of the new %s\n",
> +				    root->ops->id);
> +		netdev_set_num_tc(netdev, 0);
> +	}

[Severity: Medium]
Does replacing an offloaded mqprio with a software taprio stop TX on the
interface?

Consider "tc qdisc replace dev X root taprio ..." without full offload:

  1. taprio_change() installs its layout with netdev_set_num_tc() and
     netdev_set_tc_queue().
  2. qdisc_graft() publishes taprio as dev->qdisc and destroys the old
     mqprio.
  3. Because hw_offload is set, mqprio_destroy() calls ndo_setup_tc()
     with hw=0.
  4. otx2_teardown_tc_mqprio() reaches otx2_mqprio_clear_sw(), either
     directly or through otx2_mqprio_down()/otx2_mqprio_restart_netdev().

Here the root is detected as "taprio" and the warning is printed, but
netdev_set_num_tc(netdev, 0) still runs.

otx2 returns -EOPNOTSUPP for TAPRIO caps, so taprio uses
taprio_dequeue_tc_priority():

net/sched/sch_taprio.c:taprio_dequeue_tc_priority() {
	int num_tc = netdev_get_num_tc(dev);
	...
	for (tc = num_tc - 1; tc >= 0; tc--) {
}

With num_tc == 0 this loop never runs, so dequeue always returns NULL.

Should the TC layout also be kept for taprio, or for any new root that
owns it?

[ ... ]

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

[ ... ]

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

[ ... ]

> +		otx2_set_flag(pfvf, OTX2_FLAG_INTF_DOWN);
> +		/* visible to otx2_stop() on other cpus */
> +		smp_wmb();
> +		netif_close(netdev);
> +	}

[Severity: Low]
Is it ok for a driver to clear and set __LINK_STATE_START directly from
ndo_setup_tc()? I couldn't find another driver under drivers/net that
does this. The bit is owned by the core open/close helpers.

When ndo_open() fails, netif_close() is also called from inside the qdisc
operation. A failed "tc qdisc add/replace/del ... mqprio" can then take
the interface administratively down. That includes a delete, which the
core cannot veto. IFF_UP is cleared and NETDEV_GOING_DOWN/NETDEV_DOWN are
sent.

The commit message only says the netdev is bounced through
ndo_stop()/ndo_open(). Could it mention that a tc command can close the
interface?

[ ... ]

> +static bool otx2_tc_can_offload(struct net_device *netdev)
> +{
> +	return !!(netdev->hw_features & NETIF_F_HW_TC);
> +}
> +
> +static int otx2_teardown_tc_mqprio(struct otx2_nic *pfvf,
> +				   struct tc_mqprio_qopt_offload *mqprio)
> +{

[ ... ]

> +	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]
otx2_tc_can_offload() checks netdev->hw_features. This check uses the
core tc_can_offload(), which checks netdev->features. Can the two
disagree in the default configuration?

Both otx2_probe() and otx2vf_probe() add NETIF_F_HW_TC to hw_features
after "netdev->features |= netdev->hw_features". So hw-tc-offload is off
by default, and qdisc_offload_graft_helper() never sends TC_ROOT_GRAFT:

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

As a result, this branch always commits.

Suppose a replacement mqprio passes otx2_setup_tc_mqprio() and then fails
later in qdisc_create(). For example, an estimator on a TCQ_F_MQROOT
qdisc goes to err_out4 and ->destroy().

Wouldn't the rejected config be committed as old_mq_snap? The hardware
shapers and netdev TC layout would keep enforcing it while the old qdisc
stays root.

Separately, on the PF NETIF_F_HW_TC is only in hw_features when
OTX2_FLAG_TC_FLOWER_SUPPORT is set. Is it intended that MDQ shaping is
refused on PFs without flower/MCAM support? The commit message says the
feature applies to PF and VF netdevs, without that qualification.

[ ... ]

> +	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]
If otx2_nix_tm_clear_queue_shaper() fails inside otx2_mqprio_down(), the
software state has already been released through otx2_mqprio_clear_sw().
otx2_mqprio_down() explains this as:

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

On this error path, though, we return before otx2_mqprio_restart_netdev(),
so no restart follows. Doesn't that leave the MDQ CIR/PIR limits and the
widened SMQ allocation in place, with rate_limit false and num_tc 0?

mqprio_disable_offload() is void, so "tc qdisc del" looks successful to
the user. The stale limits would remain until an unrelated
ndo_stop()/ndo_open().

Should the restart still happen when the clear fails?

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> index 2ede0b84e5f52..732fff75ab0be 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);

[Severity: Low]
This isn't a bug, but is this comment accurate? struct tc_htb_qopt_offload
has no burst member, so HTB offload can never supply an explicit burst,
and 65536 is always used.

The comment in otx2_nix_tm_set_queue_shaper() ("Use the same 65536 byte
default as the HTB shaper path") repeats the same idea.

[ ... ]

> @@ -3420,11 +3437,13 @@ static int otx2_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	otx2_sriov_vfcfg_cleanup(pf);
>  err_pf_sriov_init:
>  	otx2_unregister_dl(pf);
> -	otx2_shutdown_tc(pf);
>  err_mcam_flow_del:
>  	otx2_mcam_flow_del(pf);
>  err_unreg_netdev:
> +	otx2_shutdown_tc_mqprio(pf);
>  	unregister_netdev(netdev);
> +err_shutdown_tc:
> +	otx2_shutdown_tc(pf);
>  err_ipsec_clean:
>  	cn10k_ipsec_clean(pf);
>  err_mcs_free:

[Severity: Medium]
Does the new label ordering stop otx2_shutdown_tc() from doing
anything on the late probe error paths?

Before this patch, err_pf_sriov_init ran otx2_shutdown_tc() and only
then fell through to otx2_mcam_flow_del(). With this patch, a failure
in otx2_register_dl(), otx2_sriov_vfcfg_init(), the af_xdp_zc_qidx
bitmap_zalloc() or otx2_dcbnl_set_ops() unwinds like this:

  err_mcam_flow_del:  otx2_mcam_flow_del()
  err_unreg_netdev:   otx2_shutdown_tc_mqprio(), unregister_netdev()
  err_shutdown_tc:    otx2_shutdown_tc()

otx2_mcam_flow_del() goes through otx2_destroy_mcam_flows(), which
clears OTX2_FLAG_MCAM_ENTRIES_ALLOC.

otx2_shutdown_tc() then calls otx2_destroy_tc_flow_list(). That
function returns early when OTX2_FLAG_MCAM_ENTRIES_ALLOC is clear, so
it no longer walks flow_cfg->flow_list_tc.

Any otx2_tc_flow nodes added while the netdev was registered are
therefore never freed. The window is small, and installing flower
rules needs CAP_NET_ADMIN.

The ordering is also now the reverse of otx2_remove() after this
patch. There, otx2_shutdown_tc() runs right after unregister_netdev(),
and otx2_mcam_flow_del() runs later.

The unwind already reaches err_del_mcam_entries by fallthrough from
err_shutdown_tc/err_ipsec_clean/err_mcs_free. err_del_mcam_entries
calls otx2_mcam_flow_del() again, so the call at err_mcam_flow_del
looks redundant.

Could that first call be dropped, with the labels folded, so the probe
error path mirrors otx2_remove()? Or is there a reason the MCAM flows
have to be deleted before unregister_netdev() here?

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