RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

["Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>]

-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Friday, June 24, 2022 12:48 AM

>Hi Arkadiusz!
>
>On 23.06.2022 16:33, Kubalewski, Arkadiusz wrote:
>> Hi Vadim,
>> 
>> Great work!
>> 
>> Although, I've been thinking that you already forget about it, so I have
>> started development of something similar.
>>
>
>Sorry for making you wait for so long. I'm happy to merge your work into these 
>series and to continue collaboration to further improve subsystem.

Not a problem, sounds great!

>
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +#
>>> +# Generic DPLL drivers configuration
>>> +#
>>> +
>>> +config DPLL
>>> +  bool
>> 
>> for RFC help and default were ommited?
>> 
>
>In private conversation with Jakub we decided to hide this subsystem from user
>facing menu and enable via in-kernel customer's dependency. If you think it's 
>better to let users enable or disable it, we can bring this discussion back to 
>wider audience.

Well, I won't insist on it. Seems fair.

>
>>> diff --git a/drivers/dpll/Makefile b/drivers/dpll/Makefile
>>> new file mode 100644
>>> index 000000000000..0748c80097e4
>>> --- /dev/null
>>> +++ b/drivers/dpll/Makefile
>>> @@ -0,0 +1,7 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +#
>>> +# Makefile for DPLL drivers.
>>> +#
>>> +
>>> +obj-$(CONFIG_DPLL)          += dpll_sys.o
>>> +dpll_sys-y                  += dpll_core.o dpll_netlink.o
>>> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>> new file mode 100644
>>> index 000000000000..e34767e723cf
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_core.c
>>> @@ -0,0 +1,152 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + *  dpll_core.c - Generic DPLL Management class support.
>>> + *
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/err.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/string.h>
>>> +
>>> +#include "dpll_core.h"
>>> +
>>> +static DEFINE_MUTEX(dpll_device_xa_lock);
>>> +static DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
>>> +#define DPLL_REGISTERED XA_MARK_1
>>> +
>>> +#define ASSERT_DPLL_REGISTERED(d)                                           \
>>> +	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>> +#define ASSERT_DPLL_NOT_REGISTERED(d)                                      \
>>> +	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>> +
>>> +
>>> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *), void *data)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	unsigned long index;
>>> +	int ret = 0;
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_for_each_start(&dpll_device_xa, index, dpll, id) {
>>> +		if (!xa_get_mark(&dpll_device_xa, index, DPLL_REGISTERED))
>>> +			continue;
>>> +		ret = cb(dpll, data);
>>> +		if (ret)
>>> +			break;
>>> +	}
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +struct dpll_device *dpll_device_get_by_id(int id)
>>> +{
>>> +	struct dpll_device *dpll = NULL;
>>> +
>>> +	if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
>>> +		dpll = xa_load(&dpll_device_xa, id);
>>> +	return dpll;
>>> +}
>>> +
>>> +void *dpll_priv(struct dpll_device *dpll)
>>> +{
>>> +	return dpll->priv;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_priv);
>>> +
>>> +static void dpll_device_release(struct device *dev)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +
>>> +	dpll = to_dpll_device(dev);
>>> +
>>> +	dpll_device_unregister(dpll);
>>> +
>>> +	mutex_destroy(&dpll->lock);
>>> +	kfree(dpll);
>>> +}
>>> +
>>> +static struct class dpll_class = {
>>> +	.name = "dpll",
>>> +	.dev_release = dpll_device_release,
>>> +};
>>> +
>>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>>> +					 int outputs_count, void *priv)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	int ret;
>>> +
>>> +	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>>> +	if (!dpll)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	mutex_init(&dpll->lock);
>>> +	dpll->ops = ops;
>>> +	dpll->dev.class = &dpll_class;
>>> +	dpll->sources_count = sources_count;
>>> +	dpll->outputs_count = outputs_count;
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, GFP_KERNEL);
>>> +	if (ret)
>>> +		goto error;
>>> +	dev_set_name(&dpll->dev, "dpll%d", dpll->id);
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +	dpll->priv = priv;
>>> +
>>> +	return dpll;
>>> +
>>> +error:
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +	kfree(dpll);
>>> +	return ERR_PTR(ret);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_alloc);
>>> +
>>> +void dpll_device_register(struct dpll_device *dpll)
>>> +{
>>> +	ASSERT_DPLL_NOT_REGISTERED(dpll);
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
>>> +	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_register);
>>> +
>>> +void dpll_device_unregister(struct dpll_device *dpll)
>>> +{
>>> +	ASSERT_DPLL_REGISTERED(dpll);
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_erase(&dpll_device_xa, dpll->id);
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_unregister);
>>> +
>>> +static int __init dpll_init(void)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = dpll_netlink_init();
>>> +	if (ret)
>>> +		goto error;
>>> +
>>> +	ret = class_register(&dpll_class);
>>> +	if (ret)
>>> +		goto unregister_netlink;
>>> +
>>> +	return 0;
>>> +
>>> +unregister_netlink:
>>> +	dpll_netlink_finish();
>>> +error:
>>> +	mutex_destroy(&dpll_device_xa_lock);
>>> +	return ret;
>>> +}
>>> +subsys_initcall(dpll_init);
>>> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>> new file mode 100644
>>> index 000000000000..5ad3224d5caf
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_core.h
>>> @@ -0,0 +1,40 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +#ifndef __DPLL_CORE_H__
>>> +#define __DPLL_CORE_H__
>>> +
>>> +#include <linux/dpll.h>
>>> +
>>> +#include "dpll_netlink.h"
>>> +
>>> +/**
>>> + * struct dpll_device - structure for a DPLL device
>>> + * @id:		unique id number for each edvice
>>> + * @dev:	&struct device for this dpll device
>>> + * @sources_count:	amount of input sources this dpll_device supports
>>> + * @outputs_count:	amount of outputs this dpll_device supports
>>> + * @ops:	operations this &dpll_device supports
>>> + * @lock:	mutex to serialize operations
>>> + * @priv:	pointer to private information of owner
>>> + */
>>> +struct dpll_device {
>>> +	int id;
>>> +	struct device dev;
>>> +	int sources_count;
>>> +	int outputs_count;
>>> +	struct dpll_device_ops *ops;
>>> +	struct mutex lock;
>>> +	void *priv;
>>> +};
>>> +
>>> +#define to_dpll_device(_dev) \
>>> +	container_of(_dev, struct dpll_device, dev)
>>> +
>>> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *),
>>> +			  void *data);
>>> +struct dpll_device *dpll_device_get_by_id(int id);
>>> +void dpll_device_unregister(struct dpll_device *dpll);
>>> +#endif
>>> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>> new file mode 100644
>>> index 000000000000..0bbdaa6dde8e
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_netlink.c
>>> @@ -0,0 +1,437 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Generic netlink for DPLL management framework
>>> + *
>>> + * Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + *
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <net/genetlink.h>
>>> +#include "dpll_core.h"
>>> +
>>> +#include <uapi/linux/dpll.h>
>>> +
>>> +static const struct genl_multicast_group dpll_genl_mcgrps[] = {
>>> +	{ .name = DPLL_CONFIG_DEVICE_GROUP_NAME, },
>>> +	{ .name = DPLL_CONFIG_SOURCE_GROUP_NAME, },
>>> +	{ .name = DPLL_CONFIG_OUTPUT_GROUP_NAME, },
>>> +	{ .name = DPLL_MONITOR_GROUP_NAME,  },
>>> +};
>>> +
>>> +static const struct nla_policy dpll_genl_get_policy[] = {
>>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_DEVICE_NAME]	= { .type = NLA_STRING,
>>> +				    .len = DPLL_NAME_LENGTH },
>>> +	[DPLLA_FLAGS]		= { .type = NLA_U32 },
>>> +};
>>> +
>>> +static const struct nla_policy dpll_genl_set_source_policy[] = {
>>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_SOURCE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_SOURCE_TYPE]	= { .type = NLA_U32 },
>>> +};
>>> +
>>> +static const struct nla_policy dpll_genl_set_output_policy[] = {
>>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_OUTPUT_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_OUTPUT_TYPE]	= { .type = NLA_U32 },
>>> +};
>>> +
>>> +struct param {
>>> +	struct netlink_callback *cb;
>>> +	struct dpll_device *dpll;
>>> +	struct nlattr **attrs;
>>> +	struct sk_buff *msg;
>>> +	int dpll_id;
>>> +	int dpll_source_id;
>>> +	int dpll_source_type;
>>> +	int dpll_output_id;
>>> +	int dpll_output_type;
>>> +};
>>> +
>>> +struct dpll_dump_ctx {
>>> +	struct dpll_device *dev;
>>> +	int flags;
>>> +	int pos_idx;
>>> +	int pos_src_idx;
>>> +	int pos_out_idx;
>>> +};
>>> +
>>> +typedef int (*cb_t)(struct 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_cmd_device_dump_one(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	if (nla_put_u32(msg, DPLLA_DEVICE_ID, dpll->id))
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, DPLLA_DEVICE_NAME, dev_name(&dpll->dev)))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	struct nlattr *src_attr;
>>> +	int i, ret = 0, type;
>>> +
>>> +	for (i = 0; i < dpll->sources_count; i++) {
>>> +		src_attr = nla_nest_start(msg, DPLLA_SOURCE);
>>> +		if (!src_attr) {
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		type = dpll->ops->get_source_type(dpll, i);
>>> +		if (nla_put_u32(msg, DPLLA_SOURCE_ID, i) ||
>>> +		    nla_put_u32(msg, DPLLA_SOURCE_TYPE, type)) {
>>> +			nla_nest_cancel(msg, src_attr);
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		nla_nest_end(msg, src_attr);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	struct nlattr *out_attr;
>>> +	int i, ret = 0, type;
>>> +
>>> +	for (i = 0; i < dpll->outputs_count; i++) {
>>> +		out_attr = nla_nest_start(msg, DPLLA_OUTPUT);
>>> +		if (!out_attr) {
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		type = dpll->ops->get_source_type(dpll, i);
>>> +		if (nla_put_u32(msg, DPLLA_OUTPUT_ID, i) ||
>>> +		    nla_put_u32(msg, DPLLA_OUTPUT_TYPE, type)) {
>>> +			nla_nest_cancel(msg, out_attr);
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		nla_nest_end(msg, out_attr);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __dpll_cmd_dump_status(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
>>> +		return 0;
>> 
>> what if dpll doesn't support one of those commands?
>> 
>
>then only supported attributes will be messaged back to user

Hmm, isn't that redundat if we need to check those again below?

>
>>> +
>>> +	if (dpll->ops->get_status) {
>>> +		ret = dpll->ops->get_status(dpll);
>>> +		if (nla_put_u32(msg, DPLLA_STATUS, ret))
>>> +			return -EMSGSIZE;
>>> +	}
>>> +
>>> +	if (dpll->ops->get_temp) {
>>> +		ret = dpll->ops->get_status(dpll);
>>> +		if (nla_put_u32(msg, DPLLA_TEMP, ret))
>>> +			return -EMSGSIZE;
>>> +	}
>> 
>> shouldn't be get_temp(dpll)?
>
>good catch, copy-paste error
>
>>> +
>>> +	if (dpll->ops->get_lock_status) {
>>> +		ret = dpll->ops->get_lock_status(dpll);
>>> +		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
>>> +			return -EMSGSIZE;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>>> +{
>>> +	struct nlattr *hdr;
>>> +	int ret;
>>> +
>>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	mutex_lock(&dev->lock);
>>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>>> +	if (ret)
>>> +		goto out_cancel_nest;
>>> +
>>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>>> +		if (ret)
>>> +			goto out_cancel_nest;
>>> +	}
>>> +
>>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>>> +		if (ret)
>>> +			goto out_cancel_nest;
>>> +	}
>>> +
>>> +	if (flags & DPLL_FLAG_STATUS) {
>>> +		ret = __dpll_cmd_dump_status(dev, msg);
>>> +		if (ret)
>>> +			goto out_cancel_nest;
>>> +	}
>>> +
>>> +	mutex_unlock(&dev->lock);
>>> +	nla_nest_end(msg, hdr);
>>> +
>>> +	return 0;
>>> +
>>> +out_cancel_nest:
>>> +	mutex_unlock(&dev->lock);
>>> +	nla_nest_cancel(msg, hdr);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_set_source(struct param *p)
>>> +{
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>> +	struct dpll_device *dpll = p->dpll;
>>> +	int ret = 0, src_id, type;
>>> +
>>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>>> +		return -EINVAL;
>>> +
>>> +	if (!dpll->ops->set_source_type)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>>> +
>>> +	mutex_lock(&dpll->lock);
>>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>>> +	mutex_unlock(&dpll->lock);

I wonder if shouldn't the dpll ptr be validated here, and in similar cases.
I mean, between calling dpll_pre_doit and actually doing something on a 'dpll',
it is possible that device gets removed?

Or maybe pre_doit/post_doit shall lock and unlock some other mutex?
Altough, I am not an expert in the netlink stuff, thus just raising a concern.

>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_set_output(struct param *p)
>>> +{
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>> +	struct dpll_device *dpll = p->dpll;
>>> +	int ret = 0, out_id, type;
>>> +
>>> +	if (!info->attrs[DPLLA_OUTPUT_ID] ||
>>> +	    !info->attrs[DPLLA_OUTPUT_TYPE])
>>> +		return -EINVAL;
>>> +
>>> +	if (!dpll->ops->set_output_type)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	out_id = nla_get_u32(info->attrs[DPLLA_OUTPUT_ID]);
>>> +	type = nla_get_u32(info->attrs[DPLLA_OUTPUT_TYPE]);
>>> +
>>> +	mutex_lock(&dpll->lock);
>>> +	ret = dpll->ops->set_source_type(dpll, out_id, type);
>>> +	mutex_unlock(&dpll->lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_device_loop_cb(struct dpll_device *dpll, void *data)
>>> +{
>>> +	struct dpll_dump_ctx *ctx;
>>> +	struct param *p = (struct param *)data;
>>> +
>>> +	ctx = dpll_dump_context(p->cb);
>>> +
>>> +	ctx->pos_idx = dpll->id;
>>> +
>>> +	return dpll_device_dump_one(dpll, p->msg, ctx->flags);
>>> +}
>>> +
>>> +static int dpll_cmd_device_dump(struct param *p)
>>> +{
>>> +	struct dpll_dump_ctx *ctx = dpll_dump_context(p->cb);
>>> +
>>> +	return for_each_dpll_device(ctx->pos_idx, dpll_device_loop_cb, p);
>>> +}
>>> +
>>> +static int dpll_genl_cmd_device_get_id(struct param *p)
>>> +{
>>> +	struct dpll_device *dpll = p->dpll;
>>> +	int flags = 0;
>>> +
>>> +	if (p->attrs[DPLLA_FLAGS])
>>> +		flags = nla_get_u32(p->attrs[DPLLA_FLAGS]);
>>> +
>>> +	return dpll_device_dump_one(dpll, p->msg, flags);
>>> +}
>>> +
>>> +static cb_t cmd_doit_cb[] = {
>>> +	[DPLL_CMD_DEVICE_GET]		= dpll_genl_cmd_device_get_id,
>>> +	[DPLL_CMD_SET_SOURCE_TYPE]	= dpll_genl_cmd_set_source,
>>> +	[DPLL_CMD_SET_OUTPUT_TYPE]	= dpll_genl_cmd_set_output,
>>> +};
>>> +
>>> +static cb_t cmd_dump_cb[] = {
>>> +	[DPLL_CMD_DEVICE_GET]		= dpll_cmd_device_dump,
>>> +};
>>> +
>>> +static int dpll_genl_cmd_start(struct netlink_callback *cb)
>>> +{
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>>> +	struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
>>> +
>>> +	ctx->dev = NULL;
>>> +	if (info->attrs[DPLLA_FLAGS])
>>> +		ctx->flags = nla_get_u32(info->attrs[DPLLA_FLAGS]);
>>> +	else
>>> +		ctx->flags = 0;
>>> +	ctx->pos_idx = 0;
>>> +	ctx->pos_src_idx = 0;
>>> +	ctx->pos_out_idx = 0;
>>> +	return 0;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_dumpit(struct sk_buff *skb,
>>> +				   struct netlink_callback *cb)
>>> +{
>>> +	struct param p = { .cb = cb, .msg = skb };
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>>> +	int cmd = info->op.cmd;
>>> +	int ret;
>>> +	void *hdr;
>>> +
>>> +	hdr = genlmsg_put(skb, 0, 0, &dpll_gnl_family, 0, cmd);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	ret = cmd_dump_cb[cmd](&p);
>>> +	if (ret)
>>> +		goto out_cancel_msg;
>>> +
>>> +	genlmsg_end(skb, hdr);
>>> +
>>> +	return 0;
>>> +
>>> +out_cancel_msg:
>>> +	genlmsg_cancel(skb, hdr);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_doit(struct sk_buff *skb,
>>> +				 struct genl_info *info)
>>> +{
>>> +	struct param p = { .attrs = info->attrs, .dpll = info->user_ptr[0] };
>>> +	int cmd = info->genlhdr->cmd;
>>> +	struct sk_buff *msg;
>>> +	void *hdr;
>>> +	int ret;
>>> +
>>> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +	p.msg = msg;
>>> +
>>> +	hdr = genlmsg_put_reply(msg, info, &dpll_gnl_family, 0, cmd);
>>> +	if (!hdr) {
>>> +		ret = -EMSGSIZE;
>>> +		goto out_free_msg;
>>> +	}
>>> +
>>> +	ret = cmd_doit_cb[cmd](&p);
>>> +	if (ret)
>>> +		goto out_cancel_msg;
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +
>>> +	return genlmsg_reply(msg, info);
>>> +
>>> +out_cancel_msg:
>>> +	genlmsg_cancel(msg, hdr);
>>> +out_free_msg:
>>> +	nlmsg_free(msg);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>>> +						 struct genl_info *info)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	int id;
>>> +
>>> +	if (!info->attrs[DPLLA_DEVICE_ID])
>>> +		return -EINVAL;
>>> +	id = nla_get_u32(info->attrs[DPLLA_DEVICE_ID]);
>>> +
>>> +	dpll = dpll_device_get_by_id(id);
>>> +	if (!dpll)
>>> +		return -ENODEV;
>>> +	info->user_ptr[0] = dpll;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct genl_ops dpll_genl_ops[] = {
>>> +	{
>>> +		.cmd	= DPLL_CMD_DEVICE_GET,
>>> +		.start	= dpll_genl_cmd_start,
>>> +		.dumpit	= dpll_genl_cmd_dumpit,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_get_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_get_policy) - 1,
>>> +	},
>>> +	{
>>> +		.cmd	= DPLL_CMD_SET_SOURCE_TYPE,
>>> +		.flags	= GENL_UNS_ADMIN_PERM,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_set_source_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_set_source_policy) - 1,
>>> +	},
>>> +	{
>>> +		.cmd	= DPLL_CMD_SET_OUTPUT_TYPE,
>>> +		.flags	= GENL_UNS_ADMIN_PERM,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_set_output_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_set_output_policy) - 1,
>>> +	},
>>> +};
>>> +
>>> +static struct genl_family dpll_gnl_family __ro_after_init = {
>>> +	.hdrsize	= 0,
>>> +	.name		= DPLL_FAMILY_NAME,
>>> +	.version	= DPLL_VERSION,
>>> +	.ops		= dpll_genl_ops,
>>> +	.n_ops		= ARRAY_SIZE(dpll_genl_ops),
>>> +	.mcgrps		= dpll_genl_mcgrps,
>>> +	.n_mcgrps	= ARRAY_SIZE(dpll_genl_mcgrps),
>>> +	.pre_doit	= dpll_pre_doit,
>>> +};
>>> +
>>> +int __init dpll_netlink_init(void)
>>> +{
>>> +	return genl_register_family(&dpll_gnl_family);
>>> +}
>>> +
>>> +void dpll_netlink_finish(void)
>>> +{
>>> +	genl_unregister_family(&dpll_gnl_family);
>>> +}
>>> +
>>> +void __exit dpll_netlink_fini(void)
>>> +{
>>> +	dpll_netlink_finish();
>>> +}
>>> diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
>>> new file mode 100644
>>> index 000000000000..e2d100f59dd6
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_netlink.h
>>> @@ -0,0 +1,7 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +int __init dpll_netlink_init(void);
>>> +void dpll_netlink_finish(void);
>>> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>> new file mode 100644
>>> index 000000000000..9051337bcf9e
>>> --- /dev/null
>>> +++ b/include/linux/dpll.h
>>> @@ -0,0 +1,25 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +#ifndef __DPLL_H__
>>> +#define __DPLL_H__
>>> +
>>> +struct dpll_device;
>>> +
>>> +struct dpll_device_ops {
>>> +	int (*get_status)(struct dpll_device *dpll);
>>> +	int (*get_temp)(struct dpll_device *dpll);
>>> +	int (*get_lock_status)(struct dpll_device *dpll);
>>> +	int (*get_source_type)(struct dpll_device *dpll, int id);
>>> +	int (*get_output_type)(struct dpll_device *dpll, int id);
>>> +	int (*set_source_type)(struct dpll_device *dpll, int id, int val);
>>> +	int (*set_output_type)(struct dpll_device *dpll, int id, int val);
>>> +};
>>> +
>>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>>> +					 int outputs_count, void *priv);
>>> +void dpll_device_register(struct dpll_device *dpll);
>>> +void *dpll_priv(struct dpll_device *dpll);
>>> +#endif
>>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>>> new file mode 100644
>>> index 000000000000..8c00f52736ee
>>> --- /dev/null
>>> +++ b/include/uapi/linux/dpll.h
>>> @@ -0,0 +1,77 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +#ifndef _UAPI_LINUX_DPLL_H
>>> +#define _UAPI_LINUX_DPLL_H
>>> +
>>> +#define DPLL_NAME_LENGTH	20
>>> +
>>> +/* Adding event notification support elements */
>>> +#define DPLL_FAMILY_NAME		"dpll"
>>> +#define DPLL_VERSION			0x01
>>> +#define DPLL_CONFIG_DEVICE_GROUP_NAME  "config"
>>> +#define DPLL_CONFIG_SOURCE_GROUP_NAME  "source"
>>> +#define DPLL_CONFIG_OUTPUT_GROUP_NAME  "output"
>>> +#define DPLL_MONITOR_GROUP_NAME        "monitor"
>>> +
>>> +#define DPLL_FLAG_SOURCES	1
>>> +#define DPLL_FLAG_OUTPUTS	2
>>> +#define DPLL_FLAG_STATUS	4
>>> +
>>> +/* Attributes of dpll_genl_family */
>>> +enum dpll_genl_get_attr {
>>> +	DPLLA_UNSPEC,
>>> +	DPLLA_DEVICE,
>>> +	DPLLA_DEVICE_ID,
>>> +	DPLLA_DEVICE_NAME,
>>> +	DPLLA_SOURCE,
>>> +	DPLLA_SOURCE_ID,
>>> +	DPLLA_SOURCE_TYPE,
>>> +	DPLLA_OUTPUT,
>>> +	DPLLA_OUTPUT_ID,
>>> +	DPLLA_OUTPUT_TYPE,
>>> +	DPLLA_STATUS,
>>> +	DPLLA_TEMP,
>>> +	DPLLA_LOCK_STATUS,
>>> +	DPLLA_FLAGS,
>>> +
>>> +	__DPLLA_MAX,
>>> +};
>>> +#define DPLLA_GET_MAX (__DPLLA_MAX - 1)
>> 
>> I think "_get_/_GET_" in above names is outdated?
>> 
>
>Yes, you are right. The earliest revision had these "GET/SET" in attributes but 
>later we decided to unite them into common attributes. I will remove these 
>artifacts in the next revision.
>
>>> +
>>> +/* DPLL signal types used as source or as output */
>>> +enum dpll_genl_signal_type {
>>> +	DPLL_TYPE_EXT_1PPS,
>>> +	DPLL_TYPE_EXT_10MHZ,
>>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>>> +	DPLL_TYPE_INT_OSCILLATOR,
>>> +	DPLL_TYPE_GNSS,
>>> +
>>> +	__DPLL_TYPE_MAX,
>>> +};
>>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>>> +
>>> +/* Events of dpll_genl_family */
>>> +enum dpll_genl_event {
>>> +	DPLL_EVENT_UNSPEC,
>>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>>> +
>>> +	__DPLL_EVENT_MAX,
>>> +};
>>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>>> +
>>> +/* Commands supported by the dpll_genl_family */
>>> +enum dpll_genl_cmd {
>>> +	DPLL_CMD_UNSPEC,
>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
>> 
>> "Get" in comment description looks like a typo.
>> I am getting bit confused with the name and comments.
>> For me, first look says: it is selection of a type of a source.
>> But in the code I can see it selects a source id and a type.
>> Type of source originates in HW design, why would the one want to "set" it?
>> I can imagine a HW design where a single source or output would allow to choose
>> where the signal originates/goes, some kind of extra selector layer for a
>> source/output, but was that the intention?
>
>In general - yes, we have experimented with our time card providing different 
>types of source synchronisation signal on different input pins, i.e. 1PPS, 
>10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external 
>pins, that's why I source id is treated as input pin identifier and source type 
>is the signal type it receives.
>
>> If so, shouldn't the user get some bitmap/list of modes available for each
>> source/output?
>
>Good idea. We have list of available modes exposed via sysfs file, and I agree 
>that it's worth to expose them via netlink interface. I will try to address this 
>in the next version.
>
>> 
>> The user shall get some extra information about the source/output. Right now
>> there can be multiple sources/outputs of the same type, but not really possible
>> to find out their purpose. I.e. a dpll equipped with four source of
>> DPLL_TYPE_EXT_1PPS type.
>>  > This implementation looks like designed for a "forced reference lock" mode
>> where the user must explicitly select one source. But a multi source/output
>> DPLL could be running in different modes. I believe most important is automatic
>> mode, where it tries to lock to a user-configured source priority list.
>> However, there is also freerun mode, where dpll isn't even trying to lock to
>> anything, or NCO - Numerically Controlled Oscillator mode.
>
>Yes, you are right, my focus was on "forced reference lock" mode as currently 
>this is the only mode supported by our hardware. But I'm happy to extend it to 
>other usecases.
>
>> It would be great to have ability to select DPLL modes, but also to be able to
>> configure priorities, read failure status, configure extra "features" (i.e.
>> Embedded Sync, EEC modes, Fast Lock)
>I absolutely agree on this way of improvement, and I already have some ongoing 
>work about failures/events/status change messages. I can see no stoppers for 
>creating priorities (if supported by HW) and other extra "features", but we have 
>to agree on the interface with flexibility in mind.

Great and makes perfect sense!

>
>> The sources and outputs can also have some extra features or capabilities, like:
>> - enable Embedded Sync
>
>Does it mean we want to enable or disable Embedded Sync within one protocol? Is 
>it like Time-Sensitive Networking (TSN) for Ethernet?

Well, from what I know, Embedded PPS (ePPS), Embedded Pulse Per 2 Seconds
(ePP2S) and Embedded Sync (eSync) can be either 25/75 or 75/25, which describes
a ratio of how the 'embedded pulse' is divided into HIGH and LOW states on a
pulse of higher frequency signal in which EPPS/EPP2S/ESync is embedded.

EPPS and EPP2S are rather straightforward, once an EPPS enabled input is
selected as a source, then output configured as PPS(PP2S) shall tick in the
same periods as signal "embedded" in input.
Embedded Sync (eSync) is similar but it allows for configuration of frequency
of a 'sync' signal, i.e. source is 10MHz with eSync configured as 100 HZ, where
the output configured for 100HZ could use it.

I cannot say how exactly Embedded Sync/PPS will be used, as from my perspective
this is user specific, and I am not very familiar with any standard describing
its usage.
I am working on SyncE, where either Embedded Sync or PPS is not a part of SyncE
standard, but I strongly believe that users would need to run a DPLL with
Embedded Sync/PPS enabled for some other things. And probably would love to
have SW control over the dpll.

Lets assume following simplified example:
input1 +-------------+ output1 
-------|             |---------
       |  DPLL 1     |         
input2 |             | output2 
-------|             |---------
       +-------------+         
where:
- input1 is external 10MHz with 25/75 Embedded PPS enabled,
- input2 is a fallback PPS from GNSS
user expects:
- output1 as a 10MHz with embedded sync
- output2 as a PPS
As long as input1 is selected source, output1 is synchonized with it and
output2 ticks are synchronized with ePPS.
Now the user selects input2 as a source, where outputs are unchanged,
both output2 and output1-ePPS are synchronized with input2, and output1
10MHz must be generated by DPLL.

I am trying to show example of what DPLL user might want to configure, this
would be a separated configuration of ePPS/ePP2S/eSync per source as well as
per output.
Also a DPLL itself can have explicitly disabled embedded signal processing on
its sources.

>
>> - add phase delay
>> - configure frequency (user might need to use source/output with different
>>    frequency then 1 PPS or 10MHz)
>
>Providing these modes I was thinking about the most common and widely used 
>signals in measurement equipment. So my point here is that both 1PPS and 10MHz 
>should stay as is, but another type of signal should be added, let's say 
>CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?

Sure, makes sense.

>
>> Generally, for simple DPLL designs this interface could do the job (although,
>> I still think user needs more information about the sources/outputs), but for
>> more complex ones, there should be something different, which takes care of my
>> comments regarding extra configuration needed.
>> 
>
>As I already mentioned earlier, I'm open for improvements and happy to 
>collaborate to cover other use cases of this subsystem from the very beginning 
>of development. We can even create an open github repo to share implementation 
>details together with comments if it works better for you.
>

Sure, great! I am happy to help.
I could start working on a part for extra DPLL modes and source-priorities as
automatic mode doesn't make sense without them.

Thank you,
Arkadiusz  

>> Thanks,
>> Arkadiusz
>> 
>>> +
>>> +	__DPLL_CMD_MAX,
>>> +};
>>> +#define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
>>> +
>>> +#endif /* _UAPI_LINUX_DPLL_H */
>>> -- 
>>> 2.27.0
>>>
>>>
>
>