Re: [Intel-wired-lan] [PATCH net-next 2/3] ice: fix undefined references from DPLL code when !CONFIG_PTP_1588_CLOCK

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

On 9/20/2023 11:07 AM, Alexander Lobakin wrote:
> DPLL code in ice unconditionally calls several PTP functions which are
> only built when CONFIG_PTP_1588_CLOCK is set. This throws a good bunch
> of link errors:
> 
> ERROR: modpost: "ice_cgu_get_pin_name"
> [drivers/net/ethernet/intel/ice/ice.ko] undefined!
> ERROR: modpost: "ice_get_cgu_state"
> [drivers/net/ethernet/intel/ice/ice.ko] undefined!
> OR: modpost: "ice_is_cgu_present"
> [drivers/net/ethernet/intel/ice/ice.ko] undefined!
> ERROR: modpost: "ice_get_cgu_rclk_pin_info"
> [drivers/net/ethernet/intel/ice/ice.ko] undefined!
> ERROR: modpost: "ice_cgu_get_pin_type"
> [drivers/net/ethernet/intel/ice/ice.ko] undefined!
> ERROR: modpost: "ice_cgu_get_pin_freq_supp"
> [drivers/net/ethernet/intel/ice/ice.ko] undefined!
> 
> ice_dpll_{,de}init() can be only called at runtime when the
> corresponding feature flags are set, which is not the case when PTP
> support is not compiled. However, the linker has no clue about this.
> Compile DPLL code only when CONFIG_PTP_1588_CLOCK is enabled and guard
> the mentioned init/deinit function calls, so that ice_dpll.o is only
> referred when it gets compiled>
> Note that ideally ice_is_feature_supported() needs to check for
> compile-time flags first to be able to handle this without any
> additional call guards, and we may want to do that in the future.
> 
> Fixes: d7999f5ea64b ("ice: implement dpll interface to control cgu")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309191214.TaYEct4H-lkp@intel.com
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/Makefile   | 5 ++---
>  drivers/net/ethernet/intel/ice/ice_main.c | 8 +++++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> index 00806ddf5bf0..16f96d5210d8 100644
> --- a/drivers/net/ethernet/intel/ice/Makefile
> +++ b/drivers/net/ethernet/intel/ice/Makefile
> @@ -34,8 +34,7 @@ ice-y := ice_main.o	\
>  	 ice_lag.o	\
>  	 ice_ethtool.o  \
>  	 ice_repr.o	\
> -	 ice_tc_lib.o	\
> -	 ice_dpll.o
> +	 ice_tc_lib.o
>  ice-$(CONFIG_PCI_IOV) +=	\
>  	ice_sriov.o		\
>  	ice_virtchnl.o		\
> @@ -44,7 +43,7 @@ ice-$(CONFIG_PCI_IOV) +=	\
>  	ice_vf_mbx.o		\
>  	ice_vf_vsi_vlan_ops.o	\
>  	ice_vf_lib.o
> -ice-$(CONFIG_PTP_1588_CLOCK) += ice_ptp.o ice_ptp_hw.o
> +ice-$(CONFIG_PTP_1588_CLOCK) += ice_dpll.o ice_ptp.o ice_ptp_hw.o
>  ice-$(CONFIG_DCB) += ice_dcb.o ice_dcb_nl.o ice_dcb_lib.o
>  ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>  ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index e22f41fea8db..9b48918dcdb7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4665,8 +4665,9 @@ static void ice_init_features(struct ice_pf *pf)
>  	if (ice_is_feature_supported(pf, ICE_F_GNSS))
>  		ice_gnss_init(pf);
>  
> -	if (ice_is_feature_supported(pf, ICE_F_CGU) ||
> -	    ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
> +	if (IS_ENABLED(CONFIG_PTP_1588_CLOCK) &&
> +	    (ice_is_feature_supported(pf, ICE_F_CGU) ||
> +	     ice_is_feature_supported(pf, ICE_F_PHY_RCLK)))
>  		ice_dpll_init(pf);

We can stub ice_dpll_init and dpll_deinit in the ice_dpll.h header so
that we don't have to check IS_ENABLED here.

I posted that fix at
https://lore.kernel.org/intel-wired-lan/20230919233435.518620-1-jacob.e.keller@intel.com/
though I guess I sent it to intel-wired-lan first instead of sending
directly to netdev.

I would prefer the resolutions in my series, as I think they're
ultimately cleaner.

Of course: priority is getting this fixed so I won't begrudge pulling
this now.

Thanks,
Jake

>  
>  	/* Note: Flow director init failure is non-fatal to load */
> @@ -4695,7 +4696,8 @@ static void ice_deinit_features(struct ice_pf *pf)
>  		ice_gnss_exit(pf);
>  	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
>  		ice_ptp_release(pf);
> -	if (test_bit(ICE_FLAG_DPLL, pf->flags))
> +	if (IS_ENABLED(CONFIG_PTP_1588_CLOCK) &&
> +	    test_bit(ICE_FLAG_DPLL, pf->flags))
>  		ice_dpll_deinit(pf);
>  }
>  
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan