Re: [PATCH net 2/3] ice: E825: clear PHY_TSTAMP memory prior to soft reset

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

On 10/30/2026 2:40 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] The loop in ice_ptp_clear_tx_memory_status_eth56g() (`for (idx =
>   0; idx < INDEX_PER_PORT; idx++)`, INDEX_PER_PORT = 64) only counts…
> 
> --
> 
> ice: E825: clear PHY_TSTAMP memory prior to soft reset
> 
> This commit replaces the E825 timestamp memory reset with a sweep. The old
> code wrote zeros to PHY_REG_TX_MEMORY_STATUS. The new code reads every
> PHY_TSTAMP index on the port, and the sweep only runs from within
> ice_ptp_phy_soft_reset_eth56g(), so the soft reset puts the internal counter
> and ready bitmap back in a consistent state. Read failures during the sweep
> are counted and reported with a warning but are not treated as fatal.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index 07b55fbb88dde..ef59b34ba52dc 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> +static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
>> +{
>> +	int err, failed = 0;
>> +	u8 idx;
>> +
>> +	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
>> +		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
>> +		if (err)
>> +			failed++;
>>  	}
>> +
>> +	if (failed)
>> +		dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
>> +			 failed, port);
>>  }
> 
> [ ... ]
> 
>> @@ -2311,6 +2320,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) {
> 
> [Severity: Low]
> What happens here if the sideband queue fails persistently instead of
> transiently?
> 

On a previous version I got asked to change to this method because of
possible transient failures. I think this is best since these reads and
side effects aren't entirely critical to functional device.

I would rather leave this as-is and not waffle back and forth about such
read/write failures that are entirely unexpected in practice.

> The loop in ice_ptp_clear_tx_memory_status_eth56g() runs for all 64
> INDEX_PER_PORT entries. It counts failures and never stops early. It also
> runs before the PHY_REG_GLOBAL read, and that read is the first access in
> ice_ptp_phy_soft_reset_eth56g() that would catch an unreachable PHY.
> 
> Each index takes this path:
> 
> ice_clear_ptp_tstamp_eth56g()
>   ice_read_ptp_tstamp_eth56g()
>     ice_read_port_mem_eth56g()
>       ice_read_phy_eth56g()
>         ice_sbq_rw_reg()
>           ice_sq_send_cmd()
>             ice_sq_done()
> 
> ice_sq_done() polls with ICE_CTL_Q_SQ_CMD_TIMEOUT, which is USEC_PER_SEC.
> 
> If the sideband queue head stops advancing, each low read waits the full
> 1 second and then returns an error. A failed low read skips the high read,
> so this costs one timeout per index. Fast -ENOSPC failures only start once
> the 64 entry ring (ICE_SBQ_LEN) fills up.
> 
> That could mean about a minute of stall in the sweep before the
> PHY_REG_GLOBAL read fails and the error is returned. Before this patch, the
> same condition was caught after a single timeout on PHY_REG_GLOBAL.
> 
> ice_ptp_init_phc_e825c() calls ice_ptp_phy_soft_reset_eth56g(). That makes
> this reachable from both ice_ptp_init_owner() during probe and
> ice_ptp_rebuild_owner() during reset rebuild.
> 
> After the later commit in this series, "ice: E825: perform a soft reset
> when starting the PHY timer", it is also reachable from
> ice_start_phy_timer_eth56g() via ice_ptp_port_phy_restart(), which holds
> ptp_port->ps_lock.
> 
> Because the sweep runs before PHY_REG_GLOBAL is checked, its read side
> effects can also land on a port whose soft reset then fails. The new kdoc
> does tell callers to treat a soft reset failure as a non-functional
> timestamp block.
> 
> Would it make sense to stop the sweep after consecutive failures, or on a
> timeout class error? Another option is to read PHY_REG_GLOBAL before
> starting the sweep.

I will change this if the maintainers demand, but otherwise I would much
rather see my actual issues fixed than continue to be stalled endlessly
by this AI looping me between different versions.

> 
>>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
> 
> [ ... ]
>