[RFC PATCH v2 2/3] dpll: add netlink events
Vadim Fedorenko
vfedorenko at novek.ru
Tue Aug 2 17:05:03 PDT 2022
On 02.08.2022 15:02, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko at 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 at novek.ru>
>>>> Sent: Sunday, June 26, 2022 9:25 PM
>>>>>
>>>>> From: Vadim Fedorenko <vadfed at 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 at 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
More information about the linux-arm-kernel
mailing list