On 02.08.2022 15:02, Kubalewski, Arkadiusz wrote:

quoted

-----Original Message-----
From: Vadim Fedorenko <redacted>
Sent: Friday, July 15, 2022 1:29 AM

quoted

On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:

quoted

-----Original Message-----
From: Vadim Fedorenko <redacted>
Sent: Sunday, June 26, 2022 9:25 PM

quoted

From: Vadim Fedorenko <redacted>

Add netlink interface to enable notification of users about
events in DPLL framework. Part of this interface should be
used by drivers directly, i.e. lock status changes.

Signed-off-by: Vadim Fedorenko <redacted>
---
drivers/dpll/dpll_core.c    |   2 +
drivers/dpll/dpll_netlink.c | 141 ++++++++++++++++++++++++++++++++++++
drivers/dpll/dpll_netlink.h |   7 ++
3 files changed, 150 insertions(+)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index dc0330e3687d..387644aa910e 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c

@@ -97,6 +97,8 @@ struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_c

	mutex_unlock(&dpll_device_xa_lock);
	dpll->priv = priv;

+	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
+
	return dpll;

error:

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index e15106f30377..4b1684fcf41e 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c

@@ -48,6 +48,8 @@ struct param {

	int dpll_source_type;
	int dpll_output_id;
	int dpll_output_type;
+	int dpll_status;
+	const char *dpll_name;
};

struct dpll_dump_ctx {

@@ -239,6 +241,8 @@ static int dpll_genl_cmd_set_source(struct param *p)

	ret = dpll->ops->set_source_type(dpll, src_id, type);
	mutex_unlock(&dpll->lock);

+	dpll_notify_source_change(dpll->id, src_id, type);
+
	return ret;
}

@@ -262,6 +266,8 @@ static int dpll_genl_cmd_set_output(struct param *p)

	ret = dpll->ops->set_source_type(dpll, out_id, type);
	mutex_unlock(&dpll->lock);

+	dpll_notify_source_change(dpll->id, out_id, type);
+
	return ret;
}

@@ -438,6 +444,141 @@ static struct genl_family dpll_gnl_family __ro_after_init = {

	.pre_doit	= dpll_pre_doit,
};

+static int dpll_event_device_create(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+	    nla_put_string(p->msg, DPLLA_DEVICE_NAME, p->dpll_name))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_device_delete(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_status(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+		nla_put_u32(p->msg, DPLLA_LOCK_STATUS, p->dpll_status))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_source_change(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+	    nla_put_u32(p->msg, DPLLA_SOURCE_ID, p->dpll_source_id) ||
+		nla_put_u32(p->msg, DPLLA_SOURCE_TYPE, p->dpll_source_type))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_output_change(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+	    nla_put_u32(p->msg, DPLLA_OUTPUT_ID, p->dpll_output_id) ||
+		nla_put_u32(p->msg, DPLLA_OUTPUT_TYPE, p->dpll_output_type))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static cb_t event_cb[] = {
+	[DPLL_EVENT_DEVICE_CREATE]	= dpll_event_device_create,
+	[DPLL_EVENT_DEVICE_DELETE]	= dpll_event_device_delete,
+	[DPLL_EVENT_STATUS_LOCKED]	= dpll_event_status,
+	[DPLL_EVENT_STATUS_UNLOCKED]	= dpll_event_status,
+	[DPLL_EVENT_SOURCE_CHANGE]	= dpll_event_source_change,
+	[DPLL_EVENT_OUTPUT_CHANGE]	= dpll_event_output_change,
+};
+/*
+ * Generic netlink DPLL event encoding
+ */
+static int dpll_send_event(enum dpll_genl_event event,
+				   struct 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_gnl_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_gnl_family, msg, 0, 1, GFP_KERNEL);

All multicasts are send only for group "1" (DPLL_CONFIG_SOURCE_GROUP_NAME),
but 4 groups were defined.

Yes, you are right! Will update it in the next round.

quoted

quoted

+
+	return 0;
+
+out_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+out_free_msg:
+	nlmsg_free(msg);
+
+	return ret;
+}
+
+int dpll_notify_device_create(int dpll_id, const char *name)
+{
+	struct param p = { .dpll_id = dpll_id, .dpll_name = name };
+
+	return dpll_send_event(DPLL_EVENT_DEVICE_CREATE, &p);
+}
+
+int dpll_notify_device_delete(int dpll_id)
+{
+	struct param p = { .dpll_id = dpll_id };
+
+	return dpll_send_event(DPLL_EVENT_DEVICE_DELETE, &p);
+}
+
+int dpll_notify_status_locked(int dpll_id)
+{
+	struct param p = { .dpll_id = dpll_id, .dpll_status = 1 };
+
+	return dpll_send_event(DPLL_EVENT_STATUS_LOCKED, &p);
+}
+
+int dpll_notify_status_unlocked(int dpll_id)
+{
+	struct param p = { .dpll_id = dpll_id, .dpll_status = 0 };
+
+	return dpll_send_event(DPLL_EVENT_STATUS_UNLOCKED, &p);
+}
+
+int dpll_notify_source_change(int dpll_id, int source_id, int source_type)
+{
+	struct param p =  { .dpll_id = dpll_id, .dpll_source_id = source_id,
+						.dpll_source_type = source_type };
+
+	return dpll_send_event(DPLL_EVENT_SOURCE_CHANGE, &p);
+}
+
+int dpll_notify_output_change(int dpll_id, int output_id, int output_type)
+{
+	struct param p =  { .dpll_id = dpll_id, .dpll_output_id = output_id,
+						.dpll_output_type = output_type };
+
+	return dpll_send_event(DPLL_EVENT_OUTPUT_CHANGE, &p);
+}
+
int __init dpll_netlink_init(void)
{
	return genl_register_family(&dpll_gnl_family);

diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
index e2d100f59dd6..0dc81320f982 100644
--- a/drivers/dpll/dpll_netlink.h
+++ b/drivers/dpll/dpll_netlink.h

@@ -3,5 +3,12 @@
   *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
   */

+int dpll_notify_device_create(int dpll_id, const char *name);
+int dpll_notify_device_delete(int dpll_id);
+int dpll_notify_status_locked(int dpll_id);
+int dpll_notify_status_unlocked(int dpll_id);
+int dpll_notify_source_change(int dpll_id, int source_id, int source_type);
+int dpll_notify_output_change(int dpll_id, int output_id, int output_type);

Only dpll_notify_device_create is actually used, rest is not.
I am getting confused a bit, who should call those "notify" functions?
It is straightforward for create/delete, dpll subsystem shall do it, but what
about the rest?
I would say notifications about status or source/output change shall originate
in the driver implementing dpll interface, thus they shall be exported and
defined in the header included by the driver.

I was thinking about driver too, because device can have different interfaces to
configure source/output, and different notifications to update status. I will
update ptp_ocp driver to implement this logic. And it will also cover question
of exporting these functions and their definitions.

Great!

Thank,
Arkadiusz

quoted

quoted

quoted

+
int __init dpll_netlink_init(void);
void dpll_netlink_finish(void);
-- 
2.27.0

Good day Vadim,

I just wanted to make sure I didn't miss anything through the spam filters or
something? We are still waiting for that github repo, or you were on
vacation/busy, right?

Hi Arkadiusz, Jakub

Actually I was on vacation which lasted unexpectedly long thanks for european 
airlines. So was busy catching up all the things happened while I was away.
Finally I created github repo with all the comments from previous conversation 
addressed and rebased on top of current net-next. No special progress apart from 
that, still need some time to prepare RFC v3 with documentation and proper 
driver usage, but current state should be usable for priorities implementation 
and simple tests:

https://github.com/vvfedorenko/linux-dpll.git

Ping me on github to have a write access to this repo, and sorry for being so late.

All best,
Vadim