| 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: <DM6PR11MB4657F423D2B3B4F0799B0F019BBC9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Thu, 16 Mar 2023 13:15:59 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>, Vadim Fedorenko <vadfed@...a.com>
CC: Jakub Kicinski <kuba@...nel.org>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [PATCH RFC v6 1/6] dpll: spec: Add Netlink spec in YAML
>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Tuesday, March 14, 2023 3:44 PM
>
>Sun, Mar 12, 2023 at 03:28:02AM CET, vadfed@...a.com wrote:
>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>
>>Add a protocol spec for DPLL.
>>Add code generated from the spec.
>>
>>Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>>Signed-off-by: Michal Michalik <michal.michalik@...el.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>>---
>> Documentation/netlink/specs/dpll.yaml | 514 ++++++++++++++++++++++++++
>> drivers/dpll/dpll_nl.c | 126 +++++++
>> drivers/dpll/dpll_nl.h | 42 +++
>> include/uapi/linux/dpll.h | 196 ++++++++++
>> 4 files changed, 878 insertions(+)
>> create mode 100644 Documentation/netlink/specs/dpll.yaml
>> create mode 100644 drivers/dpll/dpll_nl.c
>> create mode 100644 drivers/dpll/dpll_nl.h
>> create mode 100644 include/uapi/linux/dpll.h
>>
>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>b/Documentation/netlink/specs/dpll.yaml
>>new file mode 100644
>>index 000000000000..03e9f0e2d3d2
>>--- /dev/null
>>+++ b/Documentation/netlink/specs/dpll.yaml
>>@@ -0,0 +1,514 @@
>>+name: dpll
>>+
>>+doc: DPLL subsystem.
>>+
>>+definitions:
>>+ -
>>+ type: const
>>+ name: temp-divider
>>+ value: 10
>>+ -
>>+ type: const
>>+ name: pin-freq-1-hz
>>+ value: 1
>>+ -
>>+ type: const
>>+ name: pin-freq-10-mhz
>>+ value: 10000000
>>+ -
>>+ type: enum
>>+ name: lock-status
>>+ doc: |
>>+ Provides information of dpll device lock status, valid values for
>>+ DPLL_A_LOCK_STATUS attribute
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: unspecified value
>>+ -
>>+ name: unlocked
>>+ doc: |
>>+ dpll was not yet locked to any valid (or is in one of modes:
>>+ DPLL_MODE_FREERUN, DPLL_MODE_NCO)
>>+ -
>>+ name: calibrating
>>+ doc: dpll is trying to lock to a valid signal
>>+ -
>>+ name: locked
>>+ doc: dpll is locked
>>+ -
>>+ name: holdover
>>+ doc: |
>>+ dpll is in holdover state - lost a valid lock or was forced by
>>+ selecting DPLL_MODE_HOLDOVER mode
>>+ render-max: true
>>+ -
>>+ type: enum
>>+ name: pin-type
>>+ doc: Enumerates types of a pin, valid values for DPLL_A_PIN_TYPE
>>attribute
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: unspecified value
>>+ -
>>+ name: mux
>>+ doc: aggregates another layer of selectable pins
>>+ -
>>+ name: ext
>>+ doc: external source
>>+ -
>>+ name: synce-eth-port
>>+ doc: ethernet port PHY's recovered clock
>>+ -
>>+ name: int-oscillator
>>+ doc: device internal oscillator
>>+ -
>>+ name: gnss
>>+ doc: GNSS recovered clock
>>+ render-max: true
>>+ -
>>+ type: enum
>>+ name: pin-state
>>+ doc: available pin modes
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: unspecified value
>>+ -
>>+ name: connected
>>+ doc: pin connected
>>+ -
>>+ name: disconnected
>>+ doc: pin disconnected
>>+ render-max: true
>>+ -
>>+ type: enum
>>+ name: pin-direction
>>+ doc: available pin direction
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: unspecified value
>>+ -
>>+ name: source
>>+ doc: pin used as a source of a signal
>>+ -
>>+ name: output
>>+ doc: pin used to output the signal
>>+ render-max: true
>>+ -
>>+ type: enum
>>+ name: mode
>
>Could you please sort the enums so they are in the same order as the
>attribute which carries them? That would also put the device and pin
>enums together.
>
Fixed.
>
>>+ doc: |
>>+ working-modes a dpll can support, differentiate if and how dpll
>>selects
>>+ one of its sources to syntonize with it
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: unspecified value
>>+ -
>>+ name: manual
>>+ doc: source can be only selected by sending a request to dpll
>>+ -
>>+ name: automatic
>>+ doc: highest prio, valid source, auto selected by dpll
>>+ -
>>+ name: holdover
>>+ doc: dpll forced into holdover mode
>>+ -
>>+ name: freerun
>>+ doc: dpll driven on system clk, no holdover available
>>+ -
>>+ name: nco
>>+ doc: dpll driven by Numerically Controlled Oscillator
>>+ render-max: true
>>+ -
>>+ type: enum
>>+ name: type
>>+ doc: type of dpll, valid values for DPLL_A_TYPE attribute
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: unspecified value
>>+ -
>>+ name: pps
>>+ doc: dpll produces Pulse-Per-Second signal
>>+ -
>>+ name: eec
>>+ doc: dpll drives the Ethernet Equipment Clock
>>+ render-max: true
>>+ -
>>+ type: enum
>>+ name: event
>>+ doc: events of dpll generic netlink family
>>+ entries:
>>+ -
>>+ name: unspec
>>+ doc: invalid event type
>>+ -
>>+ name: device-create
>>+ doc: dpll device created
>>+ -
>>+ name: device-delete
>>+ doc: dpll device deleted
>>+ -
>>+ name: device-change
>>+ doc: |
>>+ attribute of dpll device or pin changed, reason is to be found
>>with
>>+ an attribute type (DPLL_A_*) received with the event
>>+ -
>>+ type: flags
>>+ name: pin-caps
>>+ doc: define capabilities of a pin
>>+ entries:
>>+ -
>>+ name: direction-can-change
>>+ -
>>+ name: priority-can-change
>>+ -
>>+ name: state-can-change
>>+
>>+
>>+attribute-sets:
>>+ -
>>+ name: dpll
>>+ enum-name: dplla
>>+ attributes:
>>+ -
>>+ name: device
>>+ type: nest
>>+ value: 1
>>+ multi-attr: true
>>+ nested-attributes: device
>
>What is this "device" and what is it good for? Smells like some leftover
>and with the nested scheme looks quite odd.
>
No, it is nested attribute type, used when multiple devices are returned with
netlink:
- dump of device-get command where all devices are returned, each one nested
inside it:
[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
{'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id': 1}]}]
- do/dump of pin-get, in case of shared pins, each pin contains number of dpll
handles it connects with:
[{'pin': [{'device': [{'bus-name': 'pci',
'dev-name': '0000:21:00.0_0',
'id': 0,
'pin-prio': 6,
'pin-state': {'doc': 'pin connected',
'name': 'connected'}},
{'bus-name': 'pci',
'dev-name': '0000:21:00.0_1',
'id': 1,
'pin-prio': 8,
'pin-state': {'doc': 'pin connected',
'name': 'connected'}}],
'pin-direction': {'doc': 'pin used as a source of a signal',
'name': 'source'},
'pin-frequency': 1,
'pin-frequency-supported': [1, 10000000],
'pin-idx': 0,
...
>
>>+ -
>>+ name: id
>>+ type: u32
>>+ -
>>+ name: dev-name
>>+ type: string
>>+ -
>>+ name: bus-name
>>+ type: string
>>+ -
>>+ name: mode
>>+ type: u8
>>+ enum: mode
>>+ -
>>+ name: mode-supported
>>+ type: u8
>>+ enum: mode
>>+ multi-attr: true
>>+ -
>>+ name: source-pin-idx
>>+ type: u32
>>+ -
>>+ name: lock-status
>>+ type: u8
>>+ enum: lock-status
>>+ -
>>+ name: temp
>
>Could you put some comment regarding the divider here?
>
Sure, fixed.
>
>>+ type: s32
>>+ -
>>+ name: clock-id
>>+ type: u64
>>+ -
>>+ name: type
>>+ type: u8
>>+ enum: type
>>+ -
>>+ name: pin
>>+ type: nest
>>+ multi-attr: true
>>+ nested-attributes: pin
>>+ value: 12
>>+ -
>>+ name: pin-idx
>>+ type: u32
>>+ -
>>+ name: pin-description
>>+ type: string
>>+ -
>>+ name: pin-type
>>+ type: u8
>>+ enum: pin-type
>>+ -
>>+ name: pin-direction
>>+ type: u8
>>+ enum: pin-direction
>>+ -
>>+ name: pin-frequency
>>+ type: u32
>>+ -
>>+ name: pin-frequency-supported
>>+ type: u32
>>+ multi-attr: true
>>+ -
>>+ name: pin-any-frequency-min
>>+ type: u32
>>+ -
>>+ name: pin-any-frequency-max
>>+ type: u32
>
>These 2 attrs somehow overlap with pin-frequency-supported,
>which is a bit confusing, I think that pin-frequency-supported
>should carry all supported frequencies. How about to have something
>like:
> name: pin-frequency-supported
> type: nest
> multi-attr: true
> nested-attributes: pin-frequency-range
>
>and then:
> name: pin-frequency-range
> subset-of: dpll
> attributes:
> -
> name: pin-frequency-min
> type: u32
> -
> name: pin-frequency-max
> type: u32
> doc: should be put only to specify range when value differs
> from pin-frequency-min
>
>That could carry list of frequencies and ranges, in this case multiple
>ones if possibly needed.
>
Sure
>
>
>>+ -
>>+ name: pin-prio
>>+ type: u32
>>+ -
>>+ name: pin-state
>>+ type: u8
>>+ enum: pin-state
>>+ -
>>+ name: pin-parent
>>+ type: nest
>>+ multi-attr: true
>>+ nested-attributes: pin
>>+ value: 23
>
>Value 23? What's this?
>You have it specified for some attrs all over the place.
>What is the reason for it?
>
Actually this particular one is not needed (also value: 12 on pin above),
I will remove those.
But the others you are refering to (the ones in nested attribute list),
are required because of cli.py parser issue, maybe Kuba knows a better way to
prevent the issue?
Basically, without those values, cli.py brakes on parsing responses, after
every "jump" to nested attribute list it is assigning first attribute there
with value=0, thus there is a need to assign a proper value, same as it is on
'main' attribute list.
>
>>+ -
>>+ name: pin-parent-idx
>>+ type: u32
>>+ -
>>+ name: pin-rclk-device
>>+ type: string
>>+ -
>>+ name: pin-dpll-caps
>>+ type: u32
>>+ -
>>+ name: device
>>+ subset-of: dpll
>>+ attributes:
>>+ -
>>+ name: id
>>+ type: u32
>>+ value: 2
>>+ -
>>+ name: dev-name
>>+ type: string
>>+ -
>>+ name: bus-name
>>+ type: string
>>+ -
>>+ name: mode
>>+ type: u8
>>+ enum: mode
>>+ -
>>+ name: mode-supported
>>+ type: u8
>>+ enum: mode
>>+ multi-attr: true
>>+ -
>>+ name: source-pin-idx
>>+ type: u32
>>+ -
>>+ name: lock-status
>>+ type: u8
>>+ enum: lock-status
>>+ -
>>+ name: temp
>>+ type: s32
>>+ -
>>+ name: clock-id
>>+ type: u64
>>+ -
>>+ name: type
>>+ type: u8
>>+ enum: type
>>+ -
>>+ name: pin
>>+ type: nest
>>+ value: 12
>>+ multi-attr: true
>>+ nested-attributes: pin
>
>This does not belong here.
>
What do you mean?
With device-get 'do' request the list of pins connected to the dpll is
returned, each pin is nested in this attribute.
This is required by parser to work.
>
>>+ -
>>+ name: pin-prio
>>+ type: u32
>>+ value: 21
>>+ -
>>+ name: pin-state
>>+ type: u8
>>+ enum: pin-state
>>+ -
>>+ name: pin-dpll-caps
>>+ type: u32
>>+ value: 26
>
>All these 3 do not belong here are well.
>
Same as above explanation.
>
>
>>+ -
>>+ name: pin
>>+ subset-of: dpll
>>+ attributes:
>>+ -
>>+ name: device
>>+ type: nest
>>+ value: 1
>>+ multi-attr: true
>>+ nested-attributes: device
>>+ -
>>+ name: pin-idx
>>+ type: u32
>>+ value: 13
>>+ -
>>+ name: pin-description
>>+ type: string
>>+ -
>>+ name: pin-type
>>+ type: u8
>>+ enum: pin-type
>>+ -
>>+ name: pin-direction
>>+ type: u8
>>+ enum: pin-direction
>>+ -
>>+ name: pin-frequency
>>+ type: u32
>>+ -
>>+ name: pin-frequency-supported
>>+ type: u32
>>+ multi-attr: true
>>+ -
>>+ name: pin-any-frequency-min
>>+ type: u32
>>+ -
>>+ name: pin-any-frequency-max
>>+ type: u32
>>+ -
>>+ name: pin-prio
>>+ type: u32
>>+ -
>>+ name: pin-state
>>+ type: u8
>>+ enum: pin-state
>>+ -
>>+ name: pin-parent
>>+ type: nest
>>+ multi-attr: true
>
>Multiple parents? How is that supposed to work?
>
As we have agreed, MUXed pins can have multiple parents.
In our case:
/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-get --json '{"id": 0, "pin-idx":13}'
{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
{'bus-name': 'pci',
'dev-name': '0000:21:00.0_1',
'id': 1}],
'pin-description': '0000:21:00.0',
'pin-direction': {'doc': 'pin used as a source of a signal',
'name': 'source'},
'pin-idx': 13,
'pin-parent': [{'pin-parent-idx': 2,
'pin-state': {'doc': 'pin disconnected',
'name': 'disconnected'}},
{'pin-parent-idx': 3,
'pin-state': {'doc': 'pin disconnected',
'name': 'disconnected'}}],
'pin-rclk-device': '0000:21:00.0',
'pin-type': {'doc': "ethernet port PHY's recovered clock",
'name': 'synce-eth-port'}}]}
>
>>+ nested-attributes: pin-parent
>>+ value: 23
>>+ -
>>+ name: pin-rclk-device
>>+ type: string
>>+ value: 25
>>+ -
>>+ name: pin-dpll-caps
>>+ type: u32
>
>Missing "enum: "
>
It is actually a bitmask, this is why didn't set as enum, with enum type
parser won't parse it.
>
>>+ -
>>+ name: pin-parent
>>+ subset-of: dpll
>>+ attributes:
>>+ -
>>+ name: pin-state
>>+ type: u8
>>+ value: 22
>>+ enum: pin-state
>>+ -
>>+ name: pin-parent-idx
>>+ type: u32
>>+ value: 24
>>+ -
>>+ name: pin-rclk-device
>>+ type: string
>
>Yeah, as I wrote in the other email, this really smells to
>have like a simple string like this. What is it supposed to be?
>
Yes, let's discuss there.
>
>>+
>>+
>>+operations:
>>+ list:
>>+ -
>>+ name: unspec
>>+ doc: unused
>>+
>>+ -
>>+ name: device-get
>>+ doc: |
>>+ Get list of DPLL devices (dump) or attributes of a single dpll
>device
>>+ attribute-set: dpll
>
>Shouldn't this be "device"?
>
It would brake the parser, again I hope Jakub Kicinski could take a look on
this.
>
>>+ flags: [ admin-perm ]
>>+
>>+ do:
>>+ pre: dpll-pre-doit
>>+ post: dpll-post-doit
>>+ request:
>>+ attributes:
>>+ - id
>>+ - bus-name
>>+ - dev-name
>>+ reply:
>>+ attributes:
>>+ - device
>>+
>>+ dump:
>>+ pre: dpll-pre-dumpit
>>+ post: dpll-post-dumpit
>>+ reply:
>>+ attributes:
>>+ - device
>>+
>>+ -
>>+ name: device-set
>>+ doc: Set attributes for a DPLL device
>>+ attribute-set: dpll
>
>"device" here as well?
>
Same as above.
>
>>+ flags: [ admin-perm ]
>>+
>>+ do:
>>+ pre: dpll-pre-doit
>>+ post: dpll-post-doit
>>+ request:
>>+ attributes:
>>+ - id
>>+ - bus-name
>>+ - dev-name
>>+ - mode
>
>Hmm, shouldn't source-pin-index be here as well?
No, there is no set for this.
For manual mode user selects the pin by setting enabled state on the one
he needs to recover signal from.
source-pin-index is read only, returns active source.
>
>>+
>>+ -
>>+ name: pin-get
>>+ doc: |
>>+ Get list of pins and its attributes.
>>+ - dump request without any attributes given - list all the pins
>in the system
>>+ - dump request with target dpll - list all the pins registered
>with a given dpll device
>>+ - do request with target dpll and target pin - single pin
>attributes
>>+ attribute-set: dpll
>
>"pin"?
>
Same case as with dpll/device above.
>
>>+ flags: [ admin-perm ]
>>+
>>+ do:
>>+ pre: dpll-pin-pre-doit
>>+ post: dpll-pin-post-doit
>>+ request:
>>+ attributes:
>>+ - id
>>+ - bus-name
>>+ - dev-name
>>+ - pin-idx
>>+ reply:
>>+ attributes:
>>+ - pin
>>+
>>+ dump:
>>+ pre: dpll-pin-pre-dumpit
>>+ post: dpll-pin-post-dumpit
>>+ request:
>>+ attributes:
>>+ - id
>>+ - bus-name
>>+ - dev-name
>>+ reply:
>>+ attributes:
>>+ - pin
>>+
>>+ -
>>+ name: pin-set
>>+ doc: Set attributes of a target pin
>>+ attribute-set: dpll
>
>"pin"?
>
Same case as with dpll/device above.
>
>>+ flags: [ admin-perm ]
>>+
>>+ do:
>>+ pre: dpll-pin-pre-doit
>>+ post: dpll-pin-post-doit
>>+ request:
>>+ attributes:
>>+ - id
>>+ - bus-name
>>+ - dev-name
>>+ - pin-idx
>>+ - pin-frequency
>>+ - pin-direction
>>+ - pin-prio
>>+ - pin-parent-idx
>>+ - pin-state
>>+
>>+mcast-groups:
>>+ list:
>>+ -
>>+ name: monitor
>>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
>>new file mode 100644
>>index 000000000000..099d1e30ca7c
>>--- /dev/null
>>+++ b/drivers/dpll/dpll_nl.c
>>@@ -0,0 +1,126 @@
>>+// SPDX-License-Identifier: BSD-3-Clause
>>+/* Do not edit directly, auto-generated from: */
>>+/* Documentation/netlink/specs/dpll.yaml */
>>+/* YNL-GEN kernel source */
>>+
>>+#include <net/netlink.h>
>>+#include <net/genetlink.h>
>>+
>>+#include "dpll_nl.h"
>>+
>>+#include <linux/dpll.h>
>>+
>>+/* DPLL_CMD_DEVICE_GET - do */
>>+static const struct nla_policy dpll_device_get_nl_policy[DPLL_A_BUS_NAME
>>+ 1] = {
>>+ [DPLL_A_ID] = { .type = NLA_U32, },
>>+ [DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+};
>>+
>>+/* DPLL_CMD_DEVICE_SET - do */
>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE + 1]
>>= {
>>+ [DPLL_A_ID] = { .type = NLA_U32, },
>>+ [DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>
>Hmm, any idea why the generator does not put define name
>here instead of "5"?
>
Not really, it probably needs a fix for this.
>
>>+};
>>+
>>+/* DPLL_CMD_PIN_GET - do */
>>+static const struct nla_policy dpll_pin_get_do_nl_policy[DPLL_A_PIN_IDX +
>>1] = {
>>+ [DPLL_A_ID] = { .type = NLA_U32, },
>>+ [DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>>+};
>>+
>>+/* DPLL_CMD_PIN_GET - dump */
>>+static const struct nla_policy
>>dpll_pin_get_dump_nl_policy[DPLL_A_BUS_NAME + 1] = {
>>+ [DPLL_A_ID] = { .type = NLA_U32, },
>>+ [DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+};
>>+
>>+/* DPLL_CMD_PIN_SET - do */
>>+static const struct nla_policy
>>dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_IDX + 1] = {
>>+ [DPLL_A_ID] = { .type = NLA_U32, },
>>+ [DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+ [DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>>+ [DPLL_A_PIN_FREQUENCY] = { .type = NLA_U32, },
>
>4.3GHz would be the limit, isn't it future proof to make this rather u64?
>
Sure, fixed.
>
>>+ [DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2),
>>+ [DPLL_A_PIN_PRIO] = { .type = NLA_U32, },
>>+ [DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, },
>
>This is a bit odd. The size is DPLL_A_PIN_PARENT_IDX + 1, yet PIN_STATE
>is last. I found that the order is not according to the enum in the yaml
>operation definition. Perhaps the generator can sort this?
>
Fixed. This related to the order on list of attributes expected on ops
definition.
>
>>+ [DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 2),
>>+};
>>+
>>+/* Ops table for dpll */
>>+static const struct genl_split_ops dpll_nl_ops[6] = {
>>+ {
>>+ .cmd = DPLL_CMD_DEVICE_GET,
>>+ .pre_doit = dpll_pre_doit,
>>+ .doit = dpll_nl_device_get_doit,
>>+ .post_doit = dpll_post_doit,
>>+ .policy = dpll_device_get_nl_policy,
>>+ .maxattr = DPLL_A_BUS_NAME,
>>+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+ },
>>+ {
>>+ .cmd = DPLL_CMD_DEVICE_GET,
>>+ .start = dpll_pre_dumpit,
>>+ .dumpit = dpll_nl_device_get_dumpit,
>>+ .done = dpll_post_dumpit,
>>+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>>+ },
>>+ {
>>+ .cmd = DPLL_CMD_DEVICE_SET,
>>+ .pre_doit = dpll_pre_doit,
>>+ .doit = dpll_nl_device_set_doit,
>>+ .post_doit = dpll_post_doit,
>>+ .policy = dpll_device_set_nl_policy,
>>+ .maxattr = DPLL_A_MODE,
>>+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+ },
>>+ {
>>+ .cmd = DPLL_CMD_PIN_GET,
>>+ .pre_doit = dpll_pin_pre_doit,
>>+ .doit = dpll_nl_pin_get_doit,
>>+ .post_doit = dpll_pin_post_doit,
>>+ .policy = dpll_pin_get_do_nl_policy,
>>+ .maxattr = DPLL_A_PIN_IDX,
>>+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+ },
>>+ {
>>+ .cmd = DPLL_CMD_PIN_GET,
>>+ .start = dpll_pin_pre_dumpit,
>>+ .dumpit = dpll_nl_pin_get_dumpit,
>>+ .done = dpll_pin_post_dumpit,
>>+ .policy = dpll_pin_get_dump_nl_policy,
>>+ .maxattr = DPLL_A_BUS_NAME,
>>+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>>+ },
>>+ {
>>+ .cmd = DPLL_CMD_PIN_SET,
>>+ .pre_doit = dpll_pin_pre_doit,
>>+ .doit = dpll_nl_pin_set_doit,
>>+ .post_doit = dpll_pin_post_doit,
>>+ .policy = dpll_pin_set_nl_policy,
>>+ .maxattr = DPLL_A_PIN_PARENT_IDX,
>>+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+ },
>>+};
>>+
>>+static const struct genl_multicast_group dpll_nl_mcgrps[] = {
>>+ [DPLL_NLGRP_MONITOR] = { "monitor", },
>>+};
>>+
>>+struct genl_family dpll_nl_family __ro_after_init = {
>>+ .name = DPLL_FAMILY_NAME,
>>+ .version = DPLL_FAMILY_VERSION,
>>+ .netnsok = true,
>>+ .parallel_ops = true,
>>+ .module = THIS_MODULE,
>>+ .split_ops = dpll_nl_ops,
>>+ .n_split_ops = ARRAY_SIZE(dpll_nl_ops),
>>+ .mcgrps = dpll_nl_mcgrps,
>>+ .n_mcgrps = ARRAY_SIZE(dpll_nl_mcgrps),
>>+};
>>diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h
>>new file mode 100644
>>index 000000000000..3a354dfb9a31
>>--- /dev/null
>>+++ b/drivers/dpll/dpll_nl.h
>>@@ -0,0 +1,42 @@
>>+/* SPDX-License-Identifier: BSD-3-Clause */
>>+/* Do not edit directly, auto-generated from: */
>>+/* Documentation/netlink/specs/dpll.yaml */
>>+/* YNL-GEN kernel header */
>>+
>>+#ifndef _LINUX_DPLL_GEN_H
>>+#define _LINUX_DPLL_GEN_H
>>+
>>+#include <net/netlink.h>
>>+#include <net/genetlink.h>
>>+
>>+#include <linux/dpll.h>
>>+
>>+int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>+ struct genl_info *info);
>>+int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff
>*skb,
>>+ struct genl_info *info);
>>+void
>>+dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>+ struct genl_info *info);
>>+void
>>+dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>+ struct genl_info *info);
>>+int dpll_pre_dumpit(struct netlink_callback *cb);
>>+int dpll_pin_pre_dumpit(struct netlink_callback *cb);
>>+int dpll_post_dumpit(struct netlink_callback *cb);
>>+int dpll_pin_post_dumpit(struct netlink_callback *cb);
>>+
>>+int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info);
>>+int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>netlink_callback *cb);
>>+int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info);
>>+int dpll_nl_pin_get_doit(struct sk_buff *skb, struct genl_info *info);
>>+int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback
>*cb);
>>+int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info);
>>+
>>+enum {
>>+ DPLL_NLGRP_MONITOR,
>>+};
>>+
>>+extern struct genl_family dpll_nl_family;
>>+
>>+#endif /* _LINUX_DPLL_GEN_H */
>>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>>new file mode 100644
>>index 000000000000..ece6fe685d08
>>--- /dev/null
>>+++ b/include/uapi/linux/dpll.h
>>@@ -0,0 +1,196 @@
>>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>+/* Do not edit directly, auto-generated from: */
>>+/* Documentation/netlink/specs/dpll.yaml */
>>+/* YNL-GEN uapi header */
>>+
>>+#ifndef _UAPI_LINUX_DPLL_H
>>+#define _UAPI_LINUX_DPLL_H
>>+
>>+#define DPLL_FAMILY_NAME "dpll"
>>+#define DPLL_FAMILY_VERSION 1
>>+
>>+#define DPLL_TEMP_DIVIDER 10
>
>Some comment to this define would be nice.
>
I added to the spec, but it is not generated by the regen script.
>
>>+#define DPLL_PIN_FREQ_1_HZ 1
>>+#define DPLL_PIN_FREQ_10_MHZ 10000000
>
>Have this as enum. Spell out "frequency" to be consistent with the
>related attribute name.
>
Sure, fixed.
>
>>+
>>+/**
>>+ * enum dpll_lock_status - Provides information of dpll device lock
>>status,
>>+ * valid values for DPLL_A_LOCK_STATUS attribute
>>+ * @DPLL_LOCK_STATUS_UNSPEC: unspecified value
>>+ * @DPLL_LOCK_STATUS_UNLOCKED: dpll was not yet locked to any valid (or
>is in
>
>"any valid source" perhaps?
>
Yes, fixed.
>
>>+ * one of modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
>>+ * @DPLL_LOCK_STATUS_CALIBRATING: dpll is trying to lock to a valid
>>signal
>>+ * @DPLL_LOCK_STATUS_LOCKED: dpll is locked
>>+ * @DPLL_LOCK_STATUS_HOLDOVER: dpll is in holdover state - lost a valid
>>lock or
>>+ * was forced by selecting DPLL_MODE_HOLDOVER mode
>>+ */
>>+enum dpll_lock_status {
>>+ DPLL_LOCK_STATUS_UNSPEC,
>>+ DPLL_LOCK_STATUS_UNLOCKED,
>>+ DPLL_LOCK_STATUS_CALIBRATING,
>>+ DPLL_LOCK_STATUS_LOCKED,
>>+ DPLL_LOCK_STATUS_HOLDOVER,
>>+
>>+ __DPLL_LOCK_STATUS_MAX,
>>+ DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_type - Enumerates types of a pin, valid values for
>>+ * DPLL_A_PIN_TYPE attribute
>>+ * @DPLL_PIN_TYPE_UNSPEC: unspecified value
>>+ * @DPLL_PIN_TYPE_MUX: aggregates another layer of selectable pins
>>+ * @DPLL_PIN_TYPE_EXT: external source
>>+ * @DPLL_PIN_TYPE_SYNCE_ETH_PORT: ethernet port PHY's recovered clock
>>+ * @DPLL_PIN_TYPE_INT_OSCILLATOR: device internal oscillator
>>+ * @DPLL_PIN_TYPE_GNSS: GNSS recovered clock
>>+ */
>>+enum dpll_pin_type {
>>+ DPLL_PIN_TYPE_UNSPEC,
>>+ DPLL_PIN_TYPE_MUX,
>>+ DPLL_PIN_TYPE_EXT,
>>+ DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>+ DPLL_PIN_TYPE_INT_OSCILLATOR,
>>+ DPLL_PIN_TYPE_GNSS,
>>+
>>+ __DPLL_PIN_TYPE_MAX,
>>+ DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_state - available pin modes
>
>For some of the enums, you say for what attribute it serves as a value
>set, for some you don't. Please unify the approach. I think it is
>valuable to say that for every enum.
>
Sure, fixed.
>
>>+ * @DPLL_PIN_STATE_UNSPEC: unspecified value
>>+ * @DPLL_PIN_STATE_CONNECTED: pin connected
>>+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected
>>+ */
>>+enum dpll_pin_state {
>>+ DPLL_PIN_STATE_UNSPEC,
>>+ DPLL_PIN_STATE_CONNECTED,
>>+ DPLL_PIN_STATE_DISCONNECTED,
>>+
>>+ __DPLL_PIN_STATE_MAX,
>>+ DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_direction - available pin direction
>>+ * @DPLL_PIN_DIRECTION_UNSPEC: unspecified value
>>+ * @DPLL_PIN_DIRECTION_SOURCE: pin used as a source of a signal
>>+ * @DPLL_PIN_DIRECTION_OUTPUT: pin used to output the signal
>>+ */
>>+enum dpll_pin_direction {
>>+ DPLL_PIN_DIRECTION_UNSPEC,
>>+ DPLL_PIN_DIRECTION_SOURCE,
>>+ DPLL_PIN_DIRECTION_OUTPUT,
>>+
>>+ __DPLL_PIN_DIRECTION_MAX,
>>+ DPLL_PIN_DIRECTION_MAX = (__DPLL_PIN_DIRECTION_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_mode - working-modes a dpll can support, differentiate if
>>and how
>>+ * dpll selects one of its sources to syntonize with it
>>+ * @DPLL_MODE_UNSPEC: unspecified value
>>+ * @DPLL_MODE_MANUAL: source can be only selected by sending a request to
>>dpll
>>+ * @DPLL_MODE_AUTOMATIC: highest prio, valid source, auto selected by
>>dpll
>>+ * @DPLL_MODE_HOLDOVER: dpll forced into holdover mode
>>+ * @DPLL_MODE_FREERUN: dpll driven on system clk, no holdover available
>>+ * @DPLL_MODE_NCO: dpll driven by Numerically Controlled Oscillator
>>+ */
>>+enum dpll_mode {
>>+ DPLL_MODE_UNSPEC,
>>+ DPLL_MODE_MANUAL,
>>+ DPLL_MODE_AUTOMATIC,
>>+ DPLL_MODE_HOLDOVER,
>>+ DPLL_MODE_FREERUN,
>>+ DPLL_MODE_NCO,
>>+
>>+ __DPLL_MODE_MAX,
>>+ DPLL_MODE_MAX = (__DPLL_MODE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_type - type of dpll, valid values for DPLL_A_TYPE attribute
>>+ * @DPLL_TYPE_UNSPEC: unspecified value
>>+ * @DPLL_TYPE_PPS: dpll produces Pulse-Per-Second signal
>>+ * @DPLL_TYPE_EEC: dpll drives the Ethernet Equipment Clock
>>+ */
>>+enum dpll_type {
>>+ DPLL_TYPE_UNSPEC,
>>+ DPLL_TYPE_PPS,
>>+ DPLL_TYPE_EEC,
>>+
>>+ __DPLL_TYPE_MAX,
>>+ DPLL_TYPE_MAX = (__DPLL_TYPE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_event - events of dpll generic netlink family
>>+ * @DPLL_EVENT_UNSPEC: invalid event type
>>+ * @DPLL_EVENT_DEVICE_CREATE: dpll device created
>>+ * @DPLL_EVENT_DEVICE_DELETE: dpll device deleted
>>+ * @DPLL_EVENT_DEVICE_CHANGE: attribute of dpll device or pin changed,
>>reason
>>+ * is to be found with an attribute type (DPLL_A_*) received with the
>>event
>>+ */
>>+enum dpll_event {
>>+ DPLL_EVENT_UNSPEC,
>>+ DPLL_EVENT_DEVICE_CREATE,
>>+ DPLL_EVENT_DEVICE_DELETE,
>>+ DPLL_EVENT_DEVICE_CHANGE,
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_caps - define capabilities of a pin
>>+ */
>>+enum dpll_pin_caps {
>>+ DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE = 1,
>>+ DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE = 2,
>>+ DPLL_PIN_CAPS_STATE_CAN_CHANGE = 4,
>>+};
>>+
>>+enum dplla {
>>+ DPLL_A_DEVICE = 1,
>>+ DPLL_A_ID,
>>+ DPLL_A_DEV_NAME,
>>+ DPLL_A_BUS_NAME,
>>+ DPLL_A_MODE,
>>+ DPLL_A_MODE_SUPPORTED,
>>+ DPLL_A_SOURCE_PIN_IDX,
>>+ DPLL_A_LOCK_STATUS,
>>+ DPLL_A_TEMP,
>>+ DPLL_A_CLOCK_ID,
>>+ DPLL_A_TYPE,
>>+ DPLL_A_PIN,
>>+ DPLL_A_PIN_IDX,
>>+ DPLL_A_PIN_DESCRIPTION,
>
>I commented this name in the other email. This does not describe
>anything. It is a label on a connector, isn't it? Why don't to call it
>"label"?
>
Sure, fixed.
>
>>+ DPLL_A_PIN_TYPE,
>>+ DPLL_A_PIN_DIRECTION,
>>+ DPLL_A_PIN_FREQUENCY,
>>+ DPLL_A_PIN_FREQUENCY_SUPPORTED,
>>+ DPLL_A_PIN_ANY_FREQUENCY_MIN,
>>+ DPLL_A_PIN_ANY_FREQUENCY_MAX,
>
>Drop "any". Not good for anything.
>
Sure, fixed.
>
>>+ DPLL_A_PIN_PRIO,
>>+ DPLL_A_PIN_STATE,
>>+ DPLL_A_PIN_PARENT,
>>+ DPLL_A_PIN_PARENT_IDX,
>>+ DPLL_A_PIN_RCLK_DEVICE,
>>+ DPLL_A_PIN_DPLL_CAPS,
>
>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>enum name.
Sure, fixed.
>
>>+
>>+ __DPLL_A_MAX,
>>+ DPLL_A_MAX = (__DPLL_A_MAX - 1)
>>+};
>>+
>>+enum {
>>+ DPLL_CMD_UNSPEC,
>>+ DPLL_CMD_DEVICE_GET,
>>+ DPLL_CMD_DEVICE_SET,
>>+ DPLL_CMD_PIN_GET,
>>+ DPLL_CMD_PIN_SET,
>>+
>>+ __DPLL_CMD_MAX,
>>+ DPLL_CMD_MAX = (__DPLL_CMD_MAX - 1)
>>+};
>>+
>>+#define DPLL_MCGRP_MONITOR "monitor"
>>+
>>+#endif /* _UAPI_LINUX_DPLL_H */
>>--
>>2.34.1
>>
Powered by blists - more mailing lists