Re: [PATCH RFC net-next v3 1/2] netdevsim: implement DPLL for subsystem selftests

[Simon Horman <horms@kernel.org>]

On Fri, Nov 17, 2023 at 08:05:04PM +0100, Michal Michalik wrote:
> DPLL subsystem integration tests require a module which mimics the
> behavior of real driver which supports DPLL hardware. To fully test the
> subsystem the netdevsim is amended with DPLL implementation.
> 
> Signed-off-by: Michal Michalik <michal.michalik@intel.com>

Hi Michal,

Nice to see tests being added for DPLL.
some minor feedback from my side.

> diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c

...

> +static void nsim_fill_pin_pd(struct nsim_pin_priv_data *pd, u64 frequency, u32 prio,
> +			     enum dpll_pin_direction direction)

nit: Please consider limiting Networking code to 80 columns wide.

...

> +int nsim_dpll_init_owner(struct nsim_dpll *dpll, int devid,
> +			 unsigned int ports_count)
> +{
> +	u64 clock_id;
> +	int err;
> +
> +	get_random_bytes(&clock_id, sizeof(clock_id));
> +
> +	/* Create EEC DPLL */
> +	dpll->dpll_e = dpll_device_get(clock_id, EEC_DPLL_DEV, THIS_MODULE);
> +	if (IS_ERR(dpll->dpll_e))
> +		goto dpll_e;

Branching to dpll_e will cause the function to return err,
but err is uninitialised here.

As there is nothing to unwind here I would lean towards simply
returning a negative error value directly here. But if not,
I'd suggest setting err to a negative error value inside the
if condition.

Flagged by clang-16 W=1 build, and Smatch.

> +
> +	dpll->dpll_e_pd.temperature = EEC_DPLL_TEMPERATURE;
> +	dpll->dpll_e_pd.mode = DPLL_MODE_AUTOMATIC;
> +	dpll->dpll_e_pd.clock_id = clock_id;
> +	dpll->dpll_e_pd.status = DPLL_LOCK_STATUS_UNLOCKED;
> +
> +	err = dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &nsim_dds_ops,
> +				   &dpll->dpll_e_pd);
> +	if (err)
> +		goto e_reg;
> +
> +	/* Create PPS DPLL */
> +	dpll->dpll_p = dpll_device_get(clock_id, PPS_DPLL_DEV, THIS_MODULE);
> +	if (IS_ERR(dpll->dpll_p))
> +		goto dpll_p;
> +
> +	dpll->dpll_p_pd.temperature = PPS_DPLL_TEMPERATURE;
> +	dpll->dpll_p_pd.mode = DPLL_MODE_MANUAL;
> +	dpll->dpll_p_pd.clock_id = clock_id;
> +	dpll->dpll_p_pd.status = DPLL_LOCK_STATUS_UNLOCKED;
> +
> +	err = dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &nsim_dds_ops,
> +				   &dpll->dpll_p_pd);
> +	if (err)
> +		goto p_reg;
> +
> +	/* Create first pin (GNSS) */
> +	err = nsim_fill_pin_properties(&dpll->pp_gnss, "GNSS",
> +				       DPLL_PIN_TYPE_GNSS,
> +				       PIN_GNSS_CAPABILITIES, 1,
> +				       DPLL_PIN_FREQUENCY_1_HZ,
> +				       DPLL_PIN_FREQUENCY_1_HZ);
> +	if (err)
> +		goto pp_gnss;
> +	dpll->p_gnss =
> +		dpll_pin_get(clock_id, PIN_GNSS, THIS_MODULE, &dpll->pp_gnss);
> +	if (IS_ERR(dpll->p_gnss))
> +		goto p_gnss;
> +	nsim_fill_pin_pd(&dpll->p_gnss_pd, DPLL_PIN_FREQUENCY_1_HZ,
> +			 PIN_GNSS_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
> +	err = dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
> +				&dpll->p_gnss_pd);
> +	if (err)
> +		goto e_gnss_reg;
> +
> +	/* Create second pin (PPS) */
> +	err = nsim_fill_pin_properties(&dpll->pp_pps, "PPS", DPLL_PIN_TYPE_EXT,
> +				       PIN_PPS_CAPABILITIES, 1,
> +				       DPLL_PIN_FREQUENCY_1_HZ,
> +				       DPLL_PIN_FREQUENCY_1_HZ);
> +	if (err)
> +		goto pp_pps;
> +	dpll->p_pps =
> +		dpll_pin_get(clock_id, PIN_PPS, THIS_MODULE, &dpll->pp_pps);
> +	if (IS_ERR(dpll->p_pps))
> +		goto p_pps;

This branch will cause the function to return err.
However, err is set to 0 here. Perhaps it should be set
to a negative error value instead?

Flagged by Smatch.

> +	nsim_fill_pin_pd(&dpll->p_pps_pd, DPLL_PIN_FREQUENCY_1_HZ,
> +			 PIN_PPS_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
> +	err = dpll_pin_register(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
> +				&dpll->p_pps_pd);
> +	if (err)
> +		goto e_pps_reg;
> +	err = dpll_pin_register(dpll->dpll_p, dpll->p_pps, &nsim_pin_ops,
> +				&dpll->p_pps_pd);
> +	if (err)
> +		goto p_pps_reg;
> +
> +	dpll->pp_rclk =
> +		kcalloc(ports_count, sizeof(*dpll->pp_rclk), GFP_KERNEL);
> +	dpll->p_rclk = kcalloc(ports_count, sizeof(*dpll->p_rclk), GFP_KERNEL);
> +	dpll->p_rclk_pd =
> +		kcalloc(ports_count, sizeof(*dpll->p_rclk_pd), GFP_KERNEL);
> +
> +	return 0;
> +
> +p_pps_reg:
> +	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
> +			    &dpll->p_pps_pd);
> +e_pps_reg:
> +	dpll_pin_put(dpll->p_pps);
> +p_pps:
> +	nsim_free_pin_properties(&dpll->pp_pps);
> +pp_pps:
> +	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
> +			    &dpll->p_gnss_pd);
> +e_gnss_reg:
> +	dpll_pin_put(dpll->p_gnss);
> +p_gnss:
> +	nsim_free_pin_properties(&dpll->pp_gnss);
> +pp_gnss:
> +	dpll_device_unregister(dpll->dpll_p, &nsim_dds_ops, &dpll->dpll_p_pd);
> +p_reg:
> +	dpll_device_put(dpll->dpll_p);
> +dpll_p:
> +	dpll_device_unregister(dpll->dpll_e, &nsim_dds_ops, &dpll->dpll_e_pd);
> +e_reg:
> +	dpll_device_put(dpll->dpll_e);
> +dpll_e:
> +	return err;
> +}

...

> +int nsim_rclk_init(struct netdevsim *ns)
> +{
> +	struct nsim_dpll *dpll;
> +	unsigned int index;
> +	int err, devid;
> +	char *name;
> +
> +	devid = ns->nsim_dev->nsim_bus_dev->dev.id;

devid is set but otherwise unused in this function.

Flagged gcc-14 and clang-16 W=1 builds.

> +	index = ns->nsim_dev_port->port_index;
> +	dpll = &ns->nsim_dev->dpll;
> +	err = -ENOMEM;
> +
> +	name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
> +	if (!name)
> +		goto err;
> +
> +	/* Get EEC DPLL */
> +	if (IS_ERR(dpll->dpll_e))
> +		goto dpll;
> +
> +	/* Get PPS DPLL */
> +	if (IS_ERR(dpll->dpll_p))
> +		goto dpll;
> +
> +	/* Create Recovered clock pin (RCLK) */
> +	nsim_fill_pin_properties(&dpll->pp_rclk[index], name,
> +				 DPLL_PIN_TYPE_SYNCE_ETH_PORT,
> +				 PIN_RCLK_CAPABILITIES, 1, 1e6, 125e6);
> +	dpll->p_rclk[index] = dpll_pin_get(dpll->dpll_e_pd.clock_id,
> +					   PIN_RCLK + index, THIS_MODULE,
> +					   &dpll->pp_rclk[index]);
> +	kfree(name);
> +	if (IS_ERR(dpll->p_rclk[index]))
> +		goto p_rclk;

This branch will call kfree(name), but this has already been done above.
Likewise for the other goto branches below.

Flagged by Smatch.

> +	nsim_fill_pin_pd(&dpll->p_rclk_pd[index], DPLL_PIN_FREQUENCY_10_MHZ,
> +			 PIN_RCLK_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
> +	err = dpll_pin_register(dpll->dpll_e, dpll->p_rclk[index],
> +				&nsim_pin_ops, &dpll->p_rclk_pd[index]);
> +	if (err)
> +		goto dpll_e_reg;
> +	err = dpll_pin_register(dpll->dpll_p, dpll->p_rclk[index],
> +				&nsim_pin_ops, &dpll->p_rclk_pd[index]);
> +	if (err)
> +		goto dpll_p_reg;
> +
> +	netdev_dpll_pin_set(ns->netdev, dpll->p_rclk[index]);
> +
> +	return 0;
> +
> +dpll_p_reg:
> +	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk[index], &nsim_pin_ops,
> +			    &dpll->p_rclk_pd[index]);
> +dpll_e_reg:
> +	dpll_pin_put(dpll->p_rclk[index]);
> +p_rclk:
> +	nsim_free_pin_properties(&dpll->pp_rclk[index]);
> +dpll:
> +	kfree(name);
> +err:
> +	return err;
> +}

...