Date:   Fri, 2 Dec 2022 11:27:35 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>,
        Vadim Fedorenko <vfedorenko@...ek.ru>
CC:     Jakub Kicinski <kuba@...nel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Vadim Fedorenko <vadfed@...com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "Olech, Milena" <milena.olech@...el.com>,
        "Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [RFC PATCH v4 2/4] dpll: Add DPLL framework base functions

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Wednesday, November 30, 2022 5:37 PM
>Tue, Nov 29, 2022 at 10:37:22PM CET, vfedorenko@...ek.ru wrote:
>>From: Vadim Fedorenko <vadfed@...com>
>
>[...]
>
>>+
>>+static const struct nla_policy dpll_cmd_device_get_policy[] = {
>>+	[DPLLA_ID]		= { .type = NLA_U32 },
>>+	[DPLLA_NAME]		= { .type = NLA_STRING,
>>+				    .len = DPLL_NAME_LEN },
>>+	[DPLLA_DUMP_FILTER]	= { .type = NLA_U32 },
>>+	[DPLLA_NETIFINDEX]	= { .type = NLA_U32 },
>
>Only pin has a netdevice not the dpll. Also does not make sense to allow
>as an input attr.

Yes, this part shall be removed.

>
>
>>+};
>>+
>>+static const struct nla_policy dpll_cmd_device_set_policy[] = {
>>+	[DPLLA_ID]		= { .type = NLA_U32 },
>>+	[DPLLA_NAME]		= { .type = NLA_STRING,
>>+				    .len = DPLL_NAME_LEN },
>>+	[DPLLA_MODE]		= { .type = NLA_U32 },
>>+	[DPLLA_SOURCE_PIN_IDX]	= { .type = NLA_U32 },
>>+};
>>+
>>+static const struct nla_policy dpll_cmd_pin_set_policy[] = {
>>+	[DPLLA_ID]		= { .type = NLA_U32 },
>>+	[DPLLA_PIN_IDX]		= { .type = NLA_U32 },
>>+	[DPLLA_PIN_TYPE]	= { .type = NLA_U32 },
>>+	[DPLLA_PIN_SIGNAL_TYPE]	= { .type = NLA_U32 },
>>+	[DPLLA_PIN_CUSTOM_FREQ] = { .type = NLA_U32 },
>>+	[DPLLA_PIN_STATE]	= { .type = NLA_U32 },
>>+	[DPLLA_PIN_PRIO]	= { .type = NLA_U32 },
>>+};
>>+
>>+struct dpll_param {
>>+	struct netlink_callback *cb;
>>+	struct sk_buff *msg;
>>+	struct dpll_device *dpll;
>>+	struct dpll_pin *pin;
>>+	enum dpll_event_change change_type;
>>+};
>>+
>>+struct dpll_dump_ctx {
>>+	int dump_filter;
>>+};
>>+
>>+typedef int (*cb_t)(struct dpll_param *);
>>+
>>+static struct genl_family dpll_gnl_family;
>>+
>>+static struct dpll_dump_ctx *dpll_dump_context(struct netlink_callback
>*cb)
>>+{
>>+	return (struct dpll_dump_ctx *)cb->ctx;
>>+}
>>+
>>+static int dpll_msg_add_id(struct sk_buff *msg, u32 id)
>>+{
>>+	if (nla_put_u32(msg, DPLLA_ID, id))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_name(struct sk_buff *msg, const char *name)
>>+{
>>+	if (nla_put_string(msg, DPLLA_NAME, name))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int __dpll_msg_add_mode(struct sk_buff *msg, enum dplla msg_type,
>>+			       enum dpll_mode mode)
>>+{
>>+	if (nla_put_s32(msg, msg_type, mode))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_mode(struct sk_buff *msg, const struct dpll_attr
>*attr)
>>+{
>>+	enum dpll_mode m = dpll_attr_mode_get(attr);
>>+
>>+	if (m == DPLL_MODE_UNSPEC)
>>+		return 0;
>>+
>>+	return __dpll_msg_add_mode(msg, DPLLA_MODE, m);
>>+}
>>+
>>+static int dpll_msg_add_modes_supported(struct sk_buff *msg,
>>+					const struct dpll_attr *attr)
>>+{
>>+	enum dpll_mode i;
>>+	int  ret = 0;
>>+
>>+	for (i = DPLL_MODE_UNSPEC + 1; i <= DPLL_MODE_MAX; i++) {
>>+		if (dpll_attr_mode_supported(attr, i)) {
>>+			ret = __dpll_msg_add_mode(msg, DPLLA_MODE_SUPPORTED, i);
>>+			if (ret)
>>+				return -EMSGSIZE;
>>+		}
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static int dpll_msg_add_source_pin(struct sk_buff *msg, struct dpll_attr
>*attr)
>>+{
>>+	u32 source_idx;
>>+
>>+	if (dpll_attr_source_idx_get(attr, &source_idx))
>>+		return 0;
>>+	if (nla_put_u32(msg, DPLLA_SOURCE_PIN_IDX, source_idx))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_netifindex(struct sk_buff *msg, struct dpll_attr
>*attr)
>>+{
>>+	unsigned int netifindex; // TODO: Should be u32?
>>+
>>+	if (dpll_attr_netifindex_get(attr, &netifindex))
>>+		return 0;
>>+	if (nla_put_u32(msg, DPLLA_NETIFINDEX, netifindex))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_attr
>*attr)
>>+{
>>+	enum dpll_lock_status s = dpll_attr_lock_status_get(attr);
>>+
>>+	if (s == DPLL_LOCK_STATUS_UNSPEC)
>>+		return 0;
>>+	if (nla_put_s32(msg, DPLLA_LOCK_STATUS, s))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_temp(struct sk_buff *msg, struct dpll_attr *attr)
>>+{
>>+	s32 temp;
>>+
>>+	if (dpll_attr_temp_get(attr, &temp))
>>+		return 0;
>>+	if (nla_put_u32(msg, DPLLA_TEMP, temp))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_idx(struct sk_buff *msg, u32 pin_idx)
>>+{
>>+	if (nla_put_u32(msg, DPLLA_PIN_IDX, pin_idx))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_description(struct sk_buff *msg,
>>+					const char *description)
>>+{
>>+	if (nla_put_string(msg, DPLLA_PIN_DESCRIPTION, description))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_parent_idx(struct sk_buff *msg, u32
>parent_idx)
>>+{
>>+	if (nla_put_u32(msg, DPLLA_PIN_PARENT_IDX, parent_idx))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int __dpll_msg_add_pin_type(struct sk_buff *msg, enum dplla attr,
>>+				   enum dpll_pin_type type)
>>+{
>>+	if (nla_put_s32(msg, attr, type))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+dpll_msg_add_pin_type(struct sk_buff *msg, const struct dpll_pin_attr
>*attr)
>>+{
>>+	enum dpll_pin_type t = dpll_pin_attr_type_get(attr);
>>+
>>+	if (t == DPLL_PIN_TYPE_UNSPEC)
>>+		return 0;
>>+
>>+	return __dpll_msg_add_pin_type(msg, DPLLA_PIN_TYPE, t);
>>+}
>>+
>>+static int dpll_msg_add_pin_types_supported(struct sk_buff *msg,
>>+					    const struct dpll_pin_attr *attr)
>>+{
>>+	enum dpll_pin_type i;
>>+	int ret;
>>+
>>+	for (i = DPLL_PIN_TYPE_UNSPEC + 1; i <= DPLL_PIN_TYPE_MAX; i++) {
>>+		if (dpll_pin_attr_type_supported(attr, i)) {
>>+			ret = __dpll_msg_add_pin_type(msg,
>>+						      DPLLA_PIN_TYPE_SUPPORTED,
>>+						      i);
>>+			if (ret)
>>+				return ret;
>>+		}
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int __dpll_msg_add_pin_signal_type(struct sk_buff *msg,
>>+					  enum dplla attr,
>>+					  enum dpll_pin_signal_type type)
>>+{
>>+	if (nla_put_s32(msg, attr, type))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_signal_type(struct sk_buff *msg,
>>+					const struct dpll_pin_attr *attr)
>>+{
>>+	enum dpll_pin_signal_type t = dpll_pin_attr_signal_type_get(attr);
>>+
>>+	if (t == DPLL_PIN_SIGNAL_TYPE_UNSPEC)
>>+		return 0;
>>+
>>+	return __dpll_msg_add_pin_signal_type(msg, DPLLA_PIN_SIGNAL_TYPE, t);
>>+}
>>+
>>+static int
>>+dpll_msg_add_pin_signal_types_supported(struct sk_buff *msg,
>>+					const struct dpll_pin_attr *attr)
>>+{
>>+	const enum dplla da = DPLLA_PIN_SIGNAL_TYPE_SUPPORTED;
>>+	enum dpll_pin_signal_type i;
>>+	int ret;
>>+
>>+	for (i = DPLL_PIN_SIGNAL_TYPE_UNSPEC + 1;
>>+	     i <= DPLL_PIN_SIGNAL_TYPE_MAX; i++) {
>>+		if (dpll_pin_attr_signal_type_supported(attr, i)) {
>>+			ret = __dpll_msg_add_pin_signal_type(msg, da, i);
>>+			if (ret)
>>+				return ret;
>>+		}
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_custom_freq(struct sk_buff *msg,
>>+					const struct dpll_pin_attr *attr)
>>+{
>>+	u32 freq;
>>+
>>+	if (dpll_pin_attr_custom_freq_get(attr, &freq))
>>+		return 0;
>>+	if (nla_put_u32(msg, DPLLA_PIN_CUSTOM_FREQ, freq))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_states(struct sk_buff *msg,
>>+				   const struct dpll_pin_attr *attr)
>>+{
>>+	enum dpll_pin_state i;
>>+
>>+	for (i = DPLL_PIN_STATE_UNSPEC + 1; i <= DPLL_PIN_STATE_MAX; i++)
>>+		if (dpll_pin_attr_state_enabled(attr, i))
>>+			if (nla_put_s32(msg, DPLLA_PIN_STATE, i))
>>+				return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_msg_add_pin_states_supported(struct sk_buff *msg,
>>+					     const struct dpll_pin_attr *attr)
>>+{
>>+	enum dpll_pin_state i;
>>+
>>+	for (i = DPLL_PIN_STATE_UNSPEC + 1; i <= DPLL_PIN_STATE_MAX; i++)
>>+		if (dpll_pin_attr_state_supported(attr, i))
>>+			if (nla_put_s32(msg, DPLLA_PIN_STATE_SUPPORTED, i))
>>+				return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+dpll_msg_add_pin_prio(struct sk_buff *msg, const struct dpll_pin_attr
>*attr)
>>+{
>>+	u32 prio;
>>+
>>+	if (dpll_pin_attr_prio_get(attr, &prio))
>>+		return 0;
>>+	if (nla_put_u32(msg, DPLLA_PIN_PRIO, prio))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+dpll_msg_add_pin_netifindex(struct sk_buff *msg, const struct
>dpll_pin_attr *attr)
>>+{
>>+	unsigned int netifindex; // TODO: Should be u32?
>>+
>>+	if (dpll_pin_attr_netifindex_get(attr, &netifindex))
>>+		return 0;
>>+	if (nla_put_u32(msg, DPLLA_PIN_NETIFINDEX, netifindex))
>
>I was thinking about this. It is problematic. DPLL has no notion of
>network namespaces. So if the driver passes ifindex, dpll/user has no
>clue in which network namespace it is (ifindexes ovelay in multiple
>namespaces).
>
>There is no easy/nice solution. For now, I would go without this and
>only have linkage the opposite direction, from netdev to dpll.

Well, makes sense to me.
Although as I have checked `ip a` showed the same ifindex either if port was
in the namespace or not.
Isn't it better to let the user know ifindex, even if he has to iterate all
the namespaces he has created?

>
>
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+dpll_msg_add_event_change_type(struct sk_buff *msg,
>>+			       enum dpll_event_change event)
>>+{
>>+	if (nla_put_s32(msg, DPLLA_CHANGE_TYPE, event))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+__dpll_cmd_device_dump_one(struct sk_buff *msg, struct dpll_device *dpll)
>>+{
>>+	int ret = dpll_msg_add_id(msg, dpll_id(dpll));
>>+
>>+	if (ret)
>>+		return ret;
>>+	ret = dpll_msg_add_name(msg, dpll_dev_name(dpll));
>>+
>>+	return ret;
>>+}
>>+
>>+static int
>>+__dpll_cmd_pin_dump_one(struct sk_buff *msg, struct dpll_device *dpll,
>>+			struct dpll_pin *pin)
>>+{
>>+	struct dpll_pin_attr *attr = dpll_pin_attr_alloc();
>>+	struct dpll_pin *parent = NULL;
>>+	int ret;
>>+
>>+	if (!attr)
>>+		return -ENOMEM;
>>+	ret = dpll_msg_add_pin_idx(msg, dpll_pin_idx(dpll, pin));
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_description(msg,
>dpll_pin_get_description(pin));
>>+	if (ret)
>>+		goto out;
>>+	parent = dpll_pin_get_parent(pin);
>>+	if (parent) {
>>+		ret = dpll_msg_add_pin_parent_idx(msg, dpll_pin_idx(dpll,
>>+								    parent));
>>+		if (ret)
>>+			goto out;
>>+	}
>>+	ret = dpll_pin_get_attr(dpll, pin, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_type(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_types_supported(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_signal_type(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_signal_types_supported(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_custom_freq(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_states(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_states_supported(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_prio(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+	ret = dpll_msg_add_pin_netifindex(msg, attr);
>>+	if (ret)
>>+		goto out;
>>+out:
>>+	dpll_pin_attr_free(attr);
>>+
>>+	return ret;
>>+}
>>+
>>+static int __dpll_cmd_dump_pins(struct sk_buff *msg, struct dpll_device
>*dpll)
>>+{
>>+	struct dpll_pin *pin;
>>+	struct nlattr *attr;
>>+	unsigned long i;
>>+	int ret = 0;
>>+
>>+	for_each_pin_on_dpll(dpll, pin, i) {
>>+		attr = nla_nest_start(msg, DPLLA_PIN);
>>+		if (!attr) {
>>+			ret = -EMSGSIZE;
>>+			goto nest_cancel;
>>+		}
>>+		ret = __dpll_cmd_pin_dump_one(msg, dpll, pin);
>>+		if (ret)
>>+			goto nest_cancel;
>>+		nla_nest_end(msg, attr);
>>+	}
>>+
>>+	return ret;
>>+
>>+nest_cancel:
>>+	nla_nest_cancel(msg, attr);
>>+	return ret;
>>+}
>>+
>>+static int
>>+__dpll_cmd_dump_status(struct sk_buff *msg, struct dpll_device *dpll)
>>+{
>>+	struct dpll_attr *attr = dpll_attr_alloc();
>>+	int ret = dpll_get_attr(dpll, attr);
>>+
>>+	if (ret)
>>+		return -EAGAIN;
>>+	if (dpll_msg_add_source_pin(msg, attr))
>>+		return -EMSGSIZE;
>>+	if (dpll_msg_add_temp(msg, attr))
>>+		return -EMSGSIZE;
>>+	if (dpll_msg_add_lock_status(msg, attr))
>>+		return -EMSGSIZE;
>>+	if (dpll_msg_add_mode(msg, attr))
>>+		return -EMSGSIZE;
>>+	if (dpll_msg_add_modes_supported(msg, attr))
>>+		return -EMSGSIZE;
>>+	if (dpll_msg_add_netifindex(msg, attr))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+dpll_device_dump_one(struct dpll_device *dpll, struct sk_buff *msg,
>>+		     int dump_filter)
>>+{
>>+	int ret;
>>+
>>+	dpll_lock(dpll);
>>+	ret = __dpll_cmd_device_dump_one(msg, dpll);
>>+	if (ret)
>>+		goto out_unlock;
>>+
>>+	if (dump_filter & DPLL_DUMP_FILTER_STATUS) {
>>+		ret = __dpll_cmd_dump_status(msg, dpll);
>>+		if (ret)
>>+			goto out_unlock;
>>+	}
>>+	if (dump_filter & DPLL_DUMP_FILTER_PINS)
>>+		ret = __dpll_cmd_dump_pins(msg, dpll);
>>+	dpll_unlock(dpll);
>>+
>>+	return ret;
>>+out_unlock:
>>+	dpll_unlock(dpll);
>>+	return ret;
>>+}
>>+
>>+static enum dpll_pin_type dpll_msg_read_pin_type(struct nlattr *a)
>>+{
>>+	return nla_get_s32(a);
>>+}
>>+
>>+static enum dpll_pin_signal_type dpll_msg_read_pin_sig_type(struct nlattr
>*a)
>>+{
>>+	return nla_get_s32(a);
>>+}
>>+
>>+static u32 dpll_msg_read_pin_custom_freq(struct nlattr *a)
>>+{
>>+	return nla_get_u32(a);
>>+}
>>+
>>+static enum dpll_pin_state dpll_msg_read_pin_state(struct nlattr *a)
>>+{
>>+	return nla_get_s32(a);
>>+}
>>+
>>+static u32 dpll_msg_read_pin_prio(struct nlattr *a)
>>+{
>>+	return nla_get_u32(a);
>>+}
>>+
>>+static u32 dpll_msg_read_dump_filter(struct nlattr *a)
>>+{
>>+	return nla_get_u32(a);
>>+}
>>+
>>+static int
>>+dpll_pin_attr_from_nlattr(struct dpll_pin_attr *pa, struct genl_info
>*info)
>>+{
>>+	enum dpll_pin_signal_type st;
>>+	enum dpll_pin_state state;
>>+	enum dpll_pin_type t;
>>+	struct nlattr *a;
>>+	int rem, ret = 0;
>>+	u32 prio, freq;
>>+
>>+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>>+			  genlmsg_len(info->genlhdr), rem) {
>>+		switch (nla_type(a)) {
>>+		case DPLLA_PIN_TYPE:
>>+			t = dpll_msg_read_pin_type(a);
>>+			ret = dpll_pin_attr_type_set(pa, t);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		case DPLLA_PIN_SIGNAL_TYPE:
>>+			st = dpll_msg_read_pin_sig_type(a);
>>+			ret = dpll_pin_attr_signal_type_set(pa, st);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		case DPLLA_PIN_CUSTOM_FREQ:
>>+			freq = dpll_msg_read_pin_custom_freq(a);
>>+			ret = dpll_pin_attr_custom_freq_set(pa, freq);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		case DPLLA_PIN_STATE:
>>+			state = dpll_msg_read_pin_state(a);
>>+			ret = dpll_pin_attr_state_set(pa, state);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		case DPLLA_PIN_PRIO:
>>+			prio = dpll_msg_read_pin_prio(a);
>>+			ret = dpll_pin_attr_prio_set(pa, prio);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		default:
>>+			break;
>>+		}
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static int dpll_cmd_pin_set(struct sk_buff *skb, struct genl_info *info)
>>+{
>>+	struct dpll_pin_attr *old = NULL, *new = NULL, *delta = NULL;
>>+	struct dpll_device *dpll = info->user_ptr[0];
>>+	struct nlattr **attrs = info->attrs;
>>+	struct dpll_pin *pin;
>>+	int ret, pin_id;
>>+
>>+	if (!attrs[DPLLA_PIN_IDX])
>>+		return -EINVAL;
>>+	pin_id = nla_get_u32(attrs[DPLLA_PIN_IDX]);
>>+	old = dpll_pin_attr_alloc();
>>+	new = dpll_pin_attr_alloc();
>>+	delta = dpll_pin_attr_alloc();
>>+	if (!old || !new || !delta) {
>>+		ret = -ENOMEM;
>>+		goto mem_free;
>>+	}
>>+	dpll_lock(dpll);
>>+	pin = dpll_pin_get_by_idx(dpll, pin_id);
>>+	if (!pin) {
>>+		ret = -ENODEV;
>>+		goto mem_free_unlock;
>>+	}
>>+	ret = dpll_pin_get_attr(dpll, pin, old);
>>+	if (ret)
>>+		goto mem_free_unlock;
>>+	ret = dpll_pin_attr_from_nlattr(new, info);
>>+	if (ret)
>>+		goto mem_free_unlock;
>>+	ret = dpll_pin_attr_delta(delta, new, old);
>>+	dpll_unlock(dpll);
>>+	if (!ret)
>>+		ret = dpll_pin_set_attr(dpll, pin, delta);
>>+	else
>>+		ret = -EINVAL;
>>+
>>+	dpll_pin_attr_free(delta);
>>+	dpll_pin_attr_free(new);
>>+	dpll_pin_attr_free(old);
>>+
>>+	return ret;
>>+
>>+mem_free_unlock:
>>+	dpll_unlock(dpll);
>>+mem_free:
>>+	dpll_pin_attr_free(delta);
>>+	dpll_pin_attr_free(new);
>>+	dpll_pin_attr_free(old);
>>+	return ret;
>>+}
>>+
>>+enum dpll_mode dpll_msg_read_mode(struct nlattr *a)
>>+{
>>+	return nla_get_s32(a);
>>+}
>>+
>>+u32 dpll_msg_read_source_pin_id(struct nlattr *a)
>>+{
>>+	return nla_get_u32(a);
>>+}
>>+
>>+static int
>>+dpll_attr_from_nlattr(struct dpll_attr *dpll, struct genl_info *info)
>>+{
>>+	enum dpll_mode m;
>>+	struct nlattr *a;
>>+	int rem, ret = 0;
>>+	u32 source_pin;
>>+
>>+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>>+			  genlmsg_len(info->genlhdr), rem) {
>>+		switch (nla_type(a)) {
>>+		case DPLLA_MODE:
>>+			m = dpll_msg_read_mode(a);
>>+
>>+			ret = dpll_attr_mode_set(dpll, m);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		case DPLLA_SOURCE_PIN_IDX:
>>+			source_pin = dpll_msg_read_source_pin_id(a);
>>+
>>+			ret = dpll_attr_source_idx_set(dpll, source_pin);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		default:
>>+			break;
>>+		}
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static int dpll_cmd_device_set(struct sk_buff *skb, struct genl_info
>*info)
>>+{
>>+	struct dpll_attr *old = NULL, *new = NULL, *delta = NULL;
>>+	struct dpll_device *dpll = info->user_ptr[0];
>>+	int ret;
>>+
>>+	old = dpll_attr_alloc();
>>+	new = dpll_attr_alloc();
>>+	delta = dpll_attr_alloc();
>>+	if (!old || !new || !delta) {
>>+		ret = -ENOMEM;
>>+		goto mem_free;
>>+	}
>>+	dpll_lock(dpll);
>>+	ret = dpll_get_attr(dpll, old);
>>+	dpll_unlock(dpll);
>>+	if (!ret) {
>>+		dpll_attr_from_nlattr(new, info);
>>+		ret = dpll_attr_delta(delta, new, old);
>>+		if (!ret)
>>+			ret = dpll_set_attr(dpll, delta);
>>+	}
>>+
>>+mem_free:
>>+	dpll_attr_free(old);
>>+	dpll_attr_free(new);
>>+	dpll_attr_free(delta);
>>+
>>+	return ret;
>>+}
>>+
>>+static int
>>+dpll_cmd_device_dump(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;
>>+
>>+	hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh-
>>nlmsg_seq,
>>+			  &dpll_gnl_family, 0, DPLL_CMD_DEVICE_GET);
>>+	if (!hdr)
>>+		return -EMSGSIZE;
>>+
>>+	for_each_dpll(dpll, i) {
>>+		ret = dpll_device_dump_one(dpll, skb, ctx->dump_filter);
>>+		if (ret)
>>+			break;
>>+	}
>>+
>>+	if (ret)
>>+		genlmsg_cancel(skb, hdr);
>>+	else
>>+		genlmsg_end(skb, hdr);
>>+
>>+	return ret;
>>+}
>>+
>>+static int dpll_cmd_device_get(struct sk_buff *skb, struct genl_info
>*info)
>>+{
>>+	struct dpll_device *dpll = info->user_ptr[0];
>>+	struct nlattr **attrs = info->attrs;
>>+	struct sk_buff *msg;
>>+	int dump_filter = 0;
>>+	struct nlattr *hdr;
>>+	int ret;
>>+
>>+	if (attrs[DPLLA_DUMP_FILTER])
>>+		dump_filter =
>>+			dpll_msg_read_dump_filter(attrs[DPLLA_DUMP_FILTER]);
>>+
>>+	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>+	if (!msg)
>>+		return -ENOMEM;
>>+	hdr = genlmsg_put_reply(msg, info, &dpll_gnl_family, 0,
>>+				DPLL_CMD_DEVICE_GET);
>>+	if (!hdr)
>>+		return -EMSGSIZE;
>>+
>>+	ret = dpll_device_dump_one(dpll, msg, dump_filter);
>>+	if (ret)
>>+		goto out_free_msg;
>>+	genlmsg_end(msg, hdr);
>>+
>>+	return genlmsg_reply(msg, info);
>>+
>>+out_free_msg:
>>+	nlmsg_free(msg);
>>+	return ret;
>>+
>>+}
>>+
>>+static int dpll_cmd_device_get_start(struct netlink_callback *cb)
>>+{
>>+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>>+	struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
>>+	struct nlattr *attr = info->attrs[DPLLA_DUMP_FILTER];
>>+
>>+	if (attr)
>>+		ctx->dump_filter = dpll_msg_read_dump_filter(attr);
>>+	else
>>+		ctx->dump_filter = 0;
>>+
>>+	return 0;
>>+}
>>+
>>+static int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff
>*skb,
>>+			 struct genl_info *info)
>>+{
>>+	struct dpll_device *dpll_id = NULL, *dpll_name = NULL;
>>+
>>+	if (!info->attrs[DPLLA_ID] &&
>>+	    !info->attrs[DPLLA_NAME])
>>+		return -EINVAL;
>>+
>>+	if (info->attrs[DPLLA_ID]) {
>>+		u32 id = nla_get_u32(info->attrs[DPLLA_ID]);
>>+
>>+		dpll_id = dpll_device_get_by_id(id);
>>+		if (!dpll_id)
>>+			return -ENODEV;
>>+		info->user_ptr[0] = dpll_id;
>>+	}
>>+	if (info->attrs[DPLLA_NAME]) {
>>+		const char *name = nla_data(info->attrs[DPLLA_NAME]);
>>+
>>+		dpll_name = dpll_device_get_by_name(name);
>>+		if (!dpll_name)
>>+			return -ENODEV;
>>+
>>+		if (dpll_id && dpll_name != dpll_id)
>>+			return -EINVAL;
>>+		info->user_ptr[0] = dpll_name;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static const struct genl_ops dpll_ops[] = {
>>+	{
>>+		.cmd	= DPLL_CMD_DEVICE_GET,
>>+		.flags  = GENL_UNS_ADMIN_PERM,
>>+		.start	= dpll_cmd_device_get_start,
>>+		.dumpit	= dpll_cmd_device_dump,
>>+		.doit	= dpll_cmd_device_get,
>>+		.policy	= dpll_cmd_device_get_policy,
>>+		.maxattr = ARRAY_SIZE(dpll_cmd_device_get_policy) - 1,
>>+	},
>>+	{
>>+		.cmd	= DPLL_CMD_DEVICE_SET,
>>+		.flags	= GENL_UNS_ADMIN_PERM,
>>+		.doit	= dpll_cmd_device_set,
>>+		.policy	= dpll_cmd_device_set_policy,
>>+		.maxattr = ARRAY_SIZE(dpll_cmd_device_set_policy) - 1,
>>+	},
>>+	{
>>+		.cmd	= DPLL_CMD_PIN_SET,
>>+		.flags	= GENL_UNS_ADMIN_PERM,
>>+		.doit	= dpll_cmd_pin_set,
>>+		.policy	= dpll_cmd_pin_set_policy,
>>+		.maxattr = ARRAY_SIZE(dpll_cmd_pin_set_policy) - 1,
>>+	},
>>+};
>>+
>>+static struct genl_family dpll_family __ro_after_init = {
>>+	.hdrsize	= 0,
>>+	.name		= DPLL_FAMILY_NAME,
>>+	.version	= DPLL_VERSION,
>>+	.ops		= dpll_ops,
>>+	.n_ops		= ARRAY_SIZE(dpll_ops),
>>+	.mcgrps		= dpll_mcgrps,
>>+	.n_mcgrps	= ARRAY_SIZE(dpll_mcgrps),
>>+	.pre_doit	= dpll_pre_doit,
>>+	.parallel_ops   = true,
>>+};
>>+
>>+static int dpll_event_device_id(struct dpll_param *p)
>>+{
>>+	int ret = dpll_msg_add_id(p->msg, dpll_id(p->dpll));
>>+
>>+	if (ret)
>>+		return ret;
>>+	ret = dpll_msg_add_name(p->msg, dpll_dev_name(p->dpll));
>>+
>>+	return ret;
>>+}
>>+
>>+static int dpll_event_device_change(struct dpll_param *p)
>>+{
>>+	int ret = dpll_msg_add_id(p->msg, dpll_id(p->dpll));
>>+
>>+	if (ret)
>>+		return ret;
>>+	ret = dpll_msg_add_event_change_type(p->msg, p->change_type);
>>+	if (ret)
>>+		return ret;
>>+	switch (p->change_type)	{
>>+	case DPLL_CHANGE_PIN_ADD:
>>+	case DPLL_CHANGE_PIN_DEL:
>>+	case DPLL_CHANGE_PIN_TYPE:
>>+	case DPLL_CHANGE_PIN_SIGNAL_TYPE:
>>+	case DPLL_CHANGE_PIN_STATE:
>>+	case DPLL_CHANGE_PIN_PRIO:
>>+		ret = dpll_msg_add_pin_idx(p->msg, dpll_pin_idx(p->dpll, p-
>>pin));
>>+		break;
>>+	default:
>>+		break;
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static const cb_t event_cb[] = {
>>+	[DPLL_EVENT_DEVICE_CREATE]	= dpll_event_device_id,
>>+	[DPLL_EVENT_DEVICE_DELETE]	= dpll_event_device_id,
>>+	[DPLL_EVENT_DEVICE_CHANGE]	= dpll_event_device_change,
>>+};
>>+
>>+/*
>>+ * Generic netlink DPLL event encoding
>>+ */
>>+static int dpll_send_event(enum dpll_event event, struct dpll_param *p)
>>+{
>>+	struct sk_buff *msg;
>>+	int ret = -EMSGSIZE;
>>+	void *hdr;
>>+
>>+	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>+	if (!msg)
>>+		return -ENOMEM;
>>+	p->msg = msg;
>>+
>>+	hdr = genlmsg_put(msg, 0, 0, &dpll_family, 0, event);
>>+	if (!hdr)
>>+		goto out_free_msg;
>>+
>>+	ret = event_cb[event](p);
>>+	if (ret)
>>+		goto out_cancel_msg;
>>+
>>+	genlmsg_end(msg, hdr);
>>+
>>+	genlmsg_multicast(&dpll_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_notify_device_create(struct dpll_device *dpll)
>>+{
>>+	struct dpll_param p = { .dpll = dpll };
>>+
>>+	return dpll_send_event(DPLL_EVENT_DEVICE_CREATE, &p);
>>+}
>>+
>>+int dpll_notify_device_delete(struct dpll_device *dpll)
>>+{
>>+	struct dpll_param p = { .dpll = dpll };
>>+
>>+	return dpll_send_event(DPLL_EVENT_DEVICE_DELETE, &p);
>>+}
>>+
>>+int dpll_notify_device_change(struct dpll_device *dpll,
>>+			      enum dpll_event_change event,
>>+			      struct dpll_pin *pin)
>>+{
>>+	struct dpll_param p = { .dpll = dpll,
>>+				.change_type = event,
>>+				.pin = pin };
>
>This is odd. Why don't you just pass the object you want to expose the
>event for. You should have coupling between the object and send event
>function:
>dpll_device_notify(dpll, event);
>dpll_pin_notify(pin, event);
>Then you can avoid this param struct.

Makes sense to me.

>
>
>>+
>>+	return dpll_send_event(DPLL_EVENT_DEVICE_CHANGE, &p);
>>+}
>
>[...]
>
>
>>+/* dplla - Attributes of dpll generic netlink family
>>+ *
>>+ * @DPLLA_UNSPEC - invalid attribute
>>+ * @DPLLA_ID - ID of a dpll device (unsigned int)
>>+ * @DPLLA_NAME - human-readable name (char array of DPLL_NAME_LENGTH
>size)
>>+ * @DPLLA_MODE - working mode of dpll (enum dpll_mode)
>>+ * @DPLLA_MODE_SUPPORTED - list of supported working modes (enum
>dpll_mode)
>>+ * @DPLLA_SOURCE_PIN_ID - ID of source pin selected to drive dpll
>>+ *	(unsigned int)
>>+ * @DPLLA_LOCK_STATUS - dpll's lock status (enum dpll_lock_status)
>>+ * @DPLLA_TEMP - dpll's temperature (signed int - Celsius degrees)
>>+ * @DPLLA_DUMP_FILTER - filter bitmask (int, sum of DPLL_DUMP_FILTER_*
>defines)
>>+ * @DPLLA_NETIFINDEX - related network interface index
>>+ * @DPLLA_PIN - nested attribute, each contains single pin attributes
>>+ * @DPLLA_PIN_IDX - index of a pin on dpll (unsigned int)
>>+ * @DPLLA_PIN_DESCRIPTION - human-readable pin description provided by
>driver
>>+ *	(char array of PIN_DESC_LEN size)
>>+ * @DPLLA_PIN_TYPE - current type of a pin (enum dpll_pin_type)
>>+ * @DPLLA_PIN_TYPE_SUPPORTED - pin types supported (enum dpll_pin_type)
>>+ * @DPLLA_PIN_SIGNAL_TYPE - current type of a signal
>>+ *	(enum dpll_pin_signal_type)
>>+ * @DPLLA_PIN_SIGNAL_TYPE_SUPPORTED - pin signal types supported
>>+ *	(enum dpll_pin_signal_type)
>>+ * @DPLLA_PIN_CUSTOM_FREQ - freq value for
>DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ
>>+ *	(unsigned int)
>>+ * @DPLLA_PIN_STATE - state of pin's capabilities (enum dpll_pin_state)
>>+ * @DPLLA_PIN_STATE_SUPPORTED - available pin's capabilities
>>+ *	(enum dpll_pin_state)
>>+ * @DPLLA_PIN_PRIO - priority of a pin on dpll (unsigned int)
>>+ * @DPLLA_PIN_PARENT_IDX - if of a parent pin (unsigned int)
>>+ * @DPLLA_CHANGE_TYPE - type of device change event
>>+ *	(enum dpll_change_type)
>>+ * @DPLLA_PIN_NETIFINDEX - related network interface index for the pin
>>+ **/
>>+enum dplla {
>>+	DPLLA_UNSPEC,
>>+	DPLLA_ID,
>>+	DPLLA_NAME,
>>+	DPLLA_MODE,
>>+	DPLLA_MODE_SUPPORTED,
>>+	DPLLA_SOURCE_PIN_IDX,
>>+	DPLLA_LOCK_STATUS,
>>+	DPLLA_TEMP,
>
>Did you consider need for DPLLA_CLOCK_QUALITY? The our device exposes
>quality of the clock. SyncE daemon needs to be aware of the clock
>quality
>
>Also, how about the clock identification. I recall this being discussed
>in the past as well. This is also needed for SyncE daemon.
>DPLLA_CLOCK_ID - SyncE has it at 64bit number.
>

Yep, definitely I agree with both.

>
>>+	DPLLA_DUMP_FILTER,
>>+	DPLLA_NETIFINDEX,
>
>Duplicate, you have it under pin.

The pin can have netifindex as pin signal source may originate there by
Clock recovery mechanics.
The dpll can have ifindex as it "owns" the dpll.
Shall user know about it? probably nothing usefull for him, although
didn't Maciej Machnikowski asked to have such traceability?

Thanks,
Arkadiusz

>
>
>>+	DPLLA_PIN,
>>+	DPLLA_PIN_IDX,
>>+	DPLLA_PIN_DESCRIPTION,
>>+	DPLLA_PIN_TYPE,
>>+	DPLLA_PIN_TYPE_SUPPORTED,
>>+	DPLLA_PIN_SIGNAL_TYPE,
>>+	DPLLA_PIN_SIGNAL_TYPE_SUPPORTED,
>>+	DPLLA_PIN_CUSTOM_FREQ,
>>+	DPLLA_PIN_STATE,
>>+	DPLLA_PIN_STATE_SUPPORTED,
>>+	DPLLA_PIN_PRIO,
>>+	DPLLA_PIN_PARENT_IDX,
>>+	DPLLA_CHANGE_TYPE,
>>+	DPLLA_PIN_NETIFINDEX,
>>+	__DPLLA_MAX,
>>+};
>>+
>>+#define DPLLA_MAX (__DPLLA_MAX - 1)
>
>
>[...]

Powered by blists - more mailing lists