netdev - Re: [PATCH net-next v7 5/7] net: stmmac: support fp parameter of tc-mqprio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240904154226.ztksb6sv4mjccb5l@skbuf>
Date: Wed, 4 Sep 2024 18:42:26 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Furong Xu <0x1207@...il.com>
Cc: Alexander Lobakin <aleksander.lobakin@...el.com>,
	Serge Semin <fancer.lancer@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Joao Pinto <jpinto@...opsys.com>, netdev@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	rmk+kernel@...linux.org.uk, linux@...linux.org.uk, xfr@...look.com
Subject: Re: [PATCH net-next v7 5/7] net: stmmac: support fp parameter of
 tc-mqprio

On Wed, Sep 04, 2024 at 05:21:20PM +0800, Furong Xu wrote:
> +static void stmmac_reset_tc_mqprio(struct net_device *ndev,
> +				   struct netlink_ext_ack *extack)
> +{
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +
> +	netdev_reset_tc(ndev);
> +	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
> +	stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
> +}
> +
> +static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> +				    struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	struct netlink_ext_ack *extack = mqprio->extack;
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	u32 offset, count, num_stack_tx_queues = 0;
> +	struct net_device *ndev = priv->dev;
> +	u32 num_tc = qopt->num_tc;
> +	int err;
> +
> +	if (!num_tc) {
> +		stmmac_reset_tc_mqprio(ndev, extack);
> +		return 0;
> +	}
> +
> +	err = netdev_set_num_tc(ndev, num_tc);
> +	if (err)
> +		return err;
> +
> +	for (u32 tc = 0; tc < num_tc; tc++) {
> +		offset = qopt->offset[tc];
> +		count = qopt->count[tc];
> +		num_stack_tx_queues += count;
> +
> +		err = netdev_set_tc_queue(ndev, tc, count, offset);
> +		if (err)
> +			goto err_reset_tc;
> +	}
> +
> +	err = netif_set_real_num_tx_queues(ndev, num_stack_tx_queues);
> +	if (err)
> +		goto err_reset_tc;
> +
> +	err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
> +					      mqprio->preemptible_tcs);
> +	if (err)
> +		goto err_reset_tc;

I appreciate the improvement with the separate tc_ops, but I'm still not
in love with this.

This stmmac_hw entry (copied with line numbers because it lacks a name
by which I can easily reference it):

159 »       }, {
160 »       »       .gmac = false,
161 »       »       .gmac4 = true,
162 »       »       .xgmac = false,
163 »       »       .min_id = 0,
164 »       »       .regs = {
165 »       »       »       .ptp_off = PTP_GMAC4_OFFSET,
166 »       »       »       .mmc_off = MMC_GMAC4_OFFSET,
167 »       »       »       .est_off = EST_GMAC4_OFFSET,
168 »       »       },
169 »       »       .desc = &dwmac4_desc_ops,
170 »       »       .dma = &dwmac4_dma_ops,
171 »       »       .mac = &dwmac4_ops,
172 »       »       .hwtimestamp = &stmmac_ptp,
173 »       »       .mode = NULL,
174 »       »       .tc = &dwmac510_tc_ops,
175 »       »       .mmc = &dwmac_mmc_ops,
176 »       »       .est = &dwmac510_est_ops,
177 »       »       .setup = dwmac4_setup,
178 »       »       .quirks = stmmac_dwmac4_quirks,
179 »       }, {

has .mac = &dwmac4_ops, so it does not implement .fpe_map_preemption_class().
But it also has .tc = &dwmac510_tc_ops, so tc_setup_dwmac510_mqprio() will
get called.

Thus, I suppose that the stmmac_fpe_map_preemption_class() ->
stmmac_do_void_callback() mechanism will return -EINVAL for dwmac4_ops,
and this will make mqprio offload fail, for the sole reason that FPE is
not supported, _EVEN IF_ FPE was never requested (mqprio->preemptible_tcs is 0),
and the offload could have otherwise been applied just fine.

Not to mention my previous complaint still applies, that the test for
the presence of stmmac_ops :: fpe_map_preemption_class() is unnaturally
late relative to the flow of the tc_setup_dwmac510_mqprio() function.

Thus, I really recommend you to replace the stmmac_do_void_callback()
anti-pattern with something like:

	// early
	if (mqprio->preemptible_tcs && !priv->hw->ops->fpe_map_preemption_class) {
		NL_SET_ERR_MSG_MOD(mqprio->extack,
				   "Cannot map preemptible TCs to TXQs");
		return -EOPNOTSUPP;
	}

	// late
	if (priv->hw->ops->fpe_map_preemption_class) {
		err = priv->hw->ops->fpe_map_preemption_class(priv->dev,
							      mqprio->preemptible_tcs,
							      extack);
		if (err)
			goto err_reset_tc;
	}

WARNING: code not tested. The idea is that the early check makes sure
that only dwmac410_ops and dwmac510_ops permit mqprio->preemptible_tcs
to go to a non-zero value (which can later be reset to zero if desired,
in the late code path). For dwmac4_ops, mqprio->preemptible_tcs = 0 is
the only supported value (for which nothing needs to be done), and the
late code path is bypassed, due to the "if" condition returning false.

Either organize like this, or if you really, really, really insist to
use the stmmac_do_callback() anti-pattern in new code, at least don't
share the setup_mqprio() code path between MACs that implement
fpe_map_preemption_class() and MACs that don't.