[RFC,v8,04/10] dpll: netlink: Add DPLL framework base functions

Commit Message

Arkadiusz Kubalewski June 9, 2023, 12:18 p.m. UTC

From: Vadim Fedorenko <[e-mail]>

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 <[e-mail]>
Signed-off-by: Milena Olech <[e-mail]>
Co-developed-by: Michal Michalik <[e-mail]>
Signed-off-by: Michal Michalik <[e-mail]>
Signed-off-by: Vadim Fedorenko <[e-mail]>
Co-developed-by: Arkadiusz Kubalewski <[e-mail]>
Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
---
 drivers/dpll/dpll_netlink.c | 1183 +++++++++++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.h |   44 ++
 2 files changed, 1227 insertions(+)
 create mode 100644 drivers/dpll/dpll_netlink.c
 create mode 100644 drivers/dpll/dpll_netlink.h

Comments

Jiri Pirko June 11, 2023, 11:42 a.m. UTC | #1

Fri, Jun 09, 2023 at 02:18:47PM CEST, [e-mail] wrote:
>From: Vadim Fedorenko <[e-mail]>

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 <[e-mail]>
>Signed-off-by: Milena Olech <[e-mail]>
>Co-developed-by: Michal Michalik <[e-mail]>
>Signed-off-by: Michal Michalik <[e-mail]>
>Signed-off-by: Vadim Fedorenko <[e-mail]>
>Co-developed-by: Arkadiusz Kubalewski <[e-mail]>
>Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
>---
> 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
>

Petr Oros June 21, 2023, 11:18 a.m. UTC | #2

Arkadiusz Kubalewski píše v Pá 09. 06. 2023 v 14:18 +0200:
> From: Vadim Fedorenko <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Milena Olech <[e-mail]>
> Co-developed-by: Michal Michalik <[e-mail]>
> Signed-off-by: Michal Michalik <[e-mail]>
> Signed-off-by: Vadim Fedorenko <[e-mail]>
> Co-developed-by: Arkadiusz Kubalewski
> <[e-mail]>
> Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
> ---
>  drivers/dpll/dpll_netlink.c | 1183
> +++++++++++++++++++++++++++++++++++
>  drivers/dpll/dpll_netlink.h |   44 ++
>  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);
> +
> +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)
> +{
> +       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;
> +       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;
> +       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;
> +       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;
> +       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;
> +       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;
> +       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;
> +       if (ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
> dpll,
> +                              dpll_priv(dpll), &freq, extack))
> +               return -EFAULT;
> +       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))
> +                       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;
> +               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,
> +                         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)
> +{
> +       struct dpll_pin_ref *ref;
> +       int ret;
> +
> +       ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
> +       if (!ref)
> +               return -EFAULT;
> +       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)) {
> +               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 &&
> +                   freq <=  pin->prop->freq_supported[fs].max)
> +                       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))
> +               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;
> +               __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,
> +                                                               paren
> t),
> +                                         state, extack))
> +                       return -EFAULT;
> +       }
> +       __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;
> +       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;
> +       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;
> +       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;
> +       __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))
> +                       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)
> +                               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;
> +                       clock_id = nla_get_u64(attr);
> +                       break;
> +               case DPLL_A_MODULE_NAME:
> +                       if (mod_name_attr)
> +                               return -EINVAL;
> +                       mod_name_attr = attr;
> +                       break;
> +               case DPLL_A_PIN_TYPE:
> +                       if (type)
> +                               return -EINVAL;
> +                       type = nla_get_u8(attr);
> +                       break;
> +               case DPLL_A_PIN_BOARD_LABEL:
> +                       if (board_label_attr)
> +                               return -EINVAL;
> +                       board_label_attr = attr;
> +                       break;
> +               case DPLL_A_PIN_PANEL_LABEL:
> +                       if (panel_label_attr)
> +                               return -EINVAL;
> +                       panel_label_attr = attr;
> +                       break;
> +               case DPLL_A_PIN_PACKAGE_LABEL:
> +                       if (package_label_attr)
> +                               return -EINVAL;
> +                       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);
> +       if (!pin)
> +               return -EINVAL;
> +       if (nla_put_u32(skb, DPLL_A_PIN_ID, pin->id))
> +               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);
> +
> +       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))
> +                       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)
> +                               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;
> +                       clock_id = nla_get_u64(attr);
> +                       break;
> +               case DPLL_A_MODULE_NAME:
> +                       if (mod_name_attr)
> +                               return -EINVAL;
> +                       mod_name_attr = attr;
> +                       break;
> +               case DPLL_A_TYPE:
> +                       if (type)
> +                               return -EINVAL;
> +                       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);
> +       if (!dpll)
> +               return -EINVAL;
> +
> +       return dpll_msg_add_dev_handle(skb, dpll);
> +}
> +
> +static int
> +dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info
> *info)
> +{
> +       const struct dpll_device_ops *ops = dpll_device_ops(dpll);
> +       struct nlattr *tb[DPLL_A_MAX + 1];
> +       int ret = 0;
> +
> +       nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
> +                 genlmsg_len(info->genlhdr), NULL, info->extack);
> +       if (tb[DPLL_A_MODE]) {
Hi,

Here should be something like:
               if (!ops->mode_set)
                       return -EOPNOTSUPP;

Regards,
Petr
> +               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))
> +                       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]) {
> +               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]) {
> +               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;
> +       void *hdr;
> +
> +       if (!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 out_free_msg;
> +       ret = dpll_device_get_one(dpll, msg, NULL);
> +       if (ret)
> +               goto out_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);
> +}
> +
> +int dpll_device_change_ntf(struct dpll_device *dpll)
> +{
> +       int ret = -EINVAL;
> +
> +       if (WARN_ON(!dpll))
> +               return ret;
> +
> +       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;
> +       void *hdr;
> +
> +       pin_verify = xa_load(&dpll_pin_xa, pin->id);
> +       if (pin != pin_verify)
> +               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;
> +       ret = __dpll_cmd_pin_dump_one(msg, pin, NULL);
> +       if (ret)
> +               goto out_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;
> +
> +       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.
> + */
> +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);

Jiri Pirko June 21, 2023, 11:53 a.m. UTC | #3

Wed, Jun 21, 2023 at 01:18:59PM CEST, [e-mail] wrote:
>Arkadiusz Kubalewski píše v Pá 09. 06. 2023 v 14:18 +0200:
>> From: Vadim Fedorenko <[e-mail]>

[...]

Could you perhaps cut out the text you don't comment? Saves some time
finding your reply.


>> +static int
>> +dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info
>> *info)
>> +{
>> +       const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>> +       struct nlattr *tb[DPLL_A_MAX + 1];
>> +       int ret = 0;
>> +
>> +       nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
>> +                 genlmsg_len(info->genlhdr), NULL, info->extack);
>> +       if (tb[DPLL_A_MODE]) {
>Hi,
>
>Here should be something like:
>               if (!ops->mode_set)
>                       return -EOPNOTSUPP;

Why? All drivers implement that.
I believe that it's actullaly better that way. For a called setting up
the same mode it is the dpll in, there should be 0 return by the driver.
Note that driver holds this value. I'd like to keep this code as it is.

[...]

Jiri Pirko June 21, 2023, 1:07 p.m. UTC | #4

Wed, Jun 21, 2023 at 01:53:24PM CEST, [e-mail] wrote:
>Wed, Jun 21, 2023 at 01:18:59PM CEST, [e-mail] wrote:
>>Arkadiusz Kubalewski píše v Pá 09. 06. 2023 v 14:18 +0200:
>>> From: Vadim Fedorenko <[e-mail]>
>
>[...]
>
>Could you perhaps cut out the text you don't comment? Saves some time
>finding your reply.
>
>
>>> +static int
>>> +dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info
>>> *info)
>>> +{
>>> +       const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>> +       struct nlattr *tb[DPLL_A_MAX + 1];
>>> +       int ret = 0;
>>> +
>>> +       nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
>>> +                 genlmsg_len(info->genlhdr), NULL, info->extack);
>>> +       if (tb[DPLL_A_MODE]) {
>>Hi,
>>
>>Here should be something like:
>>               if (!ops->mode_set)
>>                       return -EOPNOTSUPP;
>
>Why? All drivers implement that.
>I believe that it's actullaly better that way. For a called setting up
>the same mode it is the dpll in, there should be 0 return by the driver.
>Note that driver holds this value. I'd like to keep this code as it is.

Actually, you are correct Petr, my mistake. Actually, no driver
implements this. Arkadiusz, could you please remove this op and
possibly any other unused  op? It will be added when needed.

Thanks!


>
>[...]

Arkadiusz Kubalewski June 23, 2023, 12:56 a.m. UTC | #5

>From: Jiri Pirko <[e-mail]>
>Sent: Wednesday, June 21, 2023 3:08 PM
>
>Wed, Jun 21, 2023 at 01:53:24PM CEST, [e-mail] wrote:
>>Wed, Jun 21, 2023 at 01:18:59PM CEST, [e-mail] wrote:
>>>Arkadiusz Kubalewski píše v Pá 09. 06. 2023 v 14:18 +0200:
>>>> From: Vadim Fedorenko <[e-mail]>
>>
>>[...]
>>
>>Could you perhaps cut out the text you don't comment? Saves some time
>>finding your reply.
>>
>>
>>>> +static int
>>>> +dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info
>>>> *info)
>>>> +{
>>>> +       const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>> +       struct nlattr *tb[DPLL_A_MAX + 1];
>>>> +       int ret = 0;
>>>> +
>>>> +       nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
>>>> +                 genlmsg_len(info->genlhdr), NULL, info->extack);
>>>> +       if (tb[DPLL_A_MODE]) {
>>>Hi,
>>>
>>>Here should be something like:
>>>               if (!ops->mode_set)
>>>                       return -EOPNOTSUPP;
>>
>>Why? All drivers implement that.
>>I believe that it's actullaly better that way. For a called setting up
>>the same mode it is the dpll in, there should be 0 return by the driver.
>>Note that driver holds this value. I'd like to keep this code as it is.
>
>Actually, you are correct Petr, my mistake. Actually, no driver
>implements this. Arkadiusz, could you please remove this op and
>possibly any other unused  op? It will be added when needed.
>
>Thanks!
>

Sorry, didn't have time for such change, added only check as suggested by
Petr.
If you think this is a big issue, we could change it for next version.

Thank you!
Arkadiusz

>
>>
>>[...]

Arkadiusz Kubalewski June 23, 2023, 12:56 a.m. UTC | #6

>From: Petr Oros <[e-mail]>
>Sent: Wednesday, June 21, 2023 1:19 PM

[...]

>> +
>> +static int
>> +dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info
>> *info)
>> +{
>> +       const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>> +       struct nlattr *tb[DPLL_A_MAX + 1];
>> +       int ret = 0;
>> +
>> +       nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
>> +                 genlmsg_len(info->genlhdr), NULL, info->extack);
>> +       if (tb[DPLL_A_MODE]) {
>Hi,
>
>Here should be something like:
>               if (!ops->mode_set)
>                       return -EOPNOTSUPP;
>
>Regards,
>Petr

Sure, fixed.

Thank you!
Arkadiusz

[...]

Arkadiusz Kubalewski June 23, 2023, 1:01 a.m. UTC | #7

>From: Jiri Pirko <[e-mail]>
>Sent: Sunday, June 11, 2023 1:42 PM
>
>Fri, Jun 09, 2023 at 02:18:47PM CEST, [e-mail] wrote:
>>From: Vadim Fedorenko <[e-mail]>
>
>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?
>

Well, Vadim started it, then we did changes. I am just fine with
"Co-developed".

>
>>
>>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 <[e-mail]>
>>Signed-off-by: Milena Olech <[e-mail]>
>>Co-developed-by: Michal Michalik <[e-mail]>
>>Signed-off-by: Michal Michalik <[e-mail]>
>>Signed-off-by: Vadim Fedorenko <[e-mail]>
>>Co-developed-by: Arkadiusz Kubalewski <[e-mail]>
>>Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
>>---
>> 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!
>

Thanks for your review! :)

>
>> 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?
>

Fixed.

>
>>+
>>+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".
>

Will do.

>
>>+{
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+			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.
>

Fixed.

>
>>+		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?
>

Fixed.

>
>>+			  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.
>

Fixed.

>
>>+{
>>+	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.
>

To me, it feels pretty strange to look for it and then use it without
validating, as the function designed to return NULL.
Will try to fix it and make it consistent.

>
>>+	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.
>

Fixed.

>
>>+		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    ^^
>

Fixed.

>
>>+		    freq <=  pin->prop->freq_supported[fs].max)
>
>Avoid double space here    ^^
>

Fixed.

>
>>+			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?
>

Fixed.

>
>>+		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"
>

Fixed.

>
>>+		__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.
>

Fixed.

>
>>+	}
>>+	__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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	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 :)
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+	__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.
>

Yes it does. Will do.

>
>>+			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.
>

Fixed.

>
>>+				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
>

Fixed.

>
>>+			clock_id = nla_get_u64(attr);
>>+			break;
>>+		case DPLL_A_MODULE_NAME:
>>+			if (mod_name_attr)
>>+				return -EINVAL;
>
>Extack

Fixed.

>
>
>>+			mod_name_attr = attr;
>>+			break;
>>+		case DPLL_A_PIN_TYPE:
>>+			if (type)
>>+				return -EINVAL;
>
>Extack
>

Fixed.

>
>>+			type = nla_get_u8(attr);
>>+			break;
>>+		case DPLL_A_PIN_BOARD_LABEL:
>>+			if (board_label_attr)
>>+				return -EINVAL;
>
>Extack
>

Fixed.

>
>>+			board_label_attr = attr;
>>+			break;
>>+		case DPLL_A_PIN_PANEL_LABEL:
>>+			if (panel_label_attr)
>>+				return -EINVAL;
>
>Extack
>

Fixed.

>
>>+			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.
>

Fixed.

>
>>+			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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+		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
>

Wasn't that multiple matches of a pins with the same clock_id?
If you had multiple matches it seems it was fine.

Now, with the changes to find behavior this won't be an error but the reply
with extack, please try on new patchset.

>
>
>>+
>>+	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.
>

As said below, we need new macro for it.

>
>>+			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.
>

Fixed.

>
>>+				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
>

Fixed.

>
>>+			clock_id = nla_get_u64(attr);
>>+			break;
>>+		case DPLL_A_MODULE_NAME:
>>+			if (mod_name_attr)
>>+				return -EINVAL;
>
>Extack
>

Fixed.

>
>>+			mod_name_attr = attr;
>>+			break;
>>+		case DPLL_A_TYPE:
>>+			if (type)
>>+				return -EINVAL;
>
>Extack
>
>You can use goto with one "duplicate attribute" message.
>

Fixed.

>
>>+			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.
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+}
>>+
>>+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.
>

Fixed.

>
>>+{
>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>+	struct nlattr *tb[DPLL_A_MAX + 1];
>>+	int ret = 0;
>
>Drop pointless init.
>

Fixed.

>
>>+
>>+	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?
>

Sure, can add some helper macro here, altough we probably need to add it to
xarray lib.

>
>>+			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.
>

Fixed.

>
>>+		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.
>

Fixed.

>
>>+		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.
>

Fixed.

>
>>+	void *hdr;
>>+
>>+	if (!xa_get_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED))
>
>WARN_ON? The driver is buggy when he calls this.
>

Fixed.

>
>>+		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.
>

Fixed.

>
>>+	ret = dpll_device_get_one(dpll, msg, NULL);
>>+	if (ret)
>>+		goto out_cancel_msg;
>
>Same here, "err_cancel_msg"
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+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.
>

Fixed.

>
>>+
>>+	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.
>

Fixed.

>
>>+	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()
>

Fixed.

>
>>+		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.
>

Fixed.

>
>>+	ret = __dpll_cmd_pin_dump_one(msg, pin, NULL);
>>+	if (ret)
>>+		goto out_cancel_msg;
>
>Same here, "err_cancel_msg"
>

Fixed.

>
>>+	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.
>

Fixed.

>
>>+
>>+	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.
>

Fixed.

Thank you!
Arkadiusz

>
>
>>+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
>>

Jiri Pirko June 23, 2023, 7:48 a.m. UTC | #8

Fri, Jun 23, 2023 at 02:56:24AM CEST, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Wednesday, June 21, 2023 3:08 PM
>>
>>Wed, Jun 21, 2023 at 01:53:24PM CEST, [e-mail] wrote:
>>>Wed, Jun 21, 2023 at 01:18:59PM CEST, [e-mail] wrote:
>>>>Arkadiusz Kubalewski píše v Pá 09. 06. 2023 v 14:18 +0200:
>>>>> From: Vadim Fedorenko <[e-mail]>
>>>
>>>[...]
>>>
>>>Could you perhaps cut out the text you don't comment? Saves some time
>>>finding your reply.
>>>
>>>
>>>>> +static int
>>>>> +dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info
>>>>> *info)
>>>>> +{
>>>>> +       const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>>> +       struct nlattr *tb[DPLL_A_MAX + 1];
>>>>> +       int ret = 0;
>>>>> +
>>>>> +       nla_parse(tb, DPLL_A_MAX, genlmsg_data(info->genlhdr),
>>>>> +                 genlmsg_len(info->genlhdr), NULL, info->extack);
>>>>> +       if (tb[DPLL_A_MODE]) {
>>>>Hi,
>>>>
>>>>Here should be something like:
>>>>               if (!ops->mode_set)
>>>>                       return -EOPNOTSUPP;
>>>
>>>Why? All drivers implement that.
>>>I believe that it's actullaly better that way. For a called setting up
>>>the same mode it is the dpll in, there should be 0 return by the driver.
>>>Note that driver holds this value. I'd like to keep this code as it is.
>>
>>Actually, you are correct Petr, my mistake. Actually, no driver
>>implements this. Arkadiusz, could you please remove this op and
>>possibly any other unused  op? It will be added when needed.
>>
>>Thanks!
>>
>
>Sorry, didn't have time for such change, added only check as suggested by
>Petr.
>If you think this is a big issue, we could change it for next version.

It's odd to carry on ops which are unused. I would prefer that to be
removed now and only introduced when they are actually needed.


>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>[...]

Patch

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);
+
+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)
+{
+	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;
+	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;
+	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;
+	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;
+	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;
+	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;
+	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;
+	if (ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
+			       dpll_priv(dpll), &freq, extack))
+		return -EFAULT;
+	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))
+			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;
+		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,
+			  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)
+{
+	struct dpll_pin_ref *ref;
+	int ret;
+
+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
+	if (!ref)
+		return -EFAULT;
+	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)) {
+		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 &&
+		    freq <=  pin->prop->freq_supported[fs].max)
+			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))
+		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;
+		__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;
+	}
+	__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;
+	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;
+	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;
+	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;
+	__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))
+			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)
+				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;
+			clock_id = nla_get_u64(attr);
+			break;
+		case DPLL_A_MODULE_NAME:
+			if (mod_name_attr)
+				return -EINVAL;
+			mod_name_attr = attr;
+			break;
+		case DPLL_A_PIN_TYPE:
+			if (type)
+				return -EINVAL;
+			type = nla_get_u8(attr);
+			break;
+		case DPLL_A_PIN_BOARD_LABEL:
+			if (board_label_attr)
+				return -EINVAL;
+			board_label_attr = attr;
+			break;
+		case DPLL_A_PIN_PANEL_LABEL:
+			if (panel_label_attr)
+				return -EINVAL;
+			panel_label_attr = attr;
+			break;
+		case DPLL_A_PIN_PACKAGE_LABEL:
+			if (package_label_attr)
+				return -EINVAL;
+			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);
+	if (!pin)
+		return -EINVAL;
+	if (nla_put_u32(skb, DPLL_A_PIN_ID, pin->id))
+		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);
+
+	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))
+			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)
+				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;
+			clock_id = nla_get_u64(attr);
+			break;
+		case DPLL_A_MODULE_NAME:
+			if (mod_name_attr)
+				return -EINVAL;
+			mod_name_attr = attr;
+			break;
+		case DPLL_A_TYPE:
+			if (type)
+				return -EINVAL;
+			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);
+	if (!dpll)
+		return -EINVAL;
+
+	return dpll_msg_add_dev_handle(skb, dpll);
+}
+
+static int
+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
+{
+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
+	struct nlattr *tb[DPLL_A_MAX + 1];
+	int ret = 0;
+
+	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))
+			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]) {
+		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]) {
+		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;
+	void *hdr;
+
+	if (!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 out_free_msg;
+	ret = dpll_device_get_one(dpll, msg, NULL);
+	if (ret)
+		goto out_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);
+}
+
+int dpll_device_change_ntf(struct dpll_device *dpll)
+{
+	int ret = -EINVAL;
+
+	if (WARN_ON(!dpll))
+		return ret;
+
+	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;
+	void *hdr;
+
+	pin_verify = xa_load(&dpll_pin_xa, pin->id);
+	if (pin != pin_verify)
+		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;
+	ret = __dpll_cmd_pin_dump_one(msg, pin, NULL);
+	if (ret)
+		goto out_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;
+
+	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.
+ */
+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);