Re: [PATCH net 0/4] Intel Wired LAN Driver Updates 2026-04-20 (ice)

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

On 4/22/2026 2:23 AM, Simon Horman wrote:
> On Mon, Apr 20, 2026 at 05:51:24PM -0700, Jacob Keller wrote:
>> Since this is a set of related fixes for just the ice driver, Jake provides
>> the following description for the series:
> 
> Thanks for the excellent cover letter and patch descriptions.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> For completeness:
> 
> * I have looked over the AI generated review of patch 2/4 by Sashiko.
>   You may wish to too. But I do not believe that feedback warrants
>   holding up this series. Actually, I am skeptical those issues
>   should be addressed at all.
> 

Interestingly, the version posted to Intel Wired LAN did not receive any
feedback from Sashiko, and the code remains unchanged. As helpful as the
bots are, it is definitely frustrating that they remain somewhat
inconsistent.

To answer some of the questions, Sashiko says:

>> @@ -377,6 +377,31 @@ static void ice_ptp_cfg_sync_delay(const struct ice_hw *hw, u32 delay)
>>   * The following functions operate on devices with the ETH 56G PHY.
>>   */
>>  
>> +/**
>> + * ice_ptp_init_phc_e825c - Perform E825C specific PHC initialization
>> + * @hw: pointer to HW struct
>> + *
>> + * Perform E825C-specific PTP hardware clock initialization steps.
>> + *
>> + * Return: 0 on success, or a negative error value on failure.
>> + */
>> +static int ice_ptp_init_phc_e825c(struct ice_hw *hw)
>> +{
>> +	int err;
>> +
>> +	/* Soft reset all ports, to ensure everything is at a clean state */
>> +	for (int port = 0; port < hw->ptp.num_lports; port++) {
>> +		err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> Could this sequence disrupt active secondary PFs?
> This loop iterates over all ports and performs a soft reset without acquiring
> the global hardware semaphore.
> If the primary PF is rebound or undergoes initialization while
> secondary PFs are already bound and active, this lockless reset
> will wipe the timer configurations and outstanding Tx timestamps of those
> active secondary PFs.
> Does this need coordination with the software state of secondary PFs or use of
> ice_ptp_lock() before executing the reset?

This function resets the PTP timer, but it is called within either the
initialization or reset flows and only by the clock owner. After calling
the init function, the driver will reconfigure all ports, so I do not
think there is an issue.


It also says:

>> @@ -2179,6 +2204,69 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * ice_ptp_phy_soft_reset_eth56g - Perform a PHY soft reset on ETH56G
>> + * @hw: pointer to the HW structure
>> + * @port: PHY port number
>> + *
>> + * Trigger a soft reset of the ETH56G PHY by toggling the soft reset
>> + * bit in the PHY global register. The reset sequence consists of:
>> + *   1. Clearing the soft reset bit
>> + *   2. Asserting the soft reset bit
>> + *   3. Clearing the soft reset bit again
>> + *
>> + * Short delays are inserted between each step to allow the hardware
>> + * to settle. This provides a controlled way to reinitialize the PHY
>> + * without requiring a full device reset.
>> + *
>> + * Return: 0 on success, or a negative error code on failure when
>> + *         reading or writing the PHY register.
>> + */
>> +int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>> +{
>> +	u32 global_val;
>> +	int 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",
>> +			  port, err);
>> +		return err;
>> +	}
>> +
>> +	global_val &= ~PHY_REG_GLOBAL_SOFT_RESET_M;
>> +	ice_debug(hw, ICE_DBG_PTP, "Clearing soft reset bit for port %d, val: 0x%x\n",
>> +		  port, global_val);
>> +	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, global_val);
>> +	if (err) {
>> +		ice_debug(hw, ICE_DBG_PTP, "Failed to write PHY_REG_GLOBAL for port %d, err %d\n",
>> +			  port, err);
>> +		return err;
>> +	}
>> +
>> +	usleep_range(5000, 6000);
>> +
>> +	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
> Will this write clobber hardware state changes?
> The code reads the register into global_val once at the beginning, but modifies
> and writes it back three times, separated by sleep windows.
> If hardware or firmware modifies any other bits in this register during the
> sleep windows, the subsequent writes using the cached global_val will silently
> overwrite those updates.
> Should the register be re-read into global_val before each modification?

I do not think we need to re-read the value between these writes.
Neither the hardware or firmware update these values. If other software
is accessing and modifying the value for this register, we have a
different issue. Again, the only caller for the phy soft reset function
is the clock owning PF, so I don't see how another thread could cause an
issue here.

> * I have also looked over the AI generated review based on Chris Mason's
>   review prompts which is available at https://netdev-ai.bots.linux.dev
>   (if only it had a name!). It flags an potentially incorrect Fixes tag in
>   patch 4/4. However, the cover letter for the patch explains the
>   choice of Fixes tag, effectively rebutting the analysis generated by AI
>   (I guess it didn't take the commit message sufficiently into account.)
> 

Thanks for the review!