Re: [PATCH iwl-net 3/4] ice: fix ready bitmap check for non-E822 devices

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

On 4/8/2026 11:46 AM, Jacob Keller wrote:
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index ada42bcc4d0b..34906f972d17 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2718,7 +2718,7 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf)
>  bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
>  {
>  	struct ice_hw *hw = &pf->hw;
> -	unsigned int i;
> +	int ret;
>  
>  	/* Check software indicator */
>  	switch (pf->ptp.tx_interrupt_mode) {
> @@ -2739,16 +2739,15 @@ bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
>  	}
>  
>  	/* Check hardware indicator */
> -	for (i = 0; i < ICE_GET_QUAD_NUM(hw->ptp.num_lports); i++) {
> -		u64 tstamp_ready = 0;
> -		int err;
> -
> -		err = ice_get_phy_tx_tstamp_ready(&pf->hw, i, &tstamp_ready);
> -		if (err || tstamp_ready)
> -			return true;
> +	ret = ice_check_phy_tx_tstamp_ready(hw);
> +	if (ret < 0) {
> +		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
> +			ret);
> +		/* Stop triggering IRQs if we're unable to read PHY */
> +		return false;
>  	}
>  
> -	return false;
> +	return ret;

I got some comments off-list from Aleks about this "conversion" from
integer to boolean here. The return from ice_check_phy_tx_tstamp_ready()
is 0 when there are no timsetamps and 1 when there is at least one
timestamp available. The negative values are excluded in the check just
above this.

I guess I can re-write this to be an if/elif/else chain instead, but I
wonder what others on the list think here.

I understand it is slightly confusing to have a return which is 3-way
(error, 0 or 1), but other solutions seemed inellegant. Using a bool for
the check means we lose the nuance of the error code (and the distinct
dev_dbg message). It also doesn't let the caller decide whether its more
imporant to continue on error or stop on error (which might be dependant
on each call flow).

Just doing "ret ? true : false" seems stupid since that is literally
what this will do given that we already excluded negative values in the
check above before returning.

> +/**
> + * ice_check_phy_tx_tstamp_ready_e810 - Check Tx memory status register
> + * @hw: pointer to the HW struct
> + *
> + * The E810 devices do not have a Tx memory status register. Note this is
> + * intentionally different behavior from ice_get_phy_tx_tstamp_ready_e810
> + * which always says that all bits are ready. This function is called in cases
> + * where code will trigger interrupts if timestamps are waiting, and should
> + * not be called for E810 hardware.
> + *
> + * Return: 0.
> + */
> +static int ice_check_phy_tx_tstamp_ready_e810(struct ice_hw *hw)
> +{
> +	return 0;
> +}
> +

I got comments off list about this "unused parameter" and empty
function. The E810 hardware doesn't have a ready bitmap in its
registers. Thus, this function doesn't really do anything. I'm
considering if it makes more sense to move this comment into the
hardware wrapper layer and drop this function (and thus its unused
parameter).
> +/**
> + * ice_check_phy_tx_tstamp_ready - Check PHY Tx timestamp memory status
> + * @hw: pointer to the HW struct
> + *
> + * Check the PHY for Tx timestamp memory status on all ports. If you need to
> + * see individual timestamp status for each index, use
> + * ice_get_phy_tx_tstamp_ready() instead.
> + *
> + * Return: 1 if any port has timestamps available, 0 if there are no timestamps
> + * available, and a negative error code on failure.
> + */

This type of return is somewhat confusing and rare. Typically functions
returning int either return 0 for success or a non-zero error code. In
this case there are two "success" states. C doesn't have anything like
the Error types from newer languages.

The closest example I have is functions that read from a socket or
buffer or file, which return negative error codes or the number of bytes
read/written.

I could change this API to return the total number of timestamps
available.. but every caller just cares about whether there is at least
one timestamp, so I opted to shrink it to 0/1 in order to allow eliding
unnecessary checks for devices with multiple ports.

Thoughts?

Thanks,
Jak