Re: [RFC PATCH v3 4/6] dpll: get source/output name

[Jiri Pirko <jiri@resnulli.us>]

Mon, Oct 10, 2022 at 09:55:57PM CEST, vfedorenko@novek.ru wrote:
>On 10.10.2022 10:45, Jiri Pirko wrote:
>> Mon, Oct 10, 2022 at 03:18:02AM CEST, vfedorenko@novek.ru wrote:
>> > From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> > 
>> > Dump names of sources and outputs in response to DPLL_CMD_DEVICE_GET dump
>> > request.
>> > 
>> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> > ---
>> > drivers/dpll/dpll_netlink.c | 24 ++++++++++++++++++++++++
>> > include/linux/dpll.h        |  2 ++
>> > include/uapi/linux/dpll.h   |  2 ++
>> > 3 files changed, 28 insertions(+)
>> > 
>> > diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>> > index a5779871537a..e3604c10b59e 100644
>> > --- a/drivers/dpll/dpll_netlink.c
>> > +++ b/drivers/dpll/dpll_netlink.c
>> > @@ -31,12 +31,16 @@ 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 },
>> > +	[DPLLA_SOURCE_NAME]	= { .type = NLA_STRING,
>> > +				    .len = DPLL_NAME_LENGTH },
>> > };
>> > 
>> > 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 },
>> > +	[DPLLA_OUTPUT_NAME]	= { .type = NLA_STRING,
>> > +				    .len = DPLL_NAME_LENGTH },
>> > };
>> > 
>> > static const struct nla_policy dpll_genl_set_src_select_mode_policy[] = {
>> > @@ -100,6 +104,7 @@ static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
>> > {
>> > 	int i, ret = 0, type, prio;
>> > 	struct nlattr *src_attr;
>> > +	const char *name;
>> > 
>> > 	for (i = 0; i < dpll->sources_count; i++) {
>> > 		src_attr = nla_nest_start(msg, DPLLA_SOURCE);
>> > @@ -132,6 +137,15 @@ static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
>> > 				break;
>> > 			}
>> > 		}
>> > +		if (dpll->ops->get_source_name) {
>> > +			name = dpll->ops->get_source_name(dpll, i);
>> > +			if (name && nla_put_string(msg, DPLLA_SOURCE_NAME,
>> > +						   name)) {
>> > +				nla_nest_cancel(msg, src_attr);
>> > +				ret = -EMSGSIZE;
>> > +				break;
>> > +			}
>> > +		}
>> > 		nla_nest_end(msg, src_attr);
>> > 	}
>> > 
>> > @@ -143,6 +157,7 @@ static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
>> > {
>> > 	struct nlattr *out_attr;
>> > 	int i, ret = 0, type;
>> > +	const char *name;
>> > 
>> > 	for (i = 0; i < dpll->outputs_count; i++) {
>> > 		out_attr = nla_nest_start(msg, DPLLA_OUTPUT);
>> > @@ -167,6 +182,15 @@ static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
>> > 			}
>> > 			ret = 0;
>> > 		}
>> > +		if (dpll->ops->get_output_name) {
>> > +			name = dpll->ops->get_output_name(dpll, i);
>> > +			if (name && nla_put_string(msg, DPLLA_OUTPUT_NAME,
>> > +						   name)) {
>> > +				nla_nest_cancel(msg, out_attr);
>> > +				ret = -EMSGSIZE;
>> > +				break;
>> > +			}
>> > +		}
>> > 		nla_nest_end(msg, out_attr);
>> > 	}
>> > 
>> > diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>> > index 3fe957a06b90..2f4964dc28f0 100644
>> > --- a/include/linux/dpll.h
>> > +++ b/include/linux/dpll.h
>> > @@ -23,6 +23,8 @@ struct dpll_device_ops {
>> > 	int (*set_output_type)(struct dpll_device *dpll, int id, int val);
>> > 	int (*set_source_select_mode)(struct dpll_device *dpll, int mode);
>> > 	int (*set_source_prio)(struct dpll_device *dpll, int id, int prio);
>> > +	const char *(*get_source_name)(struct dpll_device *dpll, int id);
>> > +	const char *(*get_output_name)(struct dpll_device *dpll, int id);
>> 
>> Hmm, why you exactly need the name for?
>> 
>As with device name, user-space app can use source/output name to easily
>select one using configuration value, for example.

Can you give me some examples? I can't imagine how it can be named
differently than containing the "type string". My point is to avoid
drivers to come up with original names that basically have the same
meaning creating a mess.

Why index (which is predictable and could be used in scripts) isn't
enough?

Or, alternatively, we can leave the "resolve" up to the userspace app.
Example:
index 0
  type 1PPS
index 1
  type 10MHZ
index 2
  type SyncE
  ifindex 20
index 3
  type SyncE
  ifindex 30

Now when user does:
1) dpll X source set index 0
   could be equivalent to
   dpll X source set 1pps
     dpll app would do dump of all sources and if there is only one of
     type 1pps, it will resolve it to index 0 which it is going to send
     via ATTR_INDEX to kernel.
2) dpll X source set index 2
   could be equivalent to
   dpll X source set eth1
     dpll app would use rtnetlink to get ifindex 20 for eth1. Then it
     will do dump of all sources and see if there is SyncE source with
     ifindex 30. This will resolve to index 2 which it is going to send
     via ATTR_INDEX to kernel.

>> 
>> > };
>> > 
>> > struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, const char *name,
>> > diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>> > index f6b674e5cf01..8782d3425aae 100644
>> > --- a/include/uapi/linux/dpll.h
>> > +++ b/include/uapi/linux/dpll.h
>> > @@ -26,11 +26,13 @@ enum dpll_genl_attr {
>> > 	DPLLA_SOURCE,
>> > 	DPLLA_SOURCE_ID,
>> > 	DPLLA_SOURCE_TYPE,
>> > +	DPLLA_SOURCE_NAME,
>> > 	DPLLA_SOURCE_SUPPORTED,
>> > 	DPLLA_SOURCE_PRIO,
>> > 	DPLLA_OUTPUT,
>> > 	DPLLA_OUTPUT_ID,
>> > 	DPLLA_OUTPUT_TYPE,
>> > +	DPLLA_OUTPUT_NAME,
>> > 	DPLLA_OUTPUT_SUPPORTED,
>> > 	DPLLA_STATUS,
>> > 	DPLLA_TEMP,
>> > -- 
>> > 2.27.0
>> > 
>