Re: [PATCH v2 net-next 1/7] ptp: netc: use ioread64_lo_hi/iowrite64_lo_hi for 64-bit register access

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

Pre-existing issues:
- [High] The external timestamp event handler loop drains the hardware registers but only publishes the last event, silently dropping all preceding valid hardware timestamps. This problem wasn't introduced by this patch, but it remains a significant logic error.
--

--- Patch [1]: [PATCH v2 net-next 1/7] ptp: netc: use ioread64_lo_hi/iowrite64_lo_hi for 64-bit register access ---
commit 20c67a6850880b0f14f8d15d3feff060e9651e26
Author: Wei Fang <[email protected]>

ptp: netc: use ioread64_lo_hi/iowrite64_lo_hi for 64-bit register access

This patch refactors the 64-bit register accesses in the ptp_netc driver by
using the standard ioread64_lo_hi and iowrite64_lo_hi helpers. It introduces
netc_timer_rd64 and netc_timer_wr64 macros to reduce boilerplate and replace
open-coded register sequences.

> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> index 1c20d7efab929..0e0972596d880 100644
> --- a/drivers/ptp/ptp_netc.c
> +++ b/drivers/ptp/ptp_netc.c
[ ... ]
> @@ -497,22 +470,19 @@ static void netc_timer_handle_etts_event(struct netc_timer *priv, int index,
>  					 bool update_event)
>  {
>  	struct ptp_clock_event event;
> -	u32 etts_l = 0, etts_h = 0;
> +	u64 etts = 0;
>  
> -	while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index)) {
> -		etts_l = netc_timer_rd(priv, NETC_TMR_ETTS_L(index));
> -		etts_h = netc_timer_rd(priv, NETC_TMR_ETTS_H(index));
> -	}
> +	while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index))
> +		etts = netc_timer_rd64(priv, NETC_TMR_ETTS_L(index));

[Severity: High]
This isn't a bug introduced by this patch, but does this loop
unintentionally drop valid hardware timestamps?

In drivers/ptp/ptp_netc.c:netc_timer_handle_etts_event(), when multiple
timestamp events are latched in the hardware before the interrupt is fully
serviced, the while loop drains all pending events from the hardware
registers but only saves the final read into the local etts variable.

Since ptp_clock_event() is called outside the while loop, all preceding
timestamps appear to be silently discarded. Should ptp_clock_event() be
called from inside the loop to ensure every captured timestamp is reported
to the PTP subsystem?

>  
>  	/* Invalid time stamp */
> -	if (!etts_l && !etts_h)
> +	if (!etts)
>  		return;
>  
>  	if (update_event) {
>  		event.type = PTP_CLOCK_EXTTS;
>  		event.index = index;
> -		event.timestamp = (u64)etts_h << 32;
> -		event.timestamp |= etts_l;
> +		event.timestamp = etts;
>  		ptp_clock_event(priv->clock, &event);
>  	}
>  }

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