| 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: <15f1fb2d-35d1-918f-e7b2-6627718a179c@novek.ru>
Date: Fri, 30 Sep 2022 01:48:11 +0100
From: Vadim Fedorenko <vfedorenko@...ek.ru>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Jakub Kicinski <kuba@...nel.org>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Vadim Fedorenko <vadfed@...com>, Aya Levin <ayal@...dia.com>,
netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-clk@...r.kernel.org
Subject: Re: [RFC PATCH v2 2/3] dpll: add netlink events
On 29.09.2022 13:13, Jiri Pirko wrote:
> Sun, Jun 26, 2022 at 09:24:43PM CEST, vfedorenko@...ek.ru wrote:
>> From: Vadim Fedorenko <vadfed@...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@...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
>>
Powered by blists - more mailing lists