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

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

On 10/25/2026 4:44 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 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.
> 


We can update the kdoc to better clarify. The intention is that if any
of the writes during the restart fail we will error out to the top level
of the PHY restart and log an error that a link restart is necessary,
rather than trying to implment more complex restart or other error
handling logic in the lower level of the driver.