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

[James Clark]

> [Severity: Medium]
>
> Does the calibration behind this value account for any latency in the
> PHY's external input capture path compared with the GPIO?

[ ... ]

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

[ ... ]

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

Let's suppose there is a delay D of some small number of nanoseconds
between the time the pulse occurs, Tp, and the time of capture, Tc. The
purpose of PTP_SYS_OFFSET_EXTENDED is to allow transfer of time between
the PHC and the system clock. The bounds it provides tell you the
accuracy of that transfer. In the case where the accuracy matters most,
the PHC is being disciplined using timestamps from a GNSS receiver's PPS
signal, which means the PHC time corresponds to Tc not Tp. So I would
argue calculating bounds w.r.t. Tc is the right thing to do, and the
350 ns figure is correct for that.

In any case, D is not practically observable. On the CM4/CM5 there is
only one pin, which can be either an input or an output pin. The packet
timestamps are not calibrated: path delay shows the uncorrected sum of
the ingress and egress latency is around 280 ns, and the only way to
calibrate the split into egress and ingress is to use PPS input or
output.

> [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:

[ ... ]

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

[ ... ]

> [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:

[ ... ]

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

I agree that both are bugs, but they are in the existing framesync code
and unrelated to this patch. The patch changes only the trigger write;
the polling loop and the perout path are unchanged, and as the commit
message says, it doesn't try to fix that code's error handling.

James