Re: [net-next,13/15] ice: add support for unmanaged DPLL on E830 NIC

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ice: add support for unmanaged DPLL on E830 NIC

Hardware variants of E830 may support an unmanaged DPLL where the
configuration is hardcoded within the hardware and firmware, meaning
users cannot modify settings. However, users are able to check the DPLL
lock status and obtain configuration information through the Linux DPLL
and devlink health subsystem.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 62f75701d652..2a9eb233dbf4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c

[ ... ]

> @@ -3148,12 +3176,15 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
>  	int ret;
>
>  	ret = ice_dpll_get_pins(pf, pins, start_idx, count, pf->dplls.clock_id);
> -	if (ret)
> +	if (!cgu || ret)
>  		return ret;
> -	if (cgu) {
> +
> +	if (first) {
>  		ret = ice_dpll_register_pins(first, pins, ops, count);
>  		if (ret)
>  			goto release_pins;
> +	}
> +	if (second) {
>  		ret = ice_dpll_register_pins(second, pins, ops, count);
>  		if (ret)
>  			goto unregister_first;
> @@ -3162,7 +3193,8 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
>  	return 0;
>
>  unregister_first:
> -	ice_dpll_unregister_pins(first, pins, ops, count);
> +	if (first)
> +		ice_dpll_unregister_pins(first, pins, ops, count);
>  release_pins:
>  	ice_dpll_release_pins(pins, count);
>  	return ret;

NULL checks were added to ice_dpll_init_direct_pins for handling unmanaged
DPLL initialization where only one DPLL device is active. However, is the
corresponding cleanup function ice_dpll_deinit_direct_pins missing the same
NULL checks?

In unmanaged mode, only pf->dplls.pps.dpll is initialized while
pf->dplls.eec.dpll remains NULL. When ice_dpll_init_pins calls
ice_dpll_init_direct_pins(pf, true, outputs, count, num_outputs,
output_ops, eec.dpll, pps.dpll), if this second call (for output pins)
fails, the error path calls ice_dpll_deinit_direct_pins(pf, true, ...,
pps.dpll, eec.dpll).

Looking at ice_dpll_deinit_direct_pins:

static void ice_dpll_deinit_direct_pins(struct ice_pf *pf, bool cgu,
					struct ice_dpll_pin *pins, int count,
					const struct dpll_pin_ops *ops,
					struct dpll_device *first,
					struct dpll_device *second)
{
	if (cgu) {
		ice_dpll_unregister_pins(first, pins, ops, count);
		ice_dpll_unregister_pins(second, pins, ops, count);
	}
	ice_dpll_release_pins(pins, count);
}

When cgu=true and second=NULL, ice_dpll_unregister_pins is called with
dpll=NULL. Then dpll_pin_unregister(dpll, pin) dereferences dpll with
xa_empty(&dpll->pin_refs), causing a NULL pointer dereference.

Shouldn't ice_dpll_deinit_direct_pins add NULL checks for first and second,
matching the guards you added in ice_dpll_init_direct_pins?

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
> index ae42cdea0ee1..2c98b6c6deb0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.h
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h

[ ... ]

> @@ -173,3 +181,5 @@ static inline void ice_dpll_deinit(struct ice_pf *pf) { }
>  #define ICE_CGU_R11_SYNCE_S_BYP_CLK		GENMASK(6, 1)
>
>  #define ICE_CGU_BYPASS_MUX_OFFSET_E825C		3
> +#define ICE_DPLL_UNMANAGED_PIN_NUM		4
> +#define ICE_DPLL_IN_ESYNC_ENABLED	ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN

This isn't a bug, but ICE_DPLL_UNMANAGED_PIN_NUM is defined but never
used. Looking at the unmanaged pin descriptors, ice_e830_unmanaged_inputs
has 1 entry and ice_e830_unmanaged_outputs has 2 entries (totaling 3 pins,
not 4).