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

[netdev-bot+sashiko@kernel.org]

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?

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.

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-jk-e825c-critical-timestamping-fixes-v1-0-72128d1dd928%40intel.com