Re: [PATCH net v3 09/14] 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 patch puts the read…
- [Low] The concern is that the kernel-doc of ice_ptp_reset_ts_memory()
  ('Reset timestamp memory for all blocks') no longer matches the code,…

--

ice: E825: clear PHY_TSTAMP memory prior to soft reset

This patch replaces the E825 timestamp memory reset, which wrote
PHY_REG_TX_MEMORY_STATUS and had no effect, with a sweep that reads every
PHY_TSTAMP index on the port. The sweep now runs at the start of
ice_ptp_phy_soft_reset_eth56g(), so the soft reset that follows resets the
internal counter. ice_ptp_reset_ts_memory() no longer does anything for
E825.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dde..38d0ded1d0cc0 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 runs before any of the PHY_REG_GLOBAL accesses that can fail.
What happens to the internal counter if one of those later steps fails?

ice_ptp_clear_tx_memory_status_eth56g() returns void and always reads
every index. After it runs, ice_ptp_phy_soft_reset_eth56g() can still
return early. That can happen at the PHY_REG_GLOBAL read or at any of the
three PHY_REG_GLOBAL writes:

	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
	if (err) {
		...
		return err;
	}
	...
	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, global_val);
	if (err) {
		...
		return err;
	}

The commit message says reading a garbage entry after a cold power-on can
underflow the counter, and only the soft reset sets it back to zero. If
the reset pulse is never issued, or only partly issued, is the port then
left in the state that the new kernel-doc of
ice_ptp_clear_tx_memory_status_eth56g() warns about?

    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.

ice_ptp_init_phc_e825c() then stops at the first port that fails:

	for (int port = 0; port < hw->ptp.num_lports; port++) {
		err = ice_ptp_phy_soft_reset_eth56g(hw, port);
		if (err) {
			...
			return err;
		}
	}

That port stays swept but not reset, and ice_ptp_init_owner() and
ice_ptp_rebuild_owner() only return the error. Before this patch, a failed
soft reset could not cause this desync, because nothing destructive ran
before it.

Later in the series, ice_start_phy_timer_eth56g() also calls
ice_ptp_phy_soft_reset_eth56g() every time the PHY timer starts, so this
path runs more often.

Could the PHY_REG_GLOBAL read be done before the sweep? Alternatively,
could the reset be completed or retried once the sweep has run?

[ ... ]

> @@ -5802,8 +5805,9 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
>  		ice_ptp_reset_ts_memory_e82x(hw);
>  		break;
>  	case ICE_MAC_GENERIC_3K_E825:
> -		ice_ptp_reset_ts_memory_eth56g(hw);
> -		break;
> +		/* E825 hardware must only reset timestamp memory as part of
> +		 * the soft reset procedure.
> +		 */
>  	case ICE_MAC_E810:
>  	default:
>  		return;

[Severity: Low]
This isn't a bug, but the kernel-doc for ice_ptp_reset_ts_memory() still
reads:

 * ice_ptp_reset_ts_memory - Reset timestamp memory for all blocks

E825 is now a no-op here, like the E810 and default (E830) branches. The
only explanation is the inline comment.

Could the kernel-doc list which MAC types are skipped? It could also say
that E825 clears its memory through ice_ptp_phy_soft_reset_eth56g()
instead.

This doesn't change how ice_ptp_cfg_phy_interrupt() behaves. Both
ice_ptp_init_owner() and ice_ptp_rebuild_owner() call ice_ptp_init_phc()
before it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com