Re: [PATCH 15/20] net: dsa: xilinx: register PHC backed by the RTC timer block

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The generated PTP_CLOCK_PPS event is not phase-aligned to the integer second boundary of the PHC, violating the PPS requirement.
- [Medium] The PTP clock character device is exposed to userspace before its software state and hardware time are initialized, leading to a race condition.
--

--- Patch [15]: [PATCH 15/20] net: dsa: xilinx: register PHC backed by the RTC timer block ---
commit ffe197a5a866d50ba55dd44d6f08fb6883f25a39
Author: Nagadheeraj Rottela <[email protected]>

net: dsa: xilinx: register PHC backed by the RTC timer block

Bring the IP-wide RTC timer block up under the switch driver and
expose it as a PTP hardware clock. The timer block sits at a fixed
offset inside MAC1's per-MAC register window, which the switch driver
already owns via the "mac1" reg-name. Map it in probe() and register
the PHC in setup().

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c b/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c
> new file mode 100644
> index 0000000000000..75c177f752f1b
> --- /dev/null
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn_ptp.c

[ ... ]

> +static irqreturn_t xlnx_tsn_ptp_timer_isr(int irq, void *priv)
> +{
> +	struct ptp_clock_event event = { .type = PTP_CLOCK_PPS };
> +	struct xlnx_tsn *sw = priv;
> +
> +	sw->countpulse++;
> +	if (sw->countpulse >= TSN_TIMER_PULSES_PER_PPS) {
> +		sw->countpulse = 0;
> +		if (sw->ptp_clock && READ_ONCE(sw->pps_enable))
> +			ptp_clock_event(sw->ptp_clock, &event);
> +	}

[Severity: High]
Is the generated PTP_CLOCK_PPS event in xlnx_tsn_ptp_timer_isr() phase-aligned
to the integer second boundary of the PHC?

The hardware timer block fires 128 ticks per second. The ISR increments
a free-running counter (countpulse) and emits a PTP_CLOCK_PPS event every
128th tick. Because countpulse starts at 0 during driver load and has no
correlation with the PHC's time-of-day (when tv_nsec == 0), the PPS event
does not appear to indicate a new second boundary.

Furthermore, if the PHC time is changed via settime or adjtime, the
countpulse phase doesn't appear to be adjusted, leaving the PPS signal
decoupled from the PHC time.

[ ... ]

> +	sw->ptp_clock = ptp_clock_register(&sw->ptp_clock_info, sw->dev);
> +	if (IS_ERR_OR_NULL(sw->ptp_clock)) {
> +		ret = sw->ptp_clock ? PTR_ERR(sw->ptp_clock) : -ENODEV;
> +		sw->ptp_clock = NULL;
> +		return dev_err_probe(sw->dev, ret,
> +				     "failed to register PTP clock\n");
> +	}
> +
> +	sw->rtc_value = div_u64(NSEC_PER_SEC, TSN_TIMER_GTX_CLK_FREQ) <<
> +			TSN_TIMER_RTC_NS_SHIFT;
> +	mac_iow(&sw->mac[XLNX_TSN_PORT_MAC1], TSN_TIMER_RTC_INCREMENT,
> +		(u32)sw->rtc_value);
> +
> +	ts = ktime_to_timespec64(ktime_get_real());
> +	xlnx_tsn_ptp_settime(&sw->ptp_clock_info, &ts);

[Severity: Medium]
Could the PTP clock character device be accessed before its software state
and hardware time are initialized in xlnx_tsn_ptp_init()?

ptp_clock_register() creates the /dev/ptpX character device and exposes it to
userspace. Since this happens before sw->rtc_value and the hardware time are
initialized, a userspace process could theoretically open the device and read
uninitialized hardware time.

Additionally, adjustments (e.g., via adjfine or settime) could be calculated
incorrectly since sw->rtc_value is still 0, and then immediately overwritten
by the remaining initialization sequence.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15