Re: [PATCH net-next v8 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 2

- [Medium] emac_tc_setup_mqprio() commits all software state (netdev TC
  mapping via netdev_set_num_tc()/netdev_set_tc_queue(),…
- [Medium] icssg_iet_set_preempt_mask() builds EXPRESS_PRE_EMPTIVE_Q_MASK
  (0x0029) from scratch (`int prempt_mask = 0`) and only ORs in BIT(i)…
- [Medium] icssg_config_ietfpe() and icssg_qos_link_state_update() gate
  all FPE work on a single lockless READ_ONCE(emac->link) check, but…
- [Low] Concern (validated as a FALSE POSITIVE for the series): the FPE
  enable path added in icssg_qos.c (icssg_config_ietfpe() enable branch,…
- [Low] Unbalanced mutex lifecycle across the two ICSSG drivers:
  mutex_init(&emac->qos.iet.fpe_lock) is performed only by…

--

net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

This adds a QoS submodule to the ICSSG driver that programs IET Frame
Preemption in firmware, tracks the MAC Merge verify state machine, and
maps mqprio traffic classes to express/preemptible queues.  FPE state is
re-driven on link up/down and serialized with a new per-port fpe_lock.

A few questions about the new code below.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 4a7d1a6f470b1..d59eef61e7012 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -1754,6 +1754,7 @@ void prueth_netdev_exit(struct prueth *prueth,
>  
>  	netif_napi_del(&emac->napi_rx);
>  
> +	mutex_destroy(&emac->qos.iet.fpe_lock);

[Severity: Low]
prueth_netdev_exit() is shared by both ICSSG drivers, but the matching
mutex_init() only happens in icssg_qos_init(), which is called from
prueth_netdev_init() in icssg_prueth.c.

icssg_prueth_sr1.c has its own static prueth_netdev_init() that never
calls icssg_qos_init(), yet it reaches this teardown from both its probe
unwind path and prueth_remove().  Does that mean every SR1 emac has its
fpe_lock destroyed without ever being initialized?

Since alloc_etherdev_mq() zeroes the private area, mutex_destroy() only
sees owner == 0 today (and it is an empty inline without
CONFIG_DEBUG_MUTEXES), so nothing warns.  Would it be cleaner to
initialize the lock in the same layer that destroys it, so that any
future shared code in icssg_common.c taking fpe_lock does not end up
locking an uninitialized mutex on SR1?

>  	pruss_release_mem_region(prueth->pruss, &emac->dram);
>  	free_netdev(emac->ndev);
>  	prueth->emac[mac] = NULL;

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..283be27a240e9 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -357,11 +357,11 @@ static void emac_adjust_link(struct net_device *ndev)
>  		}
>  		if (!emac->link) {
>  			new_state = true;
> -			emac->link = 1;
> +			WRITE_ONCE(emac->link, 1);
>  		}
>  	} else if (emac->link) {
>  		new_state = true;
> -		emac->link = 0;
> +		WRITE_ONCE(emac->link, 0);
>  
>  		/* f/w should support 100 & 1000 */
>  		emac->speed = SPEED_1000;
> @@ -392,6 +392,8 @@ static void emac_adjust_link(struct net_device *ndev)
>  		} else {
>  			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>  		}
> +
> +		icssg_qos_link_state_update(ndev);
>  	}

[Severity: Medium]
emac->link is written here from the phylib state machine under
phydev->lock only, while the FPE path gates on it with a single lockless
read in icssg_config_ietfpe():

	if (!READ_ONCE(emac->link)) {
		netdev_dbg(ndev, "cannot change IET/FPE state when interface is down\n");
		return 0;
	}

Can the link drop while icssg_config_ietfpe() is mid-transaction?

CPU0 (tc mqprio or ethtool) holds fpe_lock, observes link == 1, writes
PRE_EMPTION_ENABLE_TX = 1 and PRE_EMPTION_ENABLE_VERIFY = 1, sends
ICSSG_EMAC_PORT_PREMPT_TX_ENABLE and then sleeps in
icssg_iet_verify_wait() for up to ICSSG_IET_VERIFY_ATTEMPTS *
verify_time_ms (384 ms at the default of 128).

CPU1 (phylib work) runs emac_adjust_link():

	WRITE_ONCE(emac->link, 0);
	...
	icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
	icssg_qos_link_state_update(ndev);	/* blocks on fpe_lock */

so CPU0 keeps enabling FPE on a port that firmware has already disabled,
and if PRE_EMPTION_ACTIVE_TX still reads 1 it sets fpe_active = true.

When CPU1 finally takes fpe_lock, icssg_qos_link_state_update() only
clears fpe_active and verify_status and resets the queue maps.  Should it
also clear PRE_EMPTION_ENABLE_TX and PRE_EMPTION_ENABLE_VERIFY, so the
firmware configuration and the driver's view do not diverge?

Also, prueth_emac_common_start() still contains a plain

	emac->link = 0;

store outside both phydev->lock and fpe_lock, so the annotations added
here do not cover every writer.  Was that store meant to be converted
too?

>  
>  	if (emac->link) {

[ ... ]

> @@ -1686,6 +1689,8 @@ static int prueth_netdev_init(struct prueth *prueth,
>  
>  	INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);
>  
> +	icssg_qos_init(ndev);
> +
>  	ret = pruss_request_mem_region(prueth->pruss,
>  				       port == PRUETH_PORT_MII0 ?
>  				       PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1,

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.c b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> new file mode 100644
> index 0000000000000..27a953d981538
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> @@ -0,0 +1,305 @@

[ ... ]

> +static void icssg_iet_set_preempt_mask(struct prueth_emac *emac)
> +{
> +	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
> +	struct tc_mqprio_qopt *qopt = &p_mqprio->qopt;
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int prempt_mask = 0, i;
> +	u8 tc, num_tc;
> +
> +	if (!iet->preemptible_tcs)
> +		goto reset_hw;
> +
> +	if (iet->fpe_active) {
> +		/* Reset all Q_MAP entries first to clear any stale preemptible
> +		 * entries from a prior wider TC mapping.
> +		 */
> +		for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
> +			writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +
> +		/* Configure queues for user requested preemptible tc map */
> +		num_tc = p_mqprio->qopt.num_tc;
> +		for (tc = 0; tc < num_tc; tc++) {
> +			/* check if the tc is preemptive or not */
> +			if (iet->preemptible_tcs & BIT(tc)) {
> +				/* Set the queues as preemptive queues */
> +				for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++) {
> +					writeb(BIT(4),
> +					       config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +				}
> +			} else {
> +				/* Accumulate express queue bits for the mask */
> +				for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++)
> +					prempt_mask |= BIT(i);
> +			}
> +		}
> +		writeb(prempt_mask, config + EXPRESS_PRE_EMPTIVE_Q_MASK);

[Severity: Medium]
prempt_mask starts at zero and only accumulates queues belonging to
non-preemptible traffic classes, but this writeb() replaces the whole
eight-bit queue mask.  Are the queues that no traffic class maps to
handled correctly here?

With the default emac->tx_ch_num == 1 (real_num_tx_queues == 1) and
preemptible_tcs == BIT(0), the sequence programmed is:

	Q_MAP[0]  = BIT(4)	/* preemptible */
	Q_MAP[1..7] = 0		/* express */
	Q_MASK    = 0x00	/* no queue is express */

The reset path a few lines below defines the opposite encoding for the
same registers:

	/* Reset to default: all queues as express */
	for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
		writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
	writeb(ICSSG_EXPRESS_Q_MASK_ALL, config + EXPRESS_PRE_EMPTIVE_Q_MASK);

so Q_MAP == 0 together with Q_MASK == 0xFF means express, while the
active path leaves those same entries at Q_MAP == 0 with their Q_MASK bit
cleared.  Should the bits for queues outside the mqprio map stay set?

Gaps below real_num_tx_queues are reachable as well.
mqprio_validate_queue_counts() in net/sched/sch_mqprio_lib.c only
requires counts to be non-zero, within real_num_tx_queues and
non-overlapping:

	if (qopt->offset[i] >= dev->real_num_tx_queues ||
	    last > dev->real_num_tx_queues) {

It does not require the map to start at queue 0 or to be contiguous.
Queues used outside the TC map (QUEUE_NUM_UNTAGGED,
SPL_PKT_DEFAULT_PRIORITY, and the XDP/XSK path which picks
cpu % emac->tx_ch_num) are the ones that end up with a Q_MAP and Q_MASK
that disagree.

> +		return;
> +	}
> +
> +reset_hw:
> +	/* Reset to default: all queues as express */
> +	for (i = 0; i < ICSSG_MAX_TC_QUEUES; i++)
> +		writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +	writeb(ICSSG_EXPRESS_Q_MASK_ALL, config + EXPRESS_PRE_EMPTIVE_Q_MASK);
> +}

[ ... ]

> +void icssg_qos_init(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	mutex_init(&iet->fpe_lock);
> +	/* Set default values to prevent garbage values during .get_mm() */

[Severity: Low]
This isn't a bug, but this comment (and the similar one in
icssg_qos_link_state_update()) refers to .get_mm(), which is only added
to icssg_ethtool_ops by the following patch in the series.

Within this patch alone nothing writes iet->fpe_enabled or
iet->mac_verify_configure, and mqprio_parse_tc_entries() rejects
TC_FP_PREEMPTIBLE while ethtool_dev_mm_supported() is false, so the
enable side of icssg_config_ietfpe() is not yet reachable.

Could the changelog mention that the MAC Merge uAPI arrives in the next
patch, so the forward reference in these comments is not confusing on its
own?

> +	iet->verify_time_ms = ICSSG_IET_MAX_VERIFY_TIME;
> +	iet->tx_min_frag_size = ETH_ZLEN;
> +}
> +EXPORT_SYMBOL_GPL(icssg_qos_init);
> +
> +static int icssg_iet_change_preemptible_tcs(struct prueth_emac *emac)
> +{
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int ret;
> +
> +	mutex_lock(&iet->fpe_lock);
> +	if (!iet->fpe_enabled && !iet->preemptible_tcs) {
> +		mutex_unlock(&iet->fpe_lock);
> +		return 0;
> +	}
> +	ret = icssg_config_ietfpe(emac->ndev, iet->fpe_enabled);
> +	mutex_unlock(&iet->fpe_lock);
> +
> +	return ret;
> +}

[ ... ]

> +static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
> +{

[ ... ]

> +	if (!qopt->num_tc) {
> +		netdev_reset_tc(ndev);
> +	} else {
> +		netdev_set_num_tc(ndev, qopt->num_tc);
> +
> +		for (tc = 0; tc < qopt->num_tc; tc++) {
> +			count = qopt->count[tc];
> +			offset = qopt->offset[tc];
> +			netdev_set_tc_queue(ndev, tc, count, offset);
> +		}
> +	}
> +
> +	mutex_lock(&iet->fpe_lock);
> +	if (!qopt->num_tc) {
> +		iet->preemptible_tcs = 0;
> +	} else {
> +		memcpy(&p_mqprio->qopt, qopt, sizeof(*qopt));
> +		iet->preemptible_tcs = mqprio->preemptible_tcs;
> +	}
> +	mutex_unlock(&iet->fpe_lock);
> +
> +	netdev_dbg(ndev, "dev->num_tc %u dev->real_num_tx_queues %u\n",
> +		   ndev->num_tc, ndev->real_num_tx_queues);
> +
> +	return icssg_iet_change_preemptible_tcs(emac);

[Severity: Medium]
By the time this runs, the netdev TC mapping, p_mqprio->qopt and
iet->preemptible_tcs have all been committed, and a failure is returned
straight to mqprio_init() with no rollback.

icssg_config_ietfpe() can genuinely fail:

  - icssg_set_port_state() returns -ETIMEDOUT when the firmware R30
    command is not acknowledged within the read_poll_timeout() window
  - the function returns -EIO when PRE_EMPTION_ACTIVE_TX != 1, or when
    icssg_iet_verify_wait() reports ICSSG_IETFPE_STATE_FAILED or times
    out

On that failure path mqprio_init() aborts, and since priv->hw_offload was
never set, mqprio_destroy() takes the non-offload branch:

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 driver is never called back.  Does emac->qos.iet.preemptible_tcs
and emac->qos.mqprio.qopt then describe a qdisc that does not exist?

If so, icssg_qos_link_state_update() -> icssg_iet_change_preemptible_tcs()
will re-apply that state on every subsequent link-up, using offsets and
counts that were validated against a real_num_tx_queues value which may
have changed since (ethtool -L).  Should the software state be rolled
back when the firmware configuration fails?

> +}

[ ... ]

> +void icssg_qos_link_state_update(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int ret;
> +
> +	if (!READ_ONCE(emac->link)) {
> +		/* Clear FPE active state on link-down so get_mm() reports
> +		 * accurate tx_active and verify_status while link is down.
> +		 */
> +		mutex_lock(&iet->fpe_lock);
> +		iet->fpe_active = false;
> +		iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
> +		icssg_iet_set_preempt_mask(emac);
> +		mutex_unlock(&iet->fpe_lock);
> +		return;
> +	}
> +
> +	ret = icssg_iet_change_preemptible_tcs(emac);
> +	if (ret)
> +		netdev_dbg(ndev, "IET FPE %s failed\n",
> +			   str_enable_disable(iet->fpe_enabled));
> +}
> +EXPORT_SYMBOL_GPL(icssg_qos_link_state_update);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915063052.2274986-1-m-malladi%40ti.com