Re: [PATCH 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 EXTTS event handler loop drains the hardware timestamp FIFO but drops all but the latest event because the event is dispatched outside the loop.
--

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

This patch replaces the open-coded 64-bit register read/write sequences
with ioread64_lo_hi() and iowrite64_lo_hi() helpers. It introduces two new
macros netc_timer_rd64() and netc_timer_wr64() that wrap these helpers and
uses them throughout the driver to reduce boilerplate.

> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> --- 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));
>  
>  	/* 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);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does this code drop all but the last
external timestamp event?

When the hardware queues multiple external timestamp events, the while loop
in netc_timer_handle_etts_event() pops all valid timestamps from the hardware
into the local variable etts.

Because ptp_clock_event() is called only once outside the loop, aren't all
prior events silently discarded?

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