Re: [PATCH net-next v2 4/6] net: phy: broadcom: use timestamped MDIO writes in gettimex64

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 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).

- [Medium] The calibration behind BCM_FRAMESYNC_CAPTURE_NS assumes the
  PHY's external SYNC_IN capture path and the GPIO observation path see…
- [Low] Claimed problem: no MDIO bus driver implements write_sts, so the
  new bcm_ptp_framesync_ts() branch (`sts &&…
- [Low] The new comment on BCM_FRAMESYNC_CAPTURE_NS in
  drivers/net/phy/bcm-phy-ptp.c measures the capture delay from 'the…

Pre-existing issues:
- [Medium] In bcm_ptp_framesync_ts(), `reg` is a u16 holding the int
  result of bcm_phy_read_exp(phydev, INTR_STATUS).
- [Medium] bcm_ptp_perout_work() declares `struct timespec64 ts;` without
  an initializer, ignores the return value of…

--

net: phy: broadcom: use timestamped MDIO writes in gettimex64

This commit adds bcm_phy_write_exp_sts(). bcm_ptp_framesync_ts() uses it
when the MDIO bus provides write_sts, so the gettimex64 system timestamps
bracket completion of the write that triggers the framesync. A 350 ns
BCM_FRAMESYNC_CAPTURE_NS bound, found by measurement, is passed as the
event delay to cover the PHY's capture latency.

> diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
> index 65d609ed69fb4..61593dd2dbc0a 100644
> --- a/drivers/net/phy/bcm-phy-ptp.c
> +++ b/drivers/net/phy/bcm-phy-ptp.c
> @@ -161,6 +161,11 @@ struct bcm_ptp_capture {
>  #define BCM_MAX_PULSE_8NS	((1U << 9) - 1)
>  #define BCM_MAX_PERIOD_8NS	((1U << 30) - 1)
>  
> +/* Allowed for the PHY to capture its time after the last MDC edge of
> + * the framesync write.
> + */
> +#define BCM_FRAMESYNC_CAPTURE_NS	350

[Severity: Low]

Is "the last MDC edge of the framesync write" the same reference point
that __mdiobus_write_sts() uses for event_delay_ns? The kernel-doc there
says:

drivers/net/phy/mdio_bus.c:__mdiobus_write_sts()
 * timestamps bounding completion; a transfer is considered complete on
 * the rising edge of the MDC that clocks the last data bit. The upper
 * bound is moved @event_delay_ns later, so that the bounds also contain
 * an event that follows completion.

A clause 22 frame has a falling MDC edge half a period after that rising
edge, and free-running controllers keep clocking after it. At the MDC
periods in the commit message, "the last MDC edge" could therefore be
read as a point 50 to 200 ns later.

The 350 ns value looks like it was calibrated against the rising-edge
completion bounds that the later macb and unimac write_sts patches use,
so the constant itself appears consistent with the core.

Could the comment be reworded to match the __mdiobus_write_sts()
definition? Otherwise someone re-deriving the constant from the comment
could end up with a value that is too small.

[Severity: Medium]

Does the calibration behind this value account for any latency in the
PHY's external input capture path compared with the GPIO? The commit
message says:

    The same PPS signal was wired both to
    the SYNC_OUT pin connected to the PHY's PHC and to GPIO pin 18.
    [ ... ]
    Combining these gives a PHC/system time pair for each pulse.

The PHC side of each pair comes from bcm_ptp_extts_work(), which reports
the raw heartbeat capture with no latency correction.

Suppose the PHY latches the PHC D ns after the true edge and the GPIO
sees the edge G ns after. Then every interpolated capture time would be
early by (D - G), and the true bound would be the measured one plus
(D - G).

The polling bracket compensation and the 8 ns phase allowance don't
cover this fixed path offset. The CM5 cross-check uses the same PHY
input path, so it would have the same bias. That leaves about 12 ns of
margin (350 - 330 - 8). A two or three flop synchronizer in a 125 MHz
domain would be 16 to 24 ns.

__mdiobus_write_sts() extends post_sts by exactly the value passed in:

    ptp_adjust_system_postts(sts, event_delay_ns);

So an underestimate here would quietly give PTP_SYS_OFFSET_EXTENDED
bounds that don't contain the PHC capture. The size of D - G is a
hardware property that can't be checked from the code.

Is this input latency known or bounded? If not, would a larger margin
be safer?

> +
>  #define BRCM_PHY_MODEL(phydev) \
>  	((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
>  
> @@ -214,22 +219,35 @@ static void bcm_ptp_framesync(struct phy_device *phydev, u16 ctrl)
>  	bcm_phy_write_exp(phydev, NSE_CTRL, ctrl | NSE_CPU_FRAMESYNC);
>  }
>  
> +static int bcm_ptp_framesync_sts(struct phy_device *phydev, u16 ctrl,
> +				 struct ptp_system_timestamp *sts)
> +{
> +	return bcm_phy_write_exp_sts(phydev, NSE_CTRL,
> +				     ctrl | NSE_CPU_FRAMESYNC,
> +				     BCM_FRAMESYNC_CAPTURE_NS, sts);
> +}
> +
>  static int bcm_ptp_framesync_ts(struct phy_device *phydev,
>  				struct ptp_system_timestamp *sts,
>  				struct timespec64 *ts,
>  				u16 orig_ctrl)
>  {
>  	u16 ctrl, reg;
> -	int i;
> +	int i, err = 0;
>  
>  	ctrl = bcm_ptp_framesync_disable(phydev, orig_ctrl);
>  
> -	ptp_read_system_prets(sts);
> -
>  	/* trigger framesync + capture */
> -	bcm_ptp_framesync(phydev, ctrl | NSE_CAPTURE_EN);
> -
> -	ptp_read_system_postts(sts);
> +	if (sts && bcm_phy_supports_write_exp_sts(phydev)) {

[Severity: Low]

At this commit, does any MDIO bus driver set write_sts? If none does,
mdiobus_supports_write_sts() always returns false and this branch can't
run. The 350 ns bound would also depend on bus driver timing that isn't
in the tree yet.

This appears to be resolved later in the series. "net: macb: implement
timestamped MDIO writes" adds:

    bp->mii_bus->write_sts = &macb_mdio_write_c22_sts;

and "net: mdio: bcm-unimac: implement timestamped MDIO writes" adds:

    bus->write_sts = unimac_mdio_write_sts;

So this is only a note about patch ordering in the series.

> +		/* cannot ignore error since sts may be uninitialized,
> +		 * but still poll for any triggered capture
> +		 */
> +		err = bcm_ptp_framesync_sts(phydev, ctrl | NSE_CAPTURE_EN, sts);
> +	} else {
> +		ptp_read_system_prets(sts);
> +		bcm_ptp_framesync(phydev, ctrl | NSE_CAPTURE_EN);
> +		ptp_read_system_postts(sts);
> +	}
>  
>  	/* poll for FSYNC interrupt from TS capture */
>  	for (i = 0; i < 10; i++) {

[Severity: Medium]

This isn't a bug introduced by this patch, but reg is a u16 that holds
the int result of bcm_phy_read_exp() in this loop:

	for (i = 0; i < 10; i++) {
		reg = bcm_phy_read_exp(phydev, INTR_STATUS);
		if (reg & INTC_FSYNC) {
			bcm_ptp_get_framesync_ts(phydev, ts);
			break;
		}
	}

Can a negative MDIO error look like a completed capture here? -EIO
truncated to u16 is 0xfffb, and that has INTC_FSYNC (BIT(0)) set.

bcm_ptp_get_framesync_ts() then stores the unchecked bcm_phy_read_exp()
results in u16 hb[]. If the trigger write succeeded, the function
returns 0 with a bogus timestamp.

The new error check only covers the timestamped trigger write, and the
commit message says the existing framesync error handling is out of
scope. This may be better fixed separately.

> @@ -242,6 +260,9 @@ static int bcm_ptp_framesync_ts(struct phy_device *phydev,
>  
>  	bcm_ptp_framesync_restore(phydev, orig_ctrl);
>  
> +	if (err)
> +		return err;
> +
>  	return reg & INTC_FSYNC ? 0 : -ETIMEDOUT;

[Severity: Medium]

This is a pre-existing issue, but *ts is never written on the -ETIMEDOUT
path. bcm_ptp_perout_work() ignores the return value and uses ts anyway:

drivers/net/phy/bcm-phy-ptp.c:bcm_ptp_perout_work() {
	struct timespec64 ts;
	...
	bcm_ptp_framesync_ts(phydev, NULL, &ts, priv->nse_ctrl);

	/* this is 1PPS only */
	next = NSEC_PER_SEC - ts.tv_nsec;
	...
	bcm_phy_write_exp(phydev, SYNOUT_TS_0, ns & 0xfff0);
	bcm_phy_write_exp(phydev, SYNOUT_TS_1, ns >> 16);
	bcm_phy_write_exp(phydev, SYNOUT_TS_2, ns >> 32);
	...
}

If the INTC_FSYNC poll uses up all 10 iterations, wouldn't uninitialized
stack data be written to the SYNOUT_TS registers?

This caller passes sts == NULL, so it always takes the unchanged else
branch. Its behaviour is the same before and after this patch.

>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018100257.566267-1-jjc%40jclark.com