Re: [PATCH net v5 09/13] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In ice_ptp_phy_soft_reset_eth56g()
  (drivers/net/ethernet/intel/ice/ice_ptp_hw.c), the read sweep…

--

ice: E825: clear PHY_TSTAMP memory prior to soft reset

This commit removes the write-based ice_ptp_reset_ts_memory_eth56g(). In
its place, a new ice_ptp_clear_tx_memory_status_eth56g() reads every
PHY_TSTAMP index for a port. That sweep is now called at the start of
ice_ptp_phy_soft_reset_eth56g(), so the soft reset clears the internal Tx
timestamp counter after the reads. E825 is also dropped from
ice_ptp_reset_ts_memory().

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dd..5637925c0e68 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> @@ -2311,6 +2312,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>  	u32 global_val;
>  	int err;
>  
> +	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> +
>  	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
>  	if (err) {
>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",

[Severity: Low]
The sweep has no return value and always runs before the PHY_REG_GLOBAL
read and the three PHY_REG_GLOBAL writes. Any of those sideband accesses
can fail and make ice_ptp_phy_soft_reset_eth56g() return early. If that
happens, the port has been swept but its internal counter was never
reset.

The new kdoc for ice_ptp_clear_tx_memory_status_eth56g() says:

  Due to interactions with an internal HW counter for the number of
  outstanding Tx timestamps, this *must* only be called as part of the
  ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
  may become out of sync and prevent new timestamp interrupts.

Doesn't this error path leave the port in the state that comment warns
about?

I could not find any visible effect from this today:

- ice_ptp_init_phc_e825c() stops at the first port that fails.
- The error reaches ice_ptp_init_owner() or ice_ptp_rebuild_owner(), and
  both treat it as fatal for PTP.
- Recovery paths, such as a link toggle or a driver reload, run the sweep
  and the soft reset again.

So this looks like a robustness and documentation mismatch rather than a
functional regression.

Could PHY_REG_GLOBAL be read before the sweep, so that a failure at that
first step happens before the counter is touched? Alternatively, the kdoc
could be updated to cover the error path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com