Re: [net-next,v2,12/12] ice: dpll: Support E825-C SyncE and dynamic pin discovery

[Jakub Kicinski]

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: dpll: Support E825-C SyncE and dynamic pin discovery

This patch adds DPLL support for the Intel E825-C Ethernet controller.
It implements dynamic pin discovery via firmware nodes, asynchronous
registration using a DPLL notifier chain, and hardware configuration
for CGU registers.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 4eca62688d83..9d82748810a3 100644

[ ... ]

> +static void ice_dpll_pin_notify_work(struct work_struct *work)
> +{
> +	struct ice_dpll_pin_work *w = container_of(work,
> +						   struct ice_dpll_pin_work,
> +						   work);
> +	struct ice_dpll_pin *pin, *parent = w->pin;
> +	struct ice_pf *pf = parent->pf;
> +	int ret;
> +
> +	switch (w->action) {
> +	case DPLL_PIN_CREATED:
> +		if (!IS_ERR_OR_NULL(parent->pin)) {
> +			/* We have already our pin registered */
> +			goto out;
> +		}
> +
> +		/* Grab reference on fwnode pin */
> +		parent->pin = fwnode_dpll_pin_find(parent->fwnode,
> +						   &parent->tracker);
> +		if (IS_ERR_OR_NULL(parent->pin)) {
> +			dev_err(ice_pf_to_dev(pf),
> +				"Cannot get fwnode pin reference\n");
> +			goto out;
> +		}
> +
> +		/* Register rclk pin */
> +		pin = &pf->dplls.rclk;
> +		ret = dpll_pin_on_pin_register(parent->pin, pin->pin,
> +					       &ice_dpll_rclk_ops, pin);

Can pin->pin (pf->dplls.rclk.pin) be NULL here? Looking at the init
order in ice_dpll_init_pins_e825():

    ret = ice_dpll_init_fwnode_pins(pf, pf->dplls.inputs, 0);
    ...
    ret = ice_dpll_init_rclk_pin(pf, DPLL_PIN_IDX_UNSPEC, ...);

The fwnode pins are initialized first, which registers notifiers. If a
DPLL_PIN_CREATED notification fires before ice_dpll_init_rclk_pin()
completes, this work handler will run while pf->dplls.rclk.pin is still
NULL (the structure is zeroed by kcalloc). Passing NULL to
dpll_pin_on_pin_register() causes a NULL dereference in
dpll_xa_ref_pin_add() when it accesses pin->parent_refs.

> +		if (ret) {
> +			dev_err(ice_pf_to_dev(pf),
> +				"Failed to register pin: %pe\n", ERR_PTR(ret));
> +			dpll_pin_put(parent->pin, &parent->tracker);
> +			parent->pin = NULL;
> +			goto out;
> +		}
> +		break;

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 4c8d20f2d2c0..0eb8a9e29a52 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1341,6 +1341,35 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  	if (pf->hw.reset_ongoing)
>  		return;
>
> +	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825) {
> +		int pin, err;
> +
> +		mutex_lock(&pf->dplls.lock);

Should this check ICE_FLAG_DPLL before using pf->dplls.lock? Looking at
ice_dpll_init_e825(), if initialization fails the mutex is destroyed:

    err = ice_dpll_init_info_e825c(pf);
    if (err)
        goto err_exit;
    ...
    err_exit:
        mutex_destroy(&d->lock);

But ICE_FLAG_DPLL is only set on success. If DPLL init fails and a link
change event occurs, this code will use a destroyed mutex.

> +		for (pin = 0; pin < ICE_SYNCE_CLK_NUM; pin++) {
> +			enum ice_synce_clk clk_pin;
> +			bool active;
> +			u8 port_num;
> +
> +			port_num = ptp_port->port_num;
> +			clk_pin = (enum ice_synce_clk)pin;
> +			err = ice_tspll_bypass_mux_active_e825c(hw,
> +								port_num,
> +								&active,
> +								clk_pin);
> +			if (WARN_ON_ONCE(err)) {
> +				mutex_unlock(&pf->dplls.lock);
> +				return;
> +			}
> +
> +			err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
> +			if (active && WARN_ON_ONCE(err)) {
> +				mutex_unlock(&pf->dplls.lock);
> +				return;
> +			}
> +		}
> +		mutex_unlock(&pf->dplls.lock);
> +	}