| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR11MB4657C1830DACC5EB4CD98B789BB49@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 24 Jun 2022 17:36:21 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Vadim Fedorenko <vfedorenko@...ek.ru>,
Jakub Kicinski <kuba@...nel.org>
CC: Vadim Fedorenko <vadfed@...com>, Aya Levin <ayal@...dia.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Jonathan Lemon <jonathan.lemon@...il.com>
Subject: RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@...ek.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
>>>
>>>
>
>
Powered by blists - more mailing lists