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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/19/2026 1:14 PM, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 

We can't really do the PHY_REG_GLOBAL before the sweep. The sweep could
screw up the counter and we need to do the soft reset in order to fix
any issues the sweep causes. I'm not sure the sweep would do anything if
we did it while the soft reset is in progress either.

In practice, we do not really expect the soft resets to fail at all,
transient sideband failure isnt something we observe in practice, nor do
I believe it is worth effort to try and harden against such failures
when they likely indicate something catastrophic about the device or system.

This really is just part of the fact that register access is indirect
and "technically" the access can fail. But in practice it should not
fail and I don't believe it makes a lot of sense to add a ton of complex
logic to handle such cases.

We already propagate the error up to the PHY restart and log a dev_err
message indicating that the restart failed, and that the user should try
a link reset.

I think thats good enough.

> [ ... ]
> 
>> @@ -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.
> 

I can try to clean this up.