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

[Jakub Kicinski <kuba@kernel.org>]

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?

> @@ -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?

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