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

Simon Horman
2023-07-24T16:34:23Z

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?

...