Re: [PATCH net-next 06/11] dpll: netlink: Add DPLL framework base functions

[Simon Horman <simon.horman@corigine.com>]

On Thu, Jul 20, 2023 at 10:18:58AM +0100, Vadim Fedorenko wrote:
> DPLL framework is used to represent and configure DPLL devices
> in systems. Each device that has DPLL and can configure inputs
> and outputs can use this framework.

Hi Vadim,

some minor feedback from my side.

> 
> Implement dpll netlink framework functions for enablement of dpll
> subsytem netlink family.

subsytem -> subsystem

...

> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c

...

> +/**
> + * dpll_msg_add_pin_handle - attach pin handle attribute to a given message
> + * @msg: pointer to sk_buff message to attach a pin handle
> + * @pin: pin pointer
> + *
> + * Return:
> + * * 0 - success
> + * * -EMSGSIZE - no space in message to attach pin handle
> + */
> +int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin)

This function seems to only be used in this file.
Should it be static.

...

> +static int
> +dpll_msg_add_pin_on_dpll_state(struct sk_buff *msg, struct dpll_pin *pin,
> +			       struct dpll_pin_ref *ref,
> +			       struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> +	struct dpll_device *dpll = ref->dpll;
> +	enum dpll_pin_state state;
> +	int ret;
> +
> +

nit: one blank line is enough

...

> +static int
> +dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> +		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> +	struct dpll_device *dpll = ref->dpll;
> +	struct nlattr *nest;
> +	int fs, ret;
> +	u64 freq;
> +
> +	if (!ops->frequency_get)
> +		return 0;
> +	ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
> +				 dpll_priv(dpll), &freq, extack);
> +	if (ret)
> +		return ret;
> +	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq, 0))
> +		return -EMSGSIZE;
> +	for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
> +		nest = nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
> +		if (!nest)
> +			return -EMSGSIZE;
> +		freq = pin->prop->freq_supported[fs].min;
> +		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(freq),
> +				   &freq, 0)) {

nit: The indention of the line above isn't quite right.
     There is one space too many.

> +			nla_nest_cancel(msg, nest);
> +			return -EMSGSIZE;
> +		}
> +		freq = pin->prop->freq_supported[fs].max;
> +		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(freq),
> +				   &freq, 0)) {

Ditto.

> +			nla_nest_cancel(msg, nest);
> +			return -EMSGSIZE;
> +		}
> +		nla_nest_end(msg, nest);
> +	}
> +
> +	return 0;
> +}

...

> +static int
> +dpll_device_event_send(enum dpll_cmd event, struct dpll_device *dpll)
> +{
> +	struct sk_buff *msg;
> +	void *hdr;
> +	int ret;
> +
> +	if (WARN_ON(!xa_get_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED)))
> +		return -ENODEV;
> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +	hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, event);
> +	if (!hdr)
> +		goto err_free_msg;

ret is uninitialised here, but it is used in the error path.

This is flagged by a clang-16 W=1 build, and Smatch.

> +	ret = dpll_device_get_one(dpll, msg, NULL);
> +	if (ret)
> +		goto err_cancel_msg;
> +	genlmsg_end(msg, hdr);
> +	genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
> +
> +	return 0;
> +
> +err_cancel_msg:
> +	genlmsg_cancel(msg, hdr);
> +err_free_msg:
> +	nlmsg_free(msg);
> +
> +	return ret;
> +}

...

> +int __dpll_device_change_ntf(struct dpll_device *dpll)
> +{
> +	return dpll_device_event_send(DPLL_CMD_DEVICE_CHANGE_NTF, dpll);
> +}

Should this function be static?

...

> +static int
> +dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin)
> +{
> +	struct sk_buff *msg;
> +	void *hdr;
> +	int ret;
> +
> +	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
> +		return -ENODEV;
> +
> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, event);
> +	if (!hdr)
> +		goto err_free_msg;

It looks like ret is used uninitialised here too.

> +	ret = dpll_cmd_pin_get_one(msg, pin, NULL);
> +	if (ret)
> +		goto err_cancel_msg;
> +	genlmsg_end(msg, hdr);
> +	genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
> +
> +	return 0;
> +
> +err_cancel_msg:
> +	genlmsg_cancel(msg, hdr);
> +err_free_msg:
> +	nlmsg_free(msg);
> +
> +	return ret;
> +}

...

> +void
> +dpll_unlock_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
> +		   struct genl_info *info)

The indentation of the line above is not correct.
There are two spaces too many.

...

> +void dpll_netlink_finish(void)
> +{
> +	genl_unregister_family(&dpll_nl_family);
> +}

Should this function be static?

...