Re: [Intel-wired-lan] [PATCH RFC net-next v2 12/12] ice: dpll: Support E825-C SyncE and dynamic pin discovery

[Ivan Vecera <ivecera@redhat.com>]

On 12/16/25 2:28 PM, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> ...
>> +		/* Register rclk pin */
>> +		pin = &pf->dplls.rclk;
>> +		dpll_pin_on_pin_unregister(parent->pin, pin->pin,
>> +					   &ice_dpll_rclk_ops, pin);
>> +
>> +		/* Drop fwnode pin reference */
>> +		dpll_pin_put(parent->pin, &parent->tracker);
>> +		parent->pin = NULL;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +out:
>> +	kfree(work);
> It looks like you free the embedded work_struct pointer instead of the allocated ice_dpll_pin_work container @ice_dpll_pin_notify().
> Isn't it?

You are right, will fix this in non-RFC submission.

>> +}
>> +
>> ...
>> +static int ice_dpll_init_info_e825c(struct ice_pf *pf)
>> +{
>> +	struct ice_dplls *d = &pf->dplls;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	d->clock_id = ice_generate_clock_id(pf);
>> +	d->num_inputs = ICE_SYNCE_CLK_NUM;
>> +
>> +	d->inputs = kcalloc(d->num_inputs, sizeof(*d->inputs),
>> GFP_KERNEL);
> It looks like for E825-C the allocated pin info (d->inputs and related fields) is never freed:
> error paths in ice_dpll_init_info_e825c() return after kcalloc() without cleanup, and ice_dpll_deinit() explicitly skips ice_dpll_deinit_info() for this MAC.

You are right, this is Arek's code part. I don't see any problem to call
ice_dpll_deinit_info() also for this MAC (.outputs, .pps.input_prio and
.eec.input_prio should be NULL for e825c so it is safe to kfree them).

Will add correct cleanup into ice_dpll_init_info_e825c() and call
ice_dpll_deinit_info() also for this MAC.

> With the best regards
> Alex
> 
>> +	if (!d->inputs)
>> +		return -ENOMEM;
>> +
>> +	ret = ice_get_cgu_rclk_pin_info(&pf->hw, &d->base_rclk_idx,
>> +					&pf->dplls.rclk.num_parents);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < pf->dplls.rclk.num_parents; i++)
>> +		pf->dplls.rclk.parent_idx[i] = d->base_rclk_idx + i;
>> +
>> +	if (ice_pf_src_tmr_owned(pf)) {
>> +		d->base_1588_idx = ICE_E825_1588_BASE_IDX;
>> +		pf->dplls.pin_1588.num_parents = pf-
>>> dplls.rclk.num_parents;
>> +		for (i = 0; i < pf->dplls.pin_1588.num_parents; i++)
>> +			pf->dplls.pin_1588.parent_idx[i] = d-
>>> base_1588_idx + i;
>> +	}
>> +	ret = ice_dpll_init_pins_info(pf,
>> ICE_DPLL_PIN_TYPE_RCLK_INPUT);
>> +	if (ret)
>> +		return ret;
>> +	dev_dbg(ice_pf_to_dev(pf),
>> +		"%s - success, inputs: %u, outputs: %u, rclk-parents:
>> %u, pin_1588-parents: %u\n",
>> +		 __func__, d->num_inputs, d->num_outputs, d-
>>> rclk.num_parents,
>> +		 d->pin_1588.num_parents);
>> +	return 0;
>> +}
>> +
>> ...
>> +int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port,
>> bool *active,
>> +				      enum ice_synce_clk output)
>> +{
>> +	u8 active_clk;
>> +	u32 val;
>> +
>> +	switch (output) {
>> +	case ICE_SYNCE_CLK0:
>> +		ice_read_cgu_reg(hw, ICE_CGU_R10, &val);
>> +		active_clk = FIELD_GET(ICE_CGU_R10_SYNCE_S_REF_CLK,
>> val);
>> +		break;
>> +	case ICE_SYNCE_CLK1:
>> +		ice_read_cgu_reg(hw, ICE_CGU_R11, &val);
>> +		active_clk = FIELD_GET(ICE_CGU_R11_SYNCE_S_BYP_CLK,
>> val);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> ice_read_cgu_reg() return codes are ignored, can you explain why?

Arek's code... will fix in the non-RFC submission.

Thanks a lot Alex for your sharp eyes. ;-)

Ivan