Re: [PATCH net v4 11/15] net: stmmac: restore TC offloads before restarting DMA

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The patch saves VLAN-priority steering in
  priv->plat->rx_queues_cfg[tc] and relies on stmmac_mtl_configuration()…
- [High] On XGMAC, the runtime TC-to-queue mapping that this patch
  restores is corrupted again before DMA starts.
- [Medium] The patch fixes a real divide-by-zero crash but does not say
  so and has no Fixes: tag.
- [Medium] When tc_taprio_configure() fails after programming the new
  schedule (stmmac_est_configure() or tc_config_preemption() fails), the…
- [Medium] stmmac_tc_restore_filters() walks
  priv->flow_entries[0..flow_entries_max) and…
- [Low] EST replay and DMA start go ahead even when timestamp setup
  failed.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  tc_config_flow() programs port_src and then port_dst into the same…
- [Low] This problem wasn't introduced by this patch, but a protocol-only
  flower rule (e.g. IPv4/UDP DROP with no address or port keys) is…
- [Low] This problem wasn't introduced by this patch, but
  dwmac4_config_l3_filter()/dwmac4_config_l4_filter() (and the XGMAC…

--

net: stmmac: restore TC offloads before restarting DMA

This patch saves L3/L4 flower rules, VLAN priority steering, EtherType
steering, the FPE preemption mapping and the EST schedule so they can be
replayed after a DMA software reset on open and resume. DMA start is
delayed until those offloads have been restored. TAPRIO replacement now
builds the new schedule separately and publishes it only after hardware
programming succeeds.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 618e01772ab86..edcb9a025385b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4348,6 +4350,14 @@ static int __stmmac_open(struct net_device *dev,
>  	stmmac_init_coalesce(priv);
>  
>  	stmmac_vlan_restore(priv);
> +	mutex_lock(&priv->ptp_mutex);
> +	ret = stmmac_tc_restore_est(priv);
> +	mutex_unlock(&priv->ptp_mutex);
> +	if (ret)
> +		goto irq_error;
> +
> +	/* All reset-sensitive offloads must be installed before DMA runs. */
> +	stmmac_start_all_dma(priv);
>  
>  	ret = stmmac_request_irq(dev);
>  	if (ret)
> @@ -8722,10 +8732,14 @@ int stmmac_resume(struct device *dev)
>  		goto error_stop_dma;
>  	}
>  
> -	stmmac_init_timestamping(priv);
>  	mutex_lock(&priv->ptp_mutex);
> -	stmmac_block_ptp(priv, false);
> +	stmmac_init_timestamping(priv);
> +	ret = stmmac_tc_restore_est(priv);
> +	if (!ret)
> +		stmmac_block_ptp(priv, false);
>  	mutex_unlock(&priv->ptp_mutex);
> +	if (ret)
> +		goto error_stop_dma;

[Severity: Low]
The commit message says "Restore EST after the timestamp counter is
usable". The comment above stmmac_tc_restore_est() also says it is called
after timestamp setup. Does either caller actually enforce that?

In stmmac_resume(), the return value of stmmac_init_timestamping() is
ignored before stmmac_tc_restore_est() runs. Also,
stmmac_init_tstamp_counter() throws away the result of
stmmac_init_systime(), so a TSINIT timeout is reported as success.

In __stmmac_open(), stmmac_setup_ptp() returns void. It returns early if
clk_prepare_enable() fails for clk_ptp_ref, but stmmac_tc_restore_est()
and stmmac_start_all_dma() still run afterwards.

stmmac_get_systime() cannot fail when the callback exists. In these cases,
won't the replay compute a base time from a counter that was never
initialized, arm EST, and report success?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 9ded65750e89f..de254919f6ab7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -577,23 +559,46 @@ static int tc_add_ports_flow(struct stmmac_priv *priv,

[ ... ]

> +static int tc_config_flow(struct stmmac_priv *priv,
> +			  const struct stmmac_flow_entry *entry)
> +{
> +	bool inv = entry->action & STMMAC_FLOW_ACTION_DROP;
> +	bool udp = entry->ip_proto == IPPROTO_UDP;
> +	int ret;
> +
> +	/* Clear the whole slot, including matches removed by a replacement. */
> +	ret = stmmac_config_l3_filter(priv, priv->hw, entry->idx, false,
> +				      false, false, false, 0);

[Severity: Low]
This isn't a bug introduced by this patch, but dwmac4_config_l3_filter()
and dwmac4_config_l4_filter() do an unlocked read-modify-write of
MAC_PACKET_FILTER to set IPFE. The XGMAC equivalents do the same:

    value = readl(ioaddr + GMAC_PACKET_FILTER);
    value |= GMAC_PACKET_FILTER_IPFE;
    writel(value, ioaddr + GMAC_PACKET_FILTER);

tc_add_flow() holds only RTNL. The rx-mode set_filter path writes the same
register under the address lock.

Could a concurrent rx-mode update lose the IPFE bit? This clear-slot call
adds one more read-modify-write per add or replace, but the race itself
is unchanged.

> +	if (ret || !entry->in_use)
> +		return ret;

[ ... ]

> +	if (entry->port_src) {
>  		ret = stmmac_config_l4_filter(priv, priv->hw, entry->idx, true,
> -					      is_udp, false, inv, hw_match);
> +					    udp, true, inv, entry->port_src);
>  		if (ret)
>  			return ret;
>  	}
> -
> -	entry->is_l4 = true;
> +	if (entry->port_dst)
> +		return stmmac_config_l4_filter(priv, priv->hw, entry->idx, true,
> +					      udp, false, inv, entry->port_dst);
>  	return 0;
>  }

[Severity: Medium]
This is a pre-existing issue, but on dwmac4, is the source port match
still there after port_src and then port_dst are programmed into the same
slot?

Every call to dwmac4_config_l4_filter() clears both enables and rewrites
the whole GMAC_L4_ADDR register:

    value &= ~(GMAC_L4SPM0 | GMAC_L4SPIM0);
    value &= ~(GMAC_L4DPM0 | GMAC_L4DPIM0);
    ...
    writel(value, ioaddr + GMAC_L4_ADDR(filter_no));

As a result, a flower rule with both src and dst ports seems to match only
the dst port. That happens at install time and, with this patch, on every
reset replay too. The old tc_add_ports_flow() used the same call sequence.

[ ... ]

> @@ -637,23 +643,33 @@ static int tc_add_flow(struct stmmac_priv *priv,
>  			return -ENOENT;
>  	}
>  
> -	ret = tc_parse_flow_actions(priv, &rule->action, entry,
> +	new.idx = entry->idx;
> +	ret = tc_parse_flow_actions(priv, &rule->action, &new,
>  				    cls->common.extack);
>  	if (ret)
>  		return ret;
>  
>  	for (i = 0; i < ARRAY_SIZE(tc_flow_parsers); i++) {
> -		ret = tc_flow_parsers[i].fn(priv, cls, entry);
> +		ret = tc_flow_parsers[i].fn(priv, cls, &new);
>  		if (!ret)
> -			entry->in_use = true;
> +			new.in_use = true;
>  		else if (ret == -EOPNOTSUPP)
>  			return ret;
>  	}
>  
> -	if (!entry->in_use)
> +	if (!new.in_use)
>  		return -EINVAL;
>  
> -	entry->cookie = cls->cookie;
> +	ret = tc_config_flow(priv, &new);

[Severity: Low]
This isn't a bug introduced by this patch, but what happens with a
protocol-only rule, for example an IPv4/UDP drop with no address or port
keys?

tc_add_basic_flow() records only ip_proto and returns 0, which sets
new.in_use. The IPv4 and ports parsers return -EINVAL, and that is ignored
here.

tc_config_flow() then clears the slot, enables no match and returns 0.
The cookie is published and the rule is reported as offloaded, but it has
no effect in hardware. The baseline had the same false success.

[ ... ]

> @@ -739,6 +755,8 @@ static int tc_add_vlan_flow(struct stmmac_priv *priv,
>  
>  		prio = BIT(match.key->vlan_priority);
>  		stmmac_rx_queue_prio(priv, priv->hw, prio, tc);
> +		priv->plat->rx_queues_cfg[tc].prio = prio;
> +		priv->plat->rx_queues_cfg[tc].use_prio = true;
>  
>  		entry->in_use = true;
>  		entry->cookie = cls->cookie;
> @@ -760,6 +778,8 @@ static int tc_del_vlan_flow(struct stmmac_priv *priv,
>  
>  	if (stmmac_tc_active(priv))
>  		stmmac_rx_queue_prio(priv, priv->hw, 0, entry->tc);
> +	priv->plat->rx_queues_cfg[entry->tc].prio = 0;
> +	priv->plat->rx_queues_cfg[entry->tc].use_prio = true;
>  
>  	entry->in_use = false;
>  	entry->cookie = 0;

[Severity: High]
Can tc go past the end of rx_queues_cfg[] here?

rx_queues_cfg[] has MTL_MAX_RX_QUEUES (8) entries, and tx_queues_cfg[]
follows it in struct plat_stmmacenet_data. tc comes from
tc_classid_to_hwtc(), which only guarantees tc < netdev_get_num_tc().

taprio in txtime-assist mode allows overlapping TXQ ranges and can set up
to TC_MAX_QUEUE (16) traffic classes. A rule with "vlan_prio N hw_tc 8..15
skip_sw" would then write into tx_queues_cfg[] (CBS slopes, weights,
prio). tc_del_vlan_flow() uses entry->tc as the index in the same way.

Before this patch, tc_add_vlan_flow() only called stmmac_rx_queue_prio()
and did not write to the array.

Separately, does the saved copy match what the hardware holds?
dwmac4_rx_queue_priority() and dwxgmac2_rx_queue_prio() OR the new bit
into the target queue's PSRQ field and clear it from the other queues.
The saved value, however, is overwritten:

    priv->plat->rx_queues_cfg[tc].prio = prio;

With two VLAN priority rules on the same tc, both bits are set in
hardware, but only the last one would be replayed after a reset.

In tc_del_vlan_flow(), setting prio = 0 for the whole tc also drops any
other VLAN rule still installed on that tc. It also permanently removes
the board or DT priority for that queue.

A rule can also move priority p from DT queue r to queue q. In that case
rx_queues_cfg[r].prio still contains p. stmmac_mac_config_rx_queues_prio()
replays queues in ascending order. When r > q, wouldn't replaying r take
p back from q?

In addition, stmmac_mtl_configuration() only replays priorities for
queues below rx_queues_to_use, and only when rx_queues_to_use > 1:

    if (rx_queues_count > 1)
        stmmac_mac_config_rx_queues_prio(priv);

stmmac_tc_restore_filters() skips STMMAC_RFS_T_VLAN on the assumption
that stmmac_mtl_configuration() replays it. After open or resume, won't
the RX VLAN priority steering then differ from the installed flower
rules?

> @@ -934,6 +954,45 @@ static int tc_setup_cls(struct stmmac_priv *priv,
>  	return ret;
>  }
>  
> +int stmmac_tc_restore_filters(struct stmmac_priv *priv)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < priv->flow_entries_max; i++) {
> +		struct stmmac_flow_entry *entry = &priv->flow_entries[i];
> +
> +		if (!entry->in_use)
> +			continue;

[Severity: Medium]
Can priv->flow_entries be NULL here while flow_entries_max is non-zero?

tc_init() sets flow_entries_max from dma_cap->l3l4fnum before calling
devm_kcalloc(). If the allocation fails, it returns -ENOMEM with the
count still set. tc_rfs_init() does the same with rfs_entries_total and
rfs_entries.

__stmmac_dvr_probe() ignores the error:

    ret = stmmac_tc_init(priv, priv);
    if (!ret) {
        ndev->hw_features |= NETIF_F_HW_TC;
    }

Before this patch, these tables were only reached through flower
offload, which needs NETIF_F_HW_TC. Now stmmac_hw_setup() calls
stmmac_tc_restore_filters() on every open and resume. Wouldn't that
dereference NULL on every open and resume?

The RX parser replay nearby is already guarded with if (priv->tc_entries).

> +		ret = tc_config_flow(priv, entry);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

> +	/* The XGMAC callback also restores the runtime TC-to-queue mapping. */
> +	if (priv->fpe_cfg.mapping_configured)
> +		return tc_config_preemption(priv, NULL,
> +					    priv->fpe_cfg.preemptible_tcs);
> +	return 0;
> +}

[Severity: High]
On XGMAC, is this mapping still intact when DMA starts?

dwxgmac3_fpe_map_preemption_class() writes Q2TCMAP = tc using
u32_replace_bits(). However, stmmac_hw_setup() calls
stmmac_dma_operation_mode() after stmmac_tc_restore_filters(), and
dwxgmac2_dma_tx_mode() ORs in the channel without clearing the field:

    /* Use static TC to Queue mapping */
    value |= FIELD_PREP(XGMAC_Q2TCMAP, channel);

That would leave Q2TCMAP = tc | channel. For example, with mqprio 2@0 2@2,
queue 1 would move from TC 0 to TC 1, and queues 2 and 3 would move from
TC 1 to TC 3, which does not exist.

Should the replay run after stmmac_dma_operation_mode(), or should
dwxgmac2_dma_tx_mode() clear the field first?

[ ... ]

> @@ -1040,34 +1130,27 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (qopt->cycle_time_extension >= BIT(wid + 7))
>  		return -ERANGE;
>  
> -	if (!priv->est) {
> -		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> -					 GFP_KERNEL);
> -		if (!priv->est)
> -			return -ENOMEM;
> -
> -		mutex_init(&priv->est_lock);
> -	} else {
> -		mutex_lock(&priv->est_lock);
> -		memset(priv->est, 0, sizeof(*priv->est));
> -		mutex_unlock(&priv->est_lock);
> -	}
> +	/* Build the replacement without changing the installed schedule. An
> +	 * entry rejected below must not leave an enabled, zero-cycle cache for
> +	 * PHC adjustment or reset replay to consume.
> +	 */
> +	est = kzalloc_obj(*est);
> +	if (!est)
> +		return -ENOMEM;
>  
>  	size = qopt->num_entries;
> -
> -	mutex_lock(&priv->est_lock);
> -	priv->est->gcl_size = size;
> -	priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
> -	mutex_unlock(&priv->est_lock);
> +	est->gcl_size = size;
> +	est->enable = true;

[Severity: Medium]
This change fixes a divide-by-zero in the baseline, but the commit
message only says:

  "A rejected first install must not leave an enabled zero-cycle cache
  for PHC adjustment or reset replay."

In the baseline, every TAPRIO_CMD_REPLACE cleared priv->est with memset()
and set enable before the gate list was validated. An entry with an
out-of-range interval (for example more than 65535 ns with wid=16)
returned -ERANGE and left enable set with ctr[] still zero.
taprio_change() then keeps the previous qdisc and does not send DESTROY.

The next PHC step then reaches this code in stmmac_adjust_time():

    cycle_time = (u64)priv->est->ctr[1] * NSEC_PER_SEC + priv->est->ctr[0];
    time = stmmac_calc_tas_basetime(basetime, current_time_ns, cycle_time);

That ends up calling div64_s64() with a zero divisor. It affects any
rejected replace, not only the first install.

Could this fix be split into its own patch with a Fixes: tag and a commit
message that describes the crash, so it can go to stable without the rest
of the series?

[ ... ]

> +	*priv->est = *est;
> +	mutex_unlock(&priv->est_lock);
> +free_est:
> +	kfree(est);
> +	return ret;
> +
> +restore:
> +	/* A failed hardware update must not publish the rejected schedule. */
> +	if (stmmac_est_configure(priv, priv, priv->est, priv->plat->clk_ptp_rate))
> +		netdev_err(priv->dev, "failed to restore EST\n");
> +	mutex_unlock(&priv->est_lock);
> +	goto free_est;

[Severity: Medium]
Is priv->est->btr[] still safe to write back here?

It was computed when the old schedule was installed, or at the last PHC
adjust or reset replay. It is not rebased against the current PHC time
here. stmmac_tc_restore_est() and stmmac_adjust_time() both recompute it
from btr_reserve[] with stmmac_calc_tas_basetime().

est_configure() writes cfg->btr[] directly to EST_BTR_LOW/HIGH and sets
EEST | SSWL. The EST block only accepts a past BTR for a limited number
of cycles (EST_GMAC5_BTRL allows 15). Beyond that it raises BTRE and does
not switch lists.

User space can reach this path. One example is when
dwmac5_fpe_map_preemption_class() rejects the SP algorithm with
one-to-many TC mapping after EST has already been programmed.

In that case, could the hardware keep running the rejected schedule while
priv->est and the returned error say the old schedule is still active?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926-submit-stmmac-reset-fixes-v1-v4-0-ec1c0250b3c9%40gmail.com