Re: [PATCH iwl-next v1 3/3] ice: enable timesync operation on 2xNAC E825 devices

[Simon Horman <horms@kernel.org>]

On Fri, Feb 21, 2025 at 01:31:23PM +0100, Grzegorz Nitka wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> According to the E825C specification, SBQ address for ports on a single
> complex is device 2 for PHY 0 and device 13 for PHY1.
> For accessing ports on a dual complex E825C (so called 2xNAC mode),
> the driver should use destination device 2 (referred as phy_0) for
> the current complex PHY and device 13 (referred as phy_0_peer) for
> peer complex PHY.
> 
> Differentiate SBQ destination device by checking if current PF port
> number is on the same PHY as target port number.
> 
> Adjust 'ice_get_lane_number' function to provide unique port number for
> ports from PHY1 in 'dual' mode config (by adding fixed offset for PHY1
> ports). Cache this value in ice_hw struct.
> 
> Introduce ice_get_primary_hw wrapper to get access to timesync register
> not available from second NAC.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice.h        | 60 ++++++++++++++++++++-
>  drivers/net/ethernet/intel/ice/ice_common.c |  6 ++-
>  drivers/net/ethernet/intel/ice/ice_ptp.c    | 49 ++++++++++++-----
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 39 +++++++++++---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  5 --
>  drivers/net/ethernet/intel/ice/ice_type.h   |  1 +
>  6 files changed, 134 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 53b990e2e850..d80ab48402f1 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -193,8 +193,6 @@
>  
>  #define ice_pf_to_dev(pf) (&((pf)->pdev->dev))
>  
> -#define ice_pf_src_tmr_owned(pf) ((pf)->hw.func_caps.ts_func_info.src_tmr_owned)
> -
>  enum ice_feature {
>  	ICE_F_DSCP,
>  	ICE_F_PHY_RCLK,
> @@ -1049,4 +1047,62 @@ static inline void ice_clear_rdma_cap(struct ice_pf *pf)
>  }
>  
>  extern const struct xdp_metadata_ops ice_xdp_md_ops;
> +
> +/**
> + * ice_is_dual - Check if given config is multi-NAC
> + * @hw: pointer to HW structure
> + *
> + * Return: true if the device is running in mutli-NAC (Network
> + * Acceleration Complex) configuration variant, false otherwise
> + * (always false for non-E825 devices).
> + */
> +static inline bool ice_is_dual(struct ice_hw *hw)
> +{
> +	return hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
> +	       (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
> +}

Thanks for the complete Kernel doc, and not adding unnecessary () around
the value returned. Overall these helpers seem nice and clean to me.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index ed7ef8608cbb..4ff4c99d0872 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1135,6 +1135,8 @@ int ice_init_hw(struct ice_hw *hw)
>  		}
>  	}
>  
> +	hw->lane_num = ice_get_phy_lane_number(hw);
> +

Unfortunately there seems to be a bit of a problem here:

The type of hw->lane number is u8.
But ice_get_phy_lane_number returns an int,
which may be a negative error value...

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c

...

> @@ -3177,17 +3203,16 @@ void ice_ptp_init(struct ice_pf *pf)
>  {
>  	struct ice_ptp *ptp = &pf->ptp;
>  	struct ice_hw *hw = &pf->hw;
> -	int lane_num, err;
> +	int err;
>  
>  	ptp->state = ICE_PTP_INITIALIZING;
>  
> -	lane_num = ice_get_phy_lane_number(hw);
> -	if (lane_num < 0) {
> -		err = lane_num;
> +	if (hw->lane_num < 0) {

... which is checked here.

But because hw->lane_num is unsigned, this condition is always false.

FWIIW, I think it is nice that that hw->lane is a u8.
But error handling seems broken here.

> +		err = hw->lane_num;
>  		goto err_exit;
>  	}
> +	ptp->port.port_num = hw->lane_num;
>  
> -	ptp->port.port_num = (u8)lane_num;
>  	ice_ptp_init_hw(hw);
>  
>  	ice_ptp_init_tx_interrupt_mode(pf);

...