Re: [PATCH net 08/15] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

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

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset
> 
> This replaces the register-write based ice_ptp_reset_ts_memory_eth56g()
> with a read of every Tx timestamp memory entry for a port, and moves that
> clearing into ice_ptp_phy_soft_reset_eth56g() so it always happens right
> before the PHY soft reset. ice_ptp_reset_ts_memory() now does nothing for
> E825.
> 
> Two questions below.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index d48eb3c618231..a3a9f7ce04d2b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> @@ -1139,8 +1121,8 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
>>   * internal PHYs of the 56G devices.
>>   *
>>   * To directly clear the contents of the timestamp block entirely, discarding
>> - * all timestamp data at once, software should instead use
>> - * ice_ptp_reset_ts_memory_quad_eth56g().
>> + * all timestamp data at once, software should instead perform a PHY soft
>> + * reset via ice_ptp_phy_soft_reset_eth56g().
>>   *
>>   * This function should only be called on an idx whose bit is set according to
>>   * ice_get_phy_tx_tstamp_ready().
> 
> [Severity: Low]
> 
> This isn't a bug, but should the rest of the kernel-doc for
> ice_clear_ptp_tstamp_eth56g() be refreshed too?
> 
> The summary above still says "Read and then forcibly clear the timestamp
> index", and the return list still says "%other - failed to write to PHY",
> but the ice_write_port_mem_eth56g() call is removed below, so the body only
> performs two PHY reads and no write can fail.
> 
> The retained line
> 
>   * This function should only be called on an idx whose bit is set according to
>   * ice_get_phy_tx_tstamp_ready().
> 
> is also the contract repeated by the exported wrapper ice_clear_phy_tstamp(),
> yet the new ice_ptp_clear_tx_memory_status_eth56g() calls this helper for
> every index regardless of the ready bitmap. Since reading an index whose
> valid bit is set is exactly what triggers the internal counter decrement
> described in the commit message, is the precondition still meant to apply,
> or should the doc say the unconditional sweep is allowed only when paired
> with the soft reset?
> 

Yes, I'll update this.

>> @@ -1152,24 +1134,16 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
>>  static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
>>  {
>>  	u64 unused_tstamp;
>> -	u16 lo_addr;
>>  	int err;
>>  
>> -	/* Read the timestamp register to ensure the timestamp status bit is
>> -	 * cleared.
>> +	/* Per the PHY spec, reading the timestamp memory location is what
>> +	 * clears the entry's valid bit and its corresponding (read-only)
>> +	 * ts_memory_status bit.
>>  	 */
>>  	err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
>>  	if (err) {
>>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n",
>>  			  port, idx, err);
>> -	}
>> -
>> -	lo_addr = (u16)PHY_TSTAMP_L(idx);
>> -
>> -	err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
>> -	if (err) {
>> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n",
>> -			  port, idx, err);
>>  		return err;
>>  	}
>>  
> 
> [ ... ]
> 
>> @@ -1177,19 +1151,36 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
>>  }
>>  
>>  /**
>> - * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the port block
>> + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory
>>   * @hw: pointer to the HW struct
>> + * @port: port number to clear
>> + *
>> + * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the
>> + * only way to clear a timestamp valid bit (and its read-only ts_memory_status
>> + * bit) is to read the timestamp memory location, so read every entry for the
>> + * port (two 32-bit reads each). This discards all timestamp data on the port,
>> + * so it must only be used for a full reset; callers that must preserve
>> + * in-flight timestamps clear individual indices via ice_clear_phy_tstamp().
>> + *
>> + * 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.
>> + *
>> + * Return: 0 on success, negative error code on failure to read the PHY.
>>   */
>> -static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
>> +static int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
>>  {
>> -	unsigned int port;
>> +	int err = 0;
>> +	u8 idx;
>>  
>> -	for (port = 0; port < hw->ptp.num_lports; port++) {
>> -		ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L,
>> -					 0);
>> -		ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U,
>> -					 0);
>> +	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
>> +		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
>> +		if (err)
>> +			return err;
>>  	}
>> +
>> +	return 0;
>>  }
> 
> [Severity: Medium]
> 
> Can this fail-fast return leave the internal timestamp counter underflowed
> for this port?
> 
> The reads are the side-effecting operation: each read of an entry whose
> valid bit is set clears that bit and decrements the PHY's outstanding
> timestamp counter. If ice_clear_ptp_tstamp_eth56g() fails at, say, idx 30,
> the decrements for idx 0..29 have already been applied, and this function
> returns before the soft reset that is the only thing which re-zeroes the
> counter.
> 
> The read path goes through the sideband queue:
> 
>   ice_clear_ptp_tstamp_eth56g()
>     ice_read_ptp_tstamp_eth56g()
>       ice_read_port_mem_eth56g() -> ice_read_phy_eth56g() -> ice_sbq_rw_reg()
> 
> so a transient sideband failure is enough to hit this.
> 
> Would it be better to log the failing index but continue the sweep, and
> still drive the soft reset before propagating the error?
> 

Yea, I think that makes sense.

>> @@ -2306,6 +2304,13 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>>  	u32 global_val;
>>  	int err;
>>  
>> +	err = ice_ptp_clear_tx_memory_status_eth56g(hw, port);
>> +	if (err) {
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to clear PHY_REG_TX_MEMORY_STATUS for port %d, err %d\n",
>> +			  port, err);
>> +		return err;
>> +	}
>> +
>>  	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: Medium]
> 
> Same concern at this early return, and at the two error returns that follow
> it. Once ice_ptp_clear_tx_memory_status_eth56g() has consumed entries, a
> failed ice_read_ptp_reg_eth56g(PHY_REG_GLOBAL) or a failed first
> PHY_REG_GLOBAL write also returns without ever asserting
> PHY_REG_GLOBAL_SOFT_RESET_M, so the clear happens without its paired reset.
> 
> Isn't that the exact condition the new kernel-doc warns about?
> 
>   * 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.
> 
> The error also propagates out of the per-port loop in
> ice_ptp_init_phc_e825c():
> 
>   for (int port = 0; port < hw->ptp.num_lports; port++) {
>           err = ice_ptp_phy_soft_reset_eth56g(hw, port);
>           if (err) {
>                   ...
>                   return err;
>           }
>   }
> 
> so the remaining ports get neither the clear nor the reset. A later
> successful soft reset would repair the state, but should the reset still be
> driven for the port that already had its entries read?
> 
> This was checked at the end of the series and the fail-fast return and the
> early return here are both still present, so no later patch in the series
> changes this.

When paired with a soft reset, readings only purpose is to ensure that
we have a cleared memory bank after the reset. If the rest of the
software is behaving correctly it shouldn't strictly matter if we left a
state bit or not, because we shouldn't be reading it. The logic is
supposed to ensure we do not read the register until we are certain its
already been overwritten by hardware. These writes are intended to
ensure that we've cleaned the state properly in the event that we
somehow have another bug that triggers such a read. I'll try to mention
this in the commit message and update this to log a message but not
produce an error. It should be non-fatal.

Thanks,
Jake