diff mbox series

[RFC,v2,2/3] dpll: add netlink events

Message ID 20220626192444.29321-3-vfedorenko@novek.ru (mailing list archive)
State RFC
Headers show
Series Create common DPLL/clock configuration API | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 2 of 2 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning CHECK: Alignment should match open parenthesis CHECK: Please use a blank line after function/struct/union/enum declarations WARNING: line length of 82 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vadim Fedorenko June 26, 2022, 7:24 p.m. UTC 
From: Vadim Fedorenko <vadfed@fb.com>

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 <vadfed@fb.com>
---
 drivers/dpll/dpll_core.c    |   2 +
 drivers/dpll/dpll_netlink.c | 141 ++++++++++++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.h |   7 ++
 3 files changed, 150 insertions(+)

Comments

Kubalewski, Arkadiusz July 11, 2022, 9:02 a.m. UTC | #1 
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Sunday, June 26, 2022 9:25 PM
>
>From: Vadim Fedorenko <vadfed@fb.com>
>
>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 <vadfed@fb.com>
>---
> 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.
  
>+
>+	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.

>+
> int __init dpll_netlink_init(void);
> void dpll_netlink_finish(void);
>-- 
>2.27.0
>
Vadim Fedorenko July 14, 2022, 11:29 p.m. UTC | #2 
On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
> -----Original Message-----
> From: Vadim Fedorenko <vfedorenko@novek.ru>
> Sent: Sunday, June 26, 2022 9:25 PM
>>
>> From: Vadim Fedorenko <vadfed@fb.com>
>>
>> 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 <vadfed@fb.com>
>> ---
>> 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.

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

>> +
>> int __init dpll_netlink_init(void);
>> void dpll_netlink_finish(void);
>> -- 
>> 2.27.0
>>
Kubalewski, Arkadiusz July 15, 2022, 5:31 p.m. UTC | #3 
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Friday, July 15, 2022 1:29 AM
>
>On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>> Sent: Sunday, June 26, 2022 9:25 PM
>>>
>>> From: Vadim Fedorenko <vadfed@fb.com>
>>>
>>> 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 <vadfed@fb.com>
>>> ---
>>> 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.
>
>>> +
>>> +	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
>>> +
>>> int __init dpll_netlink_init(void);
>>> void dpll_netlink_finish(void);
>>> -- 
>>> 2.27.0
>>>
>
Kubalewski, Arkadiusz Aug. 2, 2022, 2:02 p.m. UTC | #4 
>-----Original Message-----
>From: Vadim Fedorenko <vfedorenko@novek.ru> 
>Sent: Friday, July 15, 2022 1:29 AM
>>
>>On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
>>> -----Original Message-----
>>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>>> Sent: Sunday, June 26, 2022 9:25 PM
>>>>
>>>> From: Vadim Fedorenko <vadfed@fb.com>
>>>>
>>>> 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 <vadfed@fb.com>
>>>> ---
>>>> 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.
>>
>>>> +
>>>> +	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
>>>> +
>>>> 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?

Thanks,
Arkadiusz
Jakub Kicinski Aug. 2, 2022, 3:52 p.m. UTC | #5 
On Tue, 2 Aug 2022 14:02:31 +0000 Kubalewski, Arkadiusz wrote:
> 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?

Great timing, I was just asking Vadim for the code as well.
I should be able to share the auto-generated user space library 
for the netlink interface soon after Vadim shares the code.
Still very much WIP but, well, likely better than parsing by hand.
Vadim Fedorenko Aug. 3, 2022, 12:05 a.m. UTC | #6 
On 02.08.2022 15:02, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>> Sent: Friday, July 15, 2022 1:29 AM
>>>
>>> On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
>>>> -----Original Message-----
>>>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>>>> Sent: Sunday, June 26, 2022 9:25 PM
>>>>>
>>>>> From: Vadim Fedorenko <vadfed@fb.com>
>>>>>
>>>>> 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 <vadfed@fb.com>
>>>>> ---
>>>>> 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.
>>>
>>>>> +
>>>>> +	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
>>>>> +
>>>>> 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
Stephen Hemminger Aug. 3, 2022, 3:21 p.m. UTC | #7 
On Sun, 26 Jun 2022 22:24:43 +0300
Vadim Fedorenko <vfedorenko@novek.ru> wrote:

> +
> +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,
> +};

Function tables in kernel should always be const for added security
Jiri Pirko Sept. 29, 2022, 12:13 p.m. UTC | #8 
Sun, Jun 26, 2022 at 09:24:43PM CEST, vfedorenko@novek.ru wrote:
>From: Vadim Fedorenko <vadfed@fb.com>
>
>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.

I don't get why this is a separate patch. I believe it should be
squashed to the previous one, making it easier to review as a whole
thing.


>
>Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
>---
> 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 param". Can't you please maintain some namespace for
function/struct names?


>+{
>+	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);
>+
>+	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);

Just do that automatically in register() and avoid the need to rely on
drivers to be so good to do it themselves. They won't.


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

For all notification functions called from the driver, please use struct
dpll as an arg.


>+{
>+	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);

Why these are not returning void? Does driver care about the return
value? Why?


>+
> int __init dpll_netlink_init(void);
> void dpll_netlink_finish(void);
>-- 
>2.27.0
>
Vadim Fedorenko Sept. 30, 2022, 12:48 a.m. UTC | #9 
On 29.09.2022 13:13, Jiri Pirko wrote:
> Sun, Jun 26, 2022 at 09:24:43PM CEST, vfedorenko@novek.ru wrote:
>> From: Vadim Fedorenko <vadfed@fb.com>
>>
>> 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.
> 
> I don't get why this is a separate patch. I believe it should be
> squashed to the previous one, making it easier to review as a whole
> thing.
> 

I was trying to separate some functions to make review process a bit simplier.

> 
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
>> ---
>> 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 param". Can't you please maintain some namespace for
> function/struct names?
>
Ok, sure!
> 
>> +{
>> +	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);
>> +
>> +	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);
> 
> Just do that automatically in register() and avoid the need to rely on
> drivers to be so good to do it themselves. They won't.
> 
Yeah, the next version will have these changes.

> 
>> +}
>> +
>> +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)
> 
> For all notification functions called from the driver, please use struct
> dpll as an arg.

Will change it, thanks!

> 
>> +{
>> +	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);
> 
> Why these are not returning void? Does driver care about the return
> value? Why?
>
Yep, you are right, I will change to void because there is no reason to have a 
return value, thanks!

> 
>> +
>> int __init dpll_netlink_init(void);
>> void dpll_netlink_finish(void);
>> -- 
>> 2.27.0
>>
diff mbox series

Patch

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);
+
+	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);
+
 int __init dpll_netlink_init(void);
 void dpll_netlink_finish(void);