Re: [RFC PATCH v8 04/10] dpll: netlink: Add DPLL framework base functions
From: Jiri Pirko
Date: Sun Jun 11 2023 - 07:42:38 EST
Fri, Jun 09, 2023 at 02:18:47PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote:
>From: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
Arkadiusz, I think it would be appropriate to change the authorship
of this and other patches to you. I believe that you did vast majority
of the lines by now. Vadim, would you mind?
>
>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.
>
>Implement dpll netlink framework functions for enablement of dpll
>subsytem netlink family.
>
>Co-developed-by: Milena Olech <milena.olech@xxxxxxxxx>
>Signed-off-by: Milena Olech <milena.olech@xxxxxxxxx>
>Co-developed-by: Michal Michalik <michal.michalik@xxxxxxxxx>
>Signed-off-by: Michal Michalik <michal.michalik@xxxxxxxxx>
>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
>Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>---
> drivers/dpll/dpll_netlink.c | 1183 +++++++++++++++++++++++++++++++++++
> drivers/dpll/dpll_netlink.h | 44 ++
Overall, this looks very good. I did take couple of comments below.
Thanks for you work!
> 2 files changed, 1227 insertions(+)
> create mode 100644 drivers/dpll/dpll_netlink.c
> create mode 100644 drivers/dpll/dpll_netlink.h
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>new file mode 100644
>index 000000000000..44d9699c9e6c
>--- /dev/null
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -0,0 +1,1183 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Generic netlink for DPLL management framework
>+ *
>+ * Copyright (c) 2023 Meta Platforms, Inc. and affiliates
>+ * Copyright (c) 2023 Intel and affiliates
>+ *
>+ */
>+#include <linux/module.h>
>+#include <linux/kernel.h>
>+#include <net/genetlink.h>
>+#include "dpll_core.h"
>+#include "dpll_nl.h"
>+#include <uapi/linux/dpll.h>
>+
>+static int __dpll_pin_change_ntf(struct dpll_pin *pin);
Could you try to reshuffle the code to avoid forward declarations?
>+
>+struct dpll_dump_ctx {
>+ unsigned long idx;
>+};
>+
>+static struct dpll_dump_ctx *dpll_dump_context(struct netlink_callback *cb)
>+{
>+ return (struct dpll_dump_ctx *)cb->ctx;
>+}
>+
>+static int
>+dpll_msg_add_dev_handle(struct sk_buff *msg, struct dpll_device *dpll)
It is odd to see this helper here and the dpll_msg_add_pin_handle() not.
Introduce dpll_msg_add_pin_handle() here right away and only export it
later on in "netdev: expose DPLL pin handle for netdevice".
>+{
>+ if (nla_put_u32(msg, DPLL_A_ID, dpll->id))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_mode(struct sk_buff *msg, struct dpll_device *dpll,
>+ struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+ enum dpll_mode mode;
>+
>+ if (WARN_ON(!ops->mode_get))
>+ return -EOPNOTSUPP;
>+ if (ops->mode_get(dpll, dpll_priv(dpll), &mode, extack))
>+ return -EFAULT;
I'm pretty sure I commented this before. But again, please get the
value the driver op returned and return it.
>+ if (nla_put_u8(msg, DPLL_A_MODE, mode))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>+ struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+ enum dpll_lock_status status;
>+
>+ if (WARN_ON(!ops->lock_status_get))
>+ return -EOPNOTSUPP;
>+ if (ops->lock_status_get(dpll, dpll_priv(dpll), &status, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ if (nla_put_u8(msg, DPLL_A_LOCK_STATUS, status))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_temp(struct sk_buff *msg, struct dpll_device *dpll,
>+ struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+ s32 temp;
>+
>+ if (!ops->temp_get)
>+ return -EOPNOTSUPP;
>+ if (ops->temp_get(dpll, dpll_priv(dpll), &temp, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ if (nla_put_s32(msg, DPLL_A_TEMP, temp))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_pin_prio(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;
>+ u32 prio;
>+
>+ if (!ops->prio_get)
>+ return -EOPNOTSUPP;
>+ if (ops->prio_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+ dpll_priv(dpll), &prio, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ if (nla_put_u32(msg, DPLL_A_PIN_PRIO, prio))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+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;
>+
>+ if (!ops->state_on_dpll_get)
>+ return -EOPNOTSUPP;
>+ if (ops->state_on_dpll_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+ dpll_priv(dpll), &state, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ if (nla_put_u8(msg, DPLL_A_PIN_STATE, state))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_pin_direction(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_direction direction;
>+
>+ if (!ops->direction_get)
>+ return -EOPNOTSUPP;
>+ if (ops->direction_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+ dpll_priv(dpll), &direction, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ if (nla_put_u8(msg, DPLL_A_PIN_DIRECTION, direction))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+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,
>+ bool dump_freq_supported)
>+{
>+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>+ struct dpll_device *dpll = ref->dpll;
>+ struct nlattr *nest;
>+ u64 freq;
>+ int fs;
>+
>+ if (!ops->frequency_get)
>+ return -EOPNOTSUPP;
Return 0 and avoid the check of -EOPNOTSUPP in the caller.
>+ if (ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+ dpll_priv(dpll), &freq, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq, 0))
>+ return -EMSGSIZE;
>+ if (!dump_freq_supported)
>+ return 0;
>+ 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)) {
>+ 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)) {
>+ nla_nest_cancel(msg, nest);
>+ return -EMSGSIZE;
>+ }
>+ nla_nest_end(msg, nest);
>+ }
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_pin_parents(struct sk_buff *msg, struct dpll_pin *pin,
>+ struct dpll_pin_ref *dpll_ref,
>+ struct netlink_ext_ack *extack)
>+{
>+ enum dpll_pin_state state;
>+ struct dpll_pin_ref *ref;
>+ struct dpll_pin *ppin;
>+ struct nlattr *nest;
>+ unsigned long index;
>+ int ret;
>+
>+ xa_for_each(&pin->parent_refs, index, ref) {
>+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>+ void *parent_priv;
>+
>+ ppin = ref->pin;
>+ parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin);
>+ if (WARN_ON(!ops->state_on_pin_get))
Wait, so you WARN during user comment on something that driver didn't
fill up? Plese move the check and WARN to the registration function.
>+ return -EFAULT;
>+ ret = ops->state_on_pin_get(pin,
>+ dpll_pin_on_pin_priv(ppin, pin),
>+ ppin, parent_priv, &state, extack);
>+ if (ret)
>+ return -EFAULT;
Return ret please.
>+ nest = nla_nest_start(msg, DPLL_A_PIN_PARENT);
>+ if (!nest)
>+ return -EMSGSIZE;
>+ if (nla_put_u32(msg, DPLL_A_PIN_ID, ppin->id)) {
>+ ret = -EMSGSIZE;
>+ goto nest_cancel;
>+ }
>+ if (nla_put_u8(msg, DPLL_A_PIN_STATE, state)) {
>+ ret = -EMSGSIZE;
>+ goto nest_cancel;
>+ }
>+ nla_nest_end(msg, nest);
>+ }
>+
>+ return 0;
>+
>+nest_cancel:
>+ nla_nest_cancel(msg, nest);
>+ return ret;
>+}
>+
>+static int
>+dpll_msg_add_pin_dplls(struct sk_buff *msg, struct dpll_pin *pin,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct dpll_pin_ref *ref;
>+ struct nlattr *attr;
>+ unsigned long index;
>+ int ret;
>+
>+ xa_for_each(&pin->dpll_refs, index, ref) {
>+ attr = nla_nest_start(msg, DPLL_A_PIN_PARENT);
>+ if (!attr)
>+ return -EMSGSIZE;
>+ ret = dpll_msg_add_dev_handle(msg, ref->dpll);
>+ if (ret)
>+ goto nest_cancel;
>+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ goto nest_cancel;
>+ ret = dpll_msg_add_pin_prio(msg, pin, ref, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ goto nest_cancel;
>+ ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
>+ if (ret)
>+ goto nest_cancel;
>+ nla_nest_end(msg, attr);
>+ }
>+
>+ return 0;
>+
>+nest_cancel:
>+ nla_nest_end(msg, attr);
>+ return ret;
>+}
>+
>+static int
>+dpll_cmd_pin_fill_details(struct sk_buff *msg, struct dpll_pin *pin,
"details"? Sound odd. I don't think that "DPLL_A_PIN_ID" is a detail
for example. Why don't you inline this in the __dpll_cmd_pin_dump_one()
function below?
>+ struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_pin_properties *prop = pin->prop;
>+ int ret;
>+
>+ if (nla_put_u32(msg, DPLL_A_PIN_ID, pin->id))
>+ return -EMSGSIZE;
>+ if (nla_put_string(msg, DPLL_A_MODULE_NAME, module_name(pin->module)))
>+ return -EMSGSIZE;
>+ if (nla_put_64bit(msg, DPLL_A_CLOCK_ID, sizeof(pin->clock_id),
>+ &pin->clock_id, 0))
>+ return -EMSGSIZE;
>+ if (prop->board_label &&
>+ nla_put_string(msg, DPLL_A_PIN_BOARD_LABEL, prop->board_label))
>+ return -EMSGSIZE;
>+ if (prop->panel_label &&
>+ nla_put_string(msg, DPLL_A_PIN_PANEL_LABEL, prop->panel_label))
>+ return -EMSGSIZE;
>+ if (prop->package_label &&
>+ nla_put_string(msg, DPLL_A_PIN_PACKAGE_LABEL,
>+ prop->package_label))
>+ return -EMSGSIZE;
>+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, prop->type))
>+ return -EMSGSIZE;
>+ if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, prop->capabilities))
>+ return -EMSGSIZE;
>+ ret = dpll_msg_add_pin_freq(msg, pin, ref, extack, true);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ return 0;
>+}
>+
>+static int
>+__dpll_cmd_pin_dump_one(struct sk_buff *msg, struct dpll_pin *pin,
>+ struct netlink_ext_ack *extack)
To be consistent with dpll_device_get_one(), call this function
dpll_pin_get_one() please.
>+{
>+ struct dpll_pin_ref *ref;
>+ int ret;
>+
>+ ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>+ if (!ref)
>+ return -EFAULT;
-EINVAL. But it should never happen anyway. Perhaps better to avoid the
check entirely.
>+ ret = dpll_cmd_pin_fill_details(msg, pin, ref, extack);
>+ if (ret)
>+ return ret;
>+ ret = dpll_msg_add_pin_parents(msg, pin, ref, extack);
>+ if (ret)
>+ return ret;
>+ if (!xa_empty(&pin->dpll_refs)) {
Drop this check, not needed.
>+ ret = dpll_msg_add_pin_dplls(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ }
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
>+ struct netlink_ext_ack *extack)
>+{
>+ enum dpll_mode mode;
>+ int ret;
>+
>+ ret = dpll_msg_add_dev_handle(msg, dpll);
>+ if (ret)
>+ return ret;
>+ if (nla_put_string(msg, DPLL_A_MODULE_NAME, module_name(dpll->module)))
>+ return -EMSGSIZE;
>+ if (nla_put_64bit(msg, DPLL_A_CLOCK_ID, sizeof(dpll->clock_id),
>+ &dpll->clock_id, 0))
>+ return -EMSGSIZE;
>+ ret = dpll_msg_add_temp(msg, dpll, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ret = dpll_msg_add_lock_status(msg, dpll, extack);
>+ if (ret)
>+ return ret;
>+ ret = dpll_msg_add_mode(msg, dpll, extack);
>+ if (ret)
>+ return ret;
>+ for (mode = DPLL_MODE_MANUAL; mode <= DPLL_MODE_MAX; mode++)
>+ if (test_bit(mode, &dpll->mode_supported_mask))
>+ if (nla_put_s32(msg, DPLL_A_MODE_SUPPORTED, mode))
>+ return -EMSGSIZE;
>+ if (nla_put_u8(msg, DPLL_A_TYPE, dpll->type))
>+ return -EMSGSIZE;
>+
>+ return ret;
>+}
>+
>+static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
>+{
>+ int fs;
>+
>+ for (fs = 0; fs < pin->prop->freq_supported_num; fs++)
>+ if (freq >= pin->prop->freq_supported[fs].min &&
Avoid double space here ^^
>+ freq <= pin->prop->freq_supported[fs].max)
Avoid double space here ^^
>+ return true;
>+ return false;
>+}
>+
>+static int
>+dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>+ struct netlink_ext_ack *extack)
>+{
>+ u64 freq = nla_get_u64(a);
>+ struct dpll_pin_ref *ref;
>+ unsigned long i;
>+ int ret;
>+
>+ if (!dpll_pin_is_freq_supported(pin, freq))
Fill a proper extack telling the user what's wrong please.
Could you please check the rest of the cmd attr checks and make sure
the extack is always filled with meaningful message?
>+ return -EINVAL;
>+
>+ xa_for_each(&pin->dpll_refs, i, ref) {
>+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>+ struct dpll_device *dpll = ref->dpll;
>+
>+ if (!ops->frequency_set)
>+ return -EOPNOTSUPP;
>+ ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+ dpll, dpll_priv(dpll), freq, extack);
>+ if (ret)
>+ return -EFAULT;
return "ret"
>+ __dpll_pin_change_ntf(pin);
>+ }
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
>+ enum dpll_pin_state state,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct dpll_pin_ref *parent_ref;
>+ const struct dpll_pin_ops *ops;
>+ struct dpll_pin_ref *dpll_ref;
>+ struct dpll_pin *parent;
>+ unsigned long i;
>+
>+ if (!(DPLL_PIN_CAPS_STATE_CAN_CHANGE & pin->prop->capabilities))
>+ return -EOPNOTSUPP;
>+ parent = xa_load(&dpll_pin_xa, parent_idx);
>+ if (!parent)
>+ return -EINVAL;
>+ parent_ref = xa_load(&pin->parent_refs, parent->pin_idx);
>+ if (!parent_ref)
>+ return -EINVAL;
>+ xa_for_each(&parent->dpll_refs, i, dpll_ref) {
>+ ops = dpll_pin_ops(parent_ref);
>+ if (!ops->state_on_pin_set)
>+ return -EOPNOTSUPP;
>+ if (ops->state_on_pin_set(pin,
>+ dpll_pin_on_pin_priv(parent, pin),
>+ parent,
>+ dpll_pin_on_dpll_priv(dpll_ref->dpll,
>+ parent),
>+ state, extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ }
>+ __dpll_pin_change_ntf(pin);
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_pin_state_set(struct dpll_device *dpll, struct dpll_pin *pin,
>+ enum dpll_pin_state state,
>+ struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_pin_ops *ops;
>+ struct dpll_pin_ref *ref;
>+
>+ if (!(DPLL_PIN_CAPS_STATE_CAN_CHANGE & pin->prop->capabilities))
>+ return -EOPNOTSUPP;
>+ ref = xa_load(&pin->dpll_refs, dpll->device_idx);
>+ if (!ref)
>+ return -EFAULT;
-EINVAL. But looks like this should never happen. Perhaps just
WARN_ON(!ref) and don't check-return.
>+ ops = dpll_pin_ops(ref);
>+ if (!ops->state_on_dpll_set)
>+ return -EOPNOTSUPP;
>+ if (ops->state_on_dpll_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+ dpll_priv(dpll), state, extack))
>+ return -EINVAL;
>+ __dpll_pin_change_ntf(pin);
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_pin_prio_set(struct dpll_device *dpll, struct dpll_pin *pin,
>+ u32 prio, struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_pin_ops *ops;
>+ struct dpll_pin_ref *ref;
>+
>+ if (!(DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE & pin->prop->capabilities))
>+ return -EOPNOTSUPP;
>+ ref = xa_load(&pin->dpll_refs, dpll->device_idx);
>+ if (!ref)
>+ return -EFAULT;
Same here.
>+ ops = dpll_pin_ops(ref);
>+ if (!ops->prio_set)
>+ return -EOPNOTSUPP;
>+ if (ops->prio_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+ dpll_priv(dpll), prio, extack))
>+ return -EINVAL;
>+ __dpll_pin_change_ntf(pin);
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll,
>+ enum dpll_pin_direction direction,
>+ struct netlink_ext_ack *extack)
>+{
>+ const struct dpll_pin_ops *ops;
>+ struct dpll_pin_ref *ref;
>+
>+ if (!(DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE & pin->prop->capabilities))
>+ return -EOPNOTSUPP;
>+
>+ ref = xa_load(&pin->dpll_refs, dpll->device_idx);
>+ if (!ref)
>+ return -EFAULT;
Same here. This calls for a helper :)
>+ ops = dpll_pin_ops(ref);
>+ if (!ops->direction_set)
>+ return -EOPNOTSUPP;
>+ if (ops->direction_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+ dpll, dpll_priv(dpll), direction,
>+ extack))
>+ return -EFAULT;
please get the value the driver op returned and return it.
>+ __dpll_pin_change_ntf(pin);
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_pin_parent_set(struct dpll_pin *pin, struct nlattr *parent_nest,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct nlattr *tb[DPLL_A_MAX + 1];
>+ enum dpll_pin_direction direction;
>+ u32 ppin_idx, pdpll_idx, prio;
>+ enum dpll_pin_state state;
>+ struct dpll_pin_ref *ref;
>+ struct dpll_device *dpll;
>+ int ret;
>+
>+ nla_parse_nested(tb, DPLL_A_MAX, parent_nest,
>+ NULL, extack);
>+ if ((tb[DPLL_A_ID] && tb[DPLL_A_PIN_ID]) ||
>+ !(tb[DPLL_A_ID] || tb[DPLL_A_PIN_ID])) {
>+ NL_SET_ERR_MSG(extack, "one parent id expected");
>+ return -EINVAL;
>+ }
>+ if (tb[DPLL_A_ID]) {
>+ pdpll_idx = nla_get_u32(tb[DPLL_A_ID]);
>+ dpll = xa_load(&dpll_device_xa, pdpll_idx);
>+ if (!dpll)
>+ return -EINVAL;
>+ ref = xa_load(&pin->dpll_refs, dpll->device_idx);
>+ if (!ref)
>+ return -EINVAL;
>+ if (tb[DPLL_A_PIN_STATE]) {
>+ state = nla_get_u8(tb[DPLL_A_PIN_STATE]);
>+ ret = dpll_pin_state_set(dpll, pin, state, extack);
>+ if (ret)
>+ return ret;
>+ }
>+ if (tb[DPLL_A_PIN_PRIO]) {
>+ prio = nla_get_u8(tb[DPLL_A_PIN_PRIO]);
>+ ret = dpll_pin_prio_set(dpll, pin, prio, extack);
>+ if (ret)
>+ return ret;
>+ }
>+ if (tb[DPLL_A_PIN_DIRECTION]) {
>+ direction = nla_get_u8(tb[DPLL_A_PIN_DIRECTION]);
>+ ret = dpll_pin_direction_set(pin, dpll, direction,
>+ extack);
>+ if (ret)
>+ return ret;
>+ }
>+ } else if (tb[DPLL_A_PIN_ID]) {
>+ ppin_idx = nla_get_u32(tb[DPLL_A_PIN_ID]);
>+ state = nla_get_u8(tb[DPLL_A_PIN_STATE]);
>+ ret = dpll_pin_on_pin_state_set(pin, ppin_idx, state, extack);
>+ if (ret)
>+ return ret;
>+ }
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
>+{
>+ int rem, ret = -EINVAL;
>+ struct nlattr *a;
>+
>+ nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>+ genlmsg_len(info->genlhdr), rem) {
>+ switch (nla_type(a)) {
>+ case DPLL_A_PIN_FREQUENCY:
>+ ret = dpll_pin_freq_set(pin, a, info->extack);
>+ if (ret)
>+ return ret;
>+ break;
>+ case DPLL_A_PIN_PARENT:
>+ ret = dpll_pin_parent_set(pin, a, info->extack);
>+ if (ret)
>+ return ret;
>+ break;
>+ case DPLL_A_PIN_ID:
>+ case DPLL_A_ID:
>+ break;
>+ default:
>+ NL_SET_ERR_MSG_FMT(info->extack,
>+ "unsupported attribute (%d)",
>+ nla_type(a));
>+ return -EINVAL;
>+ }
>+ }
>+
>+ return 0;
>+}
>+
>+static struct dpll_pin *
>+dpll_pin_find(u64 clock_id, struct nlattr *mod_name_attr,
>+ enum dpll_pin_type type, struct nlattr *board_label,
>+ struct nlattr *panel_label, struct nlattr *package_label)
>+{
>+ bool board_match, panel_match, package_match;
>+ struct dpll_pin *pin_match = NULL, *pin;
>+ const struct dpll_pin_properties *prop;
>+ bool cid_match, mod_match, type_match;
>+ unsigned long i;
>+
>+ xa_for_each(&dpll_pin_xa, i, pin) {
>+ if (xa_empty(&pin->dpll_refs))
This filters out unregistered, right? Could you please introduce a
"REGISTERED" mark and iterate only over list of registered? Similar to
what you have for device.
>+ continue;
>+ prop = pin->prop;
>+ cid_match = clock_id ? pin->clock_id == clock_id : true;
>+ mod_match = mod_name_attr && module_name(pin->module) ?
>+ !nla_strcmp(mod_name_attr,
>+ module_name(pin->module)) : true;
>+ type_match = type ? prop->type == type : true;
>+ board_match = board_label && prop->board_label ?
>+ !nla_strcmp(board_label, prop->board_label) : true;
>+ panel_match = panel_label && prop->panel_label ?
>+ !nla_strcmp(panel_label, prop->panel_label) : true;
>+ package_match = package_label && prop->package_label ?
>+ !nla_strcmp(package_label,
>+ prop->package_label) : true;
>+ if (cid_match && mod_match && type_match && board_match &&
>+ panel_match && package_match) {
>+ if (pin_match)
Double match, rigth? Fillup the extack telling the user what happened.
>+ return NULL;
>+ pin_match = pin;
>+ };
>+ }
>+
>+ return pin_match;
>+}
>+
>+static int
>+dpll_pin_find_from_nlattr(struct genl_info *info, struct sk_buff *skb)
>+{
>+ struct nlattr *attr, *mod_name_attr = NULL, *board_label_attr = NULL,
>+ *panel_label_attr = NULL, *package_label_attr = NULL;
>+ struct dpll_pin *pin = NULL;
>+ enum dpll_pin_type type = 0;
>+ u64 clock_id = 0;
>+ int rem = 0;
>+
>+ nla_for_each_attr(attr, genlmsg_data(info->genlhdr),
>+ genlmsg_len(info->genlhdr), rem) {
>+ switch (nla_type(attr)) {
>+ case DPLL_A_CLOCK_ID:
>+ if (clock_id)
>+ return -EINVAL;
Extack
>+ clock_id = nla_get_u64(attr);
>+ break;
>+ case DPLL_A_MODULE_NAME:
>+ if (mod_name_attr)
>+ return -EINVAL;
Extack
>+ mod_name_attr = attr;
>+ break;
>+ case DPLL_A_PIN_TYPE:
>+ if (type)
>+ return -EINVAL;
Extack
>+ type = nla_get_u8(attr);
>+ break;
>+ case DPLL_A_PIN_BOARD_LABEL:
>+ if (board_label_attr)
>+ return -EINVAL;
Extack
>+ board_label_attr = attr;
>+ break;
>+ case DPLL_A_PIN_PANEL_LABEL:
>+ if (panel_label_attr)
>+ return -EINVAL;
Extack
>+ panel_label_attr = attr;
>+ break;
>+ case DPLL_A_PIN_PACKAGE_LABEL:
>+ if (package_label_attr)
>+ return -EINVAL;
Extack
You can use goto with one "duplicate attribute" message.
>+ package_label_attr = attr;
>+ break;
>+ default:
>+ break;
>+ }
>+ }
>+ if (!(clock_id || mod_name_attr || board_label_attr ||
>+ panel_label_attr || package_label_attr))
>+ return -EINVAL;
>+ pin = dpll_pin_find(clock_id, mod_name_attr, type, board_label_attr,
>+ panel_label_attr, package_label_attr);
Error is either "notfound" of "duplicate match". Have the function
dpll_pin_find() return ERR_PTR with -ENODEV / -EINVAL and let
the function dpll_pin_find() also fill-up the proper extack inside.
>+ if (!pin)
>+ return -EINVAL;
>+ if (nla_put_u32(skb, DPLL_A_PIN_ID, pin->id))
Please move this call to the caller. This function should return ERR_PTR
or dpll_pin pointer.
>+ return -EMSGSIZE;
>+ return 0;
>+}
>+
>+int dpll_nl_pin_id_get_doit(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct sk_buff *msg;
>+ struct nlattr *hdr;
>+ int ret;
>+
>+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+ hdr = genlmsg_put_reply(msg, info, &dpll_nl_family, 0,
>+ DPLL_CMD_PIN_ID_GET);
>+ if (!hdr)
>+ return -EMSGSIZE;
>+
>+ ret = dpll_pin_find_from_nlattr(info, msg);
>+ if (ret) {
>+ nlmsg_free(msg);
>+ return ret;
>+ }
>+ genlmsg_end(msg, hdr);
This does not seem to be working:
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do device-id-get --json '{"module-name": "mlx5_dpll"}'
{'id': 0}
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-id-get --json '{"module-name": "mlx5_dpll"}'
Traceback (most recent call last):
File "/mnt/share156/jiri/net-next/./tools/net/ynl/cli.py", line 52, in <module>
main()
File "/mnt/share156/jiri/net-next/./tools/net/ynl/cli.py", line 40, in main
reply = ynl.do(args.do, attrs)
File "/mnt/share156/jiri/net-next/tools/net/ynl/lib/ynl.py", line 596, in do
return self._op(method, vals)
File "/mnt/share156/jiri/net-next/tools/net/ynl/lib/ynl.py", line 567, in _op
raise NlError(nl_msg)
lib.ynl.NlError: Netlink error: Invalid argument
nl_len = 36 (20) nl_flags = 0x100 nl_type = 2
error: -22
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do device-id-get --json '{"clock-id": "630763432553410540"}'
{'id': 0}
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-id-get --json '{"clock-id": "630763432553410540"}'
Traceback (most recent call last):
File "/mnt/share156/jiri/net-next/./tools/net/ynl/cli.py", line 52, in <module>
main()
File "/mnt/share156/jiri/net-next/./tools/net/ynl/cli.py", line 40, in main
reply = ynl.do(args.do, attrs)
File "/mnt/share156/jiri/net-next/tools/net/ynl/lib/ynl.py", line 596, in do
return self._op(method, vals)
File "/mnt/share156/jiri/net-next/tools/net/ynl/lib/ynl.py", line 567, in _op
raise NlError(nl_msg)
lib.ynl.NlError: Netlink error: Invalid argument
nl_len = 36 (20) nl_flags = 0x100 nl_type = 2
error: -22
>+
>+ return genlmsg_reply(msg, info);
>+}
>+
>+int dpll_nl_pin_get_doit(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct dpll_pin *pin = info->user_ptr[0];
>+ struct sk_buff *msg;
>+ struct nlattr *hdr;
>+ int ret;
>+
>+ if (!pin)
>+ return -ENODEV;
>+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+ hdr = genlmsg_put_reply(msg, info, &dpll_nl_family, 0,
>+ DPLL_CMD_PIN_GET);
>+ if (!hdr)
>+ return -EMSGSIZE;
>+ ret = __dpll_cmd_pin_dump_one(msg, pin, info->extack);
>+ if (ret) {
>+ nlmsg_free(msg);
>+ return ret;
>+ }
>+ genlmsg_end(msg, hdr);
>+
>+ return genlmsg_reply(msg, info);
>+}
>+
>+int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>+{
>+ struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
>+ struct dpll_pin *pin;
>+ struct nlattr *hdr;
>+ unsigned long i;
>+ int ret = 0;
>+
>+ xa_for_each_start(&dpll_pin_xa, i, pin, ctx->idx) {
>+ if (xa_empty(&pin->dpll_refs))
Same here, also use REGISTERED mark and iterate over them.
>+ continue;
>+ hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
>+ cb->nlh->nlmsg_seq,
>+ &dpll_nl_family, NLM_F_MULTI,
>+ DPLL_CMD_PIN_GET);
>+ if (!hdr) {
>+ ret = -EMSGSIZE;
>+ break;
>+ }
>+ ret = __dpll_cmd_pin_dump_one(skb, pin, cb->extack);
>+ if (ret) {
>+ genlmsg_cancel(skb, hdr);
>+ break;
>+ }
>+ genlmsg_end(skb, hdr);
>+ }
>+ if (ret == -EMSGSIZE) {
>+ ctx->idx = i;
>+ return skb->len;
>+ }
>+ return ret;
>+}
>+
>+int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct dpll_pin *pin = info->user_ptr[0];
>+
>+ return dpll_pin_set_from_nlattr(pin, info);
>+}
>+
>+static struct dpll_device *
>+dpll_device_find(u64 clock_id, struct nlattr *mod_name_attr,
>+ enum dpll_type type)
>+{
>+ struct dpll_device *dpll_match = NULL, *dpll;
>+ bool cid_match, mod_match, type_match;
>+ unsigned long i;
>+
>+ xa_for_each_marked(&dpll_device_xa, i, dpll, DPLL_REGISTERED) {
>+ cid_match = clock_id ? dpll->clock_id == clock_id : true;
>+ mod_match = mod_name_attr && module_name(dpll->module) ?
>+ !nla_strcmp(mod_name_attr,
>+ module_name(dpll->module)) : true;
>+ type_match = type ? dpll->type == type : true;
>+ if (cid_match && mod_match && type_match) {
>+ if (dpll_match)
Double match, rigth? Fillup the extack telling the user what happened.
>+ return NULL;
>+ dpll_match = dpll;
>+ }
>+ }
>+
>+ return dpll_match;
>+}
>+
>+static int
>+dpll_device_find_from_nlattr(struct genl_info *info, struct sk_buff *skb)
>+{
>+ struct nlattr *attr, *mod_name_attr = NULL;
>+ struct dpll_device *dpll = NULL;
>+ enum dpll_type type = 0;
>+ u64 clock_id = 0;
>+ int rem = 0;
>+
>+ nla_for_each_attr(attr, genlmsg_data(info->genlhdr),
>+ genlmsg_len(info->genlhdr), rem) {
>+ switch (nla_type(attr)) {
>+ case DPLL_A_CLOCK_ID:
>+ if (clock_id)
>+ return -EINVAL;
Extack
>+ clock_id = nla_get_u64(attr);
>+ break;
>+ case DPLL_A_MODULE_NAME:
>+ if (mod_name_attr)
>+ return -EINVAL;
Extack
>+ mod_name_attr = attr;
>+ break;
>+ case DPLL_A_TYPE:
>+ if (type)
>+ return -EINVAL;
Extack
You can use goto with one "duplicate attribute" message.
>+ type = nla_get_u8(attr);
>+ break;
>+ default:
>+ break;
>+ }
>+ }
>+
>+ if (!clock_id && !mod_name_attr && !type)
>+ return -EINVAL;
>+ dpll = dpll_device_find(clock_id, mod_name_attr, type);
Error is either "notfound" of "duplicate match". Have the function
dpll_device_find() return ERR_PTR with -ENODEV / -EINVAL and let
the function dpll_device_find() also fill-up the proper extack inside.
>+ if (!dpll)
>+ return -EINVAL;
>+
>+ return dpll_msg_add_dev_handle(skb, dpll);
Please move this call to the caller. This function should return ERR_PTR
or dpll_device pointer.
>+}
>+
>+static int
>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
Nit: Please move this function above dpll_device_find() to maintain the
same functions ordering as there is for similar pin functions above.
>+{
>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+ struct nlattr *tb[DPLL_A_MAX + 1];
>+ int ret = 0;
Drop pointless init.
>+
>+ nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
>+ genlmsg_len(info->genlhdr), NULL, info->extack);
>+ if (tb[DPLL_A_MODE]) {
>+ ret = ops->mode_set(dpll, dpll_priv(dpll),
>+ nla_get_u8(tb[DPLL_A_MODE]), info->extack);
>+ if (ret)
>+ return ret;
>+ }
>+
>+ return 0;
>+}
>+
>+int dpll_nl_device_id_get_doit(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct sk_buff *msg;
>+ struct nlattr *hdr;
>+ int ret;
>+
>+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+ hdr = genlmsg_put_reply(msg, info, &dpll_nl_family, 0,
>+ DPLL_CMD_DEVICE_ID_GET);
>+ if (!hdr)
>+ return -EMSGSIZE;
>+
>+ ret = dpll_device_find_from_nlattr(info, msg);
>+ if (ret) {
>+ nlmsg_free(msg);
>+ return ret;
>+ }
>+ genlmsg_end(msg, hdr);
>+
>+ return genlmsg_reply(msg, info);
>+}
>+
>+int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct dpll_device *dpll = info->user_ptr[0];
>+ struct sk_buff *msg;
>+ struct nlattr *hdr;
>+ int ret;
>+
>+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+ hdr = genlmsg_put_reply(msg, info, &dpll_nl_family, 0,
>+ DPLL_CMD_DEVICE_GET);
>+ if (!hdr)
>+ return -EMSGSIZE;
>+
>+ ret = dpll_device_get_one(dpll, msg, info->extack);
>+ if (ret) {
>+ nlmsg_free(msg);
>+ return ret;
>+ }
>+ genlmsg_end(msg, hdr);
>+
>+ return genlmsg_reply(msg, info);
>+}
>+
>+int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
>+{
>+ struct dpll_device *dpll = info->user_ptr[0];
>+
>+ return dpll_set_from_nlattr(dpll, info);
>+}
>+
>+int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>+{
>+ struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
>+ struct dpll_device *dpll;
>+ struct nlattr *hdr;
>+ unsigned long i;
>+ int ret = 0;
>+
>+ xa_for_each_start(&dpll_device_xa, i, dpll, ctx->idx) {
>+ if (!xa_get_mark(&dpll_device_xa, i, DPLL_REGISTERED))
Hmm, did you consider adding xa_for_each_marked_start?
>+ continue;
>+ hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
>+ cb->nlh->nlmsg_seq, &dpll_nl_family,
>+ NLM_F_MULTI, DPLL_CMD_DEVICE_GET);
>+ if (!hdr) {
>+ ret = -EMSGSIZE;
>+ break;
>+ }
>+ ret = dpll_device_get_one(dpll, skb, cb->extack);
>+ if (ret) {
>+ genlmsg_cancel(skb, hdr);
>+ break;
>+ }
>+ genlmsg_end(skb, hdr);
>+ }
>+ if (ret == -EMSGSIZE) {
>+ ctx->idx = i;
>+ return skb->len;
>+ }
>+ return ret;
>+}
>+
>+int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ struct dpll_device *dpll_id = NULL;
>+ u32 id;
>+
>+ if (!info->attrs[DPLL_A_ID])
>+ return -EINVAL;
>+
>+ mutex_lock(&dpll_lock);
>+ id = nla_get_u32(info->attrs[DPLL_A_ID]);
>+
>+ dpll_id = dpll_device_get_by_id(id);
>+ if (!dpll_id)
>+ goto unlock;
>+ info->user_ptr[0] = dpll_id;
>+ return 0;
>+unlock:
>+ mutex_unlock(&dpll_lock);
>+ return -ENODEV;
>+}
>+
>+void dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ mutex_unlock(&dpll_lock);
>+}
>+
>+int
>+dpll_lock_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ mutex_lock(&dpll_lock);
>+
>+ return 0;
>+}
>+
>+void
>+dpll_unlock_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ mutex_unlock(&dpll_lock);
>+}
>+
>+int dpll_lock_dumpit(struct netlink_callback *cb)
>+{
>+ mutex_lock(&dpll_lock);
>+
>+ return 0;
>+}
>+
>+int dpll_unlock_dumpit(struct netlink_callback *cb)
>+{
>+ mutex_unlock(&dpll_lock);
>+
>+ return 0;
>+}
>+
>+int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ int ret;
>+
>+ mutex_lock(&dpll_lock);
>+ if (!info->attrs[DPLL_A_PIN_ID]) {
Use GENL_REQ_ATTR_CHECK(info, DPLL_A_PIN_ID);
If fills-up the extack info about missing attr giving the user info
about what went wrong.
>+ ret = -EINVAL;
>+ goto unlock_dev;
>+ }
>+ info->user_ptr[0] = xa_load(&dpll_pin_xa,
>+ nla_get_u32(info->attrs[DPLL_A_PIN_ID]));
>+ if (!info->user_ptr[0]) {
Fill-up the extack message please.
>+ ret = -ENODEV;
>+ goto unlock_dev;
>+ }
>+
>+ return 0;
>+
>+unlock_dev:
>+ mutex_unlock(&dpll_lock);
>+ return ret;
>+}
>+
>+void dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ mutex_unlock(&dpll_lock);
>+}
>+
>+static int
>+dpll_device_event_send(enum dpll_cmd event, struct dpll_device *dpll)
>+{
>+ struct sk_buff *msg;
>+ int ret = -EMSGSIZE;
Drop the pointless init.
>+ void *hdr;
>+
>+ if (!xa_get_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED))
WARN_ON? The driver is buggy when he calls this.
>+ 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 out_free_msg;
"err_free_msg" so that is clear is an error path.
>+ ret = dpll_device_get_one(dpll, msg, NULL);
>+ if (ret)
>+ goto out_cancel_msg;
Same here, "err_cancel_msg"
>+ genlmsg_end(msg, hdr);
>+ genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
>+
>+ return 0;
>+
>+out_cancel_msg:
>+ genlmsg_cancel(msg, hdr);
>+out_free_msg:
>+ nlmsg_free(msg);
>+
>+ return ret;
>+}
>+
>+int dpll_device_create_ntf(struct dpll_device *dpll)
>+{
>+ return dpll_device_event_send(DPLL_CMD_DEVICE_CREATE_NTF, dpll);
>+}
>+
>+int dpll_device_delete_ntf(struct dpll_device *dpll)
>+{
>+ return dpll_device_event_send(DPLL_CMD_DEVICE_DELETE_NTF, dpll);
>+}
>+
This is an exported function, documentation commentary perhaps?
I mean, you sometimes have it for static functions, here you don't. Very
odd.
Let's have that for all exported functions please.
>+int dpll_device_change_ntf(struct dpll_device *dpll)
>+{
>+ int ret = -EINVAL;
>+
>+ if (WARN_ON(!dpll))
>+ return ret;
Rely on basic driver sanity and drop this check. don't forget to remove
the ret initialization.
>+
>+ mutex_lock(&dpll_lock);
>+ ret = dpll_device_event_send(DPLL_CMD_DEVICE_CHANGE_NTF, dpll);
>+ mutex_unlock(&dpll_lock);
>+
>+ return ret;
>+}
>+EXPORT_SYMBOL_GPL(dpll_device_change_ntf);
>+
>+static int
>+dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin)
>+{
>+ struct dpll_pin *pin_verify;
>+ struct sk_buff *msg;
>+ int ret = -EMSGSIZE;
Drop the pointless init.
>+ void *hdr;
>+
>+ pin_verify = xa_load(&dpll_pin_xa, pin->id);
>+ if (pin != pin_verify)
I don't follow. What is the purpose for this check? Once you have
REGISTERED mark for pin, you can check it here and be consistent with
dpll_device_event_send()
>+ 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 out_free_msg;
"err_free_msg" so that is clear is an error path.
>+ ret = __dpll_cmd_pin_dump_one(msg, pin, NULL);
>+ if (ret)
>+ goto out_cancel_msg;
Same here, "err_cancel_msg"
>+ genlmsg_end(msg, hdr);
>+ genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
>+
>+ return 0;
>+
>+out_cancel_msg:
>+ genlmsg_cancel(msg, hdr);
>+out_free_msg:
>+ nlmsg_free(msg);
>+
>+ return ret;
>+}
>+
>+int dpll_pin_create_ntf(struct dpll_pin *pin)
>+{
>+ return dpll_pin_event_send(DPLL_CMD_PIN_CREATE_NTF, pin);
>+}
>+
>+int dpll_pin_delete_ntf(struct dpll_pin *pin)
>+{
>+ return dpll_pin_event_send(DPLL_CMD_PIN_DELETE_NTF, pin);
>+}
>+
>+static int __dpll_pin_change_ntf(struct dpll_pin *pin)
>+{
>+ return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);
>+}
>+
>+int dpll_pin_change_ntf(struct dpll_pin *pin)
>+{
>+ int ret = -EINVAL;
>+
>+ if (WARN_ON(!pin))
>+ return ret;
Remove this check and expect basic sanity from driver. Also, don't
forget to drop the "ret" initialization.
>+
>+ mutex_lock(&dpll_lock);
>+ ret = __dpll_pin_change_ntf(pin);
>+ mutex_unlock(&dpll_lock);
>+
>+ return ret;
>+}
>+EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>+
>+int __init dpll_netlink_init(void)
>+{
>+ return genl_register_family(&dpll_nl_family);
>+}
>+
>+void dpll_netlink_finish(void)
>+{
>+ genl_unregister_family(&dpll_nl_family);
>+}
>+
>+void __exit dpll_netlink_fini(void)
>+{
>+ dpll_netlink_finish();
>+}
>diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
>new file mode 100644
>index 000000000000..b5f9bfc88c9e
>--- /dev/null
>+++ b/drivers/dpll/dpll_netlink.h
>@@ -0,0 +1,44 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ * Copyright (c) 2023 Meta Platforms, Inc. and affiliates
>+ * Copyright (c) 2023 Intel and affiliates
>+ */
>+
>+/**
>+ * dpll_device_create_ntf - notify that the device has been created
>+ * @dpll: registered dpll pointer
>+ *
>+ * Context: caller shall hold dpll_xa_lock.
>+ * Return: 0 if succeeds, error code otherwise.
>+ */
>+int dpll_device_create_ntf(struct dpll_device *dpll);
>+
>+/**
>+ * dpll_device_delete_ntf - notify that the device has been deleted
>+ * @dpll: registered dpll pointer
>+ *
>+ * Context: caller shall hold dpll_xa_lock.
>+ * Return: 0 if succeeds, error code otherwise.
>+ */
Again, I'm going to repeat myself. Please have this kdoc comments once,
in the .c file. Header should not contain this.
>+int dpll_device_delete_ntf(struct dpll_device *dpll);
>+
>+/**
>+ * dpll_pin_create_ntf - notify that the pin has been created
>+ * @pin: registered pin pointer
>+ *
>+ * Context: caller shall hold dpll_xa_lock.
>+ * Return: 0 if succeeds, error code otherwise.
>+ */
>+int dpll_pin_create_ntf(struct dpll_pin *pin);
>+
>+/**
>+ * dpll_pin_delete_ntf - notify that the pin has been deleted
>+ * @pin: registered pin pointer
>+ *
>+ * Context: caller shall hold dpll_xa_lock.
>+ * Return: 0 if succeeds, error code otherwise.
>+ */
>+int dpll_pin_delete_ntf(struct dpll_pin *pin);
>+
>+int __init dpll_netlink_init(void);
>+void dpll_netlink_finish(void);
>--
>2.37.3
>