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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 4/2/2026 3:26 AM, Paolo Abeni wrote:
> 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).

Hi Paolo,

We're working on changes to resolve these issues.

Thanks,
Tony