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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In ice_ptp_phy_soft_reset_eth56g()
  (drivers/net/ethernet/intel/ice/ice_ptp_hw.c), the patch adds an…
- [Low] The new kdoc for ice_ptp_clear_tx_memory_status_eth56g() in
  drivers/net/ethernet/intel/ice/ice_ptp_hw.c has a typo in the sentence…

--

ice: E825: clear PHY_TSTAMP memory prior to soft reset

This patch drops the E825 path that cleared Tx timestamp memory by writing
PHY_REG_TX_MEMORY_STATUS. Instead, ice_ptp_phy_soft_reset_eth56g() now
first reads every PHY_TSTAMP index for the port, then runs the soft reset,
so stale valid bits are cleared and the internal counter is reset. The
kdoc for ice_ptp_reset_ts_memory() and ice_ptp_phy_soft_reset_eth56g() is
updated to match.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dde..5c63f88bbf48a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> @@ -1177,19 +1147,43 @@ 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

[ ... ]

> + * Due to interactions with an internal HW counter for the number of
> + * outstanding Tx timestamps, this *must* not be called outside if the
> + * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
> + * may become out of sync and prevent new timestamp interrupts.

[Severity: Low]
This isn't a bug, but there's a typo in this kdoc: "outside if the"
should be "outside of the".

[ ... ]

> @@ -2303,6 +2298,20 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)

[ ... ]

> + * In the event that the driver fails to access the PHY during the completion
> + * of the soft reset, (e.g. due to sideband queue message failures), the
> + * device may be left in an inconsistent state. Such failures are expected to
> + * be rare. Callers are responsible for checking the return of this function
> + * and treating errors as indicating that the timestamping block of the PHY is
> + * non-functional. Recovery may be possible by issuing another soft reset
> + * after any transient access errors have abated.
> + *
>   * Return: 0 on success, or a negative error code on failure when
>   *         reading or writing the PHY register.
>   */
> @@ -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) {
>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",

[Severity: Medium]
Suppose the sweep finishes, and then the ice_read_ptp_reg_eth56g() of
PHY_REG_GLOBAL fails, or one of the three ice_write_ptp_reg_eth56g() calls
to PHY_REG_GLOBAL fails. The function returns early and the soft reset bit
is never toggled.

The commit message says each read of a valid entry, including cold boot
garbage, decrements the internal counter. Only the soft reset puts the
counter back to zero.

Can this leave the counter underflowed, with no Tx timestamp interrupts?
That is the condition this patch tries to prevent. Before this patch, a
failed soft reset did no PHY_TSTAMP reads.

The commit message says recovery happens on a link reset:

  "Upon a link reset, the driver will re-sweep the timestamp indexes and
  perform a fresh soft reset, clearing any inconsistency from the left
  over failed state."

The new kdoc also says another soft reset may recover.

Is there a code path that does this? The only caller of
ice_ptp_phy_soft_reset_eth56g() is ice_ptp_init_phc_e825c(). That is
reached only from ice_ptp_init_owner() and ice_ptp_rebuild_owner(), through
ice_ptp_init_phc().

On a link change, ice_ptp_link_change()->ice_ptp_port_phy_restart() does
this for E825:

	case ICE_MAC_GENERIC_3K_E825:
		...
		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

		err = ice_start_phy_timer_eth56g(hw, port);

This does not sweep and does not soft reset. Also, ice_ptp_link_change()
returns early when pf->ptp.state != ICE_PTP_READY. That is the case after
a failed init or rebuild.

After a failed rebuild the state is ICE_PTP_ERROR, and later calls to
ice_ptp_rebuild() exit here:

	} else if (ptp->state != ICE_PTP_RESETTING) {
		err = -EINVAL;
		dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
		goto err;
	}

So ice_ptp_rebuild_owner() is never retried. It looks like only a driver
reload would run the sweep and soft reset again.

Separately, ice_ptp_init_phc_e825c() returns on 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;
		}
	}

The ports after it are neither swept nor reset in that pass.

Should the commit message and kdoc say a driver reload is needed? Or
should a real retry be added, for example by checking PHY_REG_GLOBAL access
before the sweep, or by retrying the reset when a failure happens after the
sweep?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-reduction-v1-0-81237c67a361%40intel.com