Re: [PATCH net v4 05/15] net: stmmac: serialize and retain PHC configuration across reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: stmmac: divide by zero in stmmac_ptp_restore() on single-shot PEROUT
- [Medium] net: stmmac: frequency adjustment lost in stmmac_dl_ts_coarse_set()
--

commit 04a85bc09b8c93cccea45ce07b1d943738de8a9b
Author: James Hilliard <james.hilliard1@gmail.com>
Subject: net: stmmac: serialize and retain PHC configuration across reset

This commit retains PHC configuration such as frequency correction, PEROUT
requests, and EXTTS selection across hardware resets. It serializes
timestamp writers and devlink mode updates using a new mutex, and adds a
common replay helper for reset transactions.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -7708,6 +7722,8 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
>  	 * reconfigure the systime, subsecond increment and addend.
>  	 */
>  	stmmac_update_subsecond_increment(priv);
> +	write_unlock_irqrestore(&priv->ptp_lock, flags);
> +	mutex_unlock(&priv->ptp_mutex);
>  
>  	return 0;
>  }

[Severity: Medium]
Does this code lose the currently active frequency adjustment when the devlink
coarse mode is changed?

When a user sets the devlink phc_coarse_adj parameter,
stmmac_dl_ts_coarse_set() calls stmmac_update_subsecond_increment().
Looking at stmmac_update_subsecond_increment(), it reconfigures the hardware
with priv->default_addend without accounting for the active adjustment:

stmmac_update_subsecond_increment() {
    ...
    priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
    stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
}

Since it does not reapply the cached priv->ptp_scaled_ppm, would this cause
the hardware to run unadjusted while the cached ptp_scaled_ppm implies to
the PTP daemon that an active adjustment is still present?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
[ ... ]
> +int stmmac_ptp_restore(struct stmmac_priv *priv)
> +{
[ ... ]
> +	for (i = 0; !ret && i < STMMAC_PPS_MAX; i++) {
> +		struct stmmac_pps_cfg cfg = priv->pps[i];
> +
> +		if (!(priv->ptp_perout & BIT(i)))
> +			continue;
> +		stmmac_get_systime(priv, priv->ptpaddr, &ns);
> +		period = timespec64_to_ns(&cfg.period);
> +		/* Retain phase, but move an expired target into the future. */
> +		cfg.start = stmmac_calc_tas_basetime(timespec64_to_ktime(cfg.start),
> +						     ns + PTP_SAFE_TIME_OFFSET_NS, period);

[Severity: High]
Can this cause a division by zero panic when restoring a single-shot PEROUT
request?

If a user configures a single-shot PTP PEROUT pulse, its period is 0.
When a MAC reset occurs later (such as an MTU change), stmmac_ptp_restore()
attempts to replay the PEROUT request and passes the period (0) as the
cycle_time argument to stmmac_calc_tas_basetime().

Looking at stmmac_calc_tas_basetime() in stmmac_tc.c:

struct timespec64 stmmac_calc_tas_basetime(...) {
    ...
    } else {
        s64 n;
        ktime_t base_time;

        n = div64_s64(ktime_sub_ns(current_time, old_base_time),
                      cycle_time);

If the target time is in the past, taking this else branch with
cycle_time = 0 will trigger a 64-bit divide error. Should single-shot
requests skip this phase recalculation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926-submit-stmmac-reset-fixes-v1-v4-0-ec1c0250b3c9@gmail.com?part=5