| Message ID | 20231006114101.1608796-3-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpll: add phase-offset and phase-adjust | expand |
Fri, Oct 06, 2023 at 01:40:58PM CEST, arkadiusz.kubalewski@intel.com wrote: >Add attributes for providing the user with: >- measurement of signals phase offset between pin and dpll >- ability to adjust the phase of pin signal > >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- > Documentation/netlink/specs/dpll.yaml | 33 ++++++++++++++++++++++++++- > drivers/dpll/dpll_nl.c | 8 ++++--- > drivers/dpll/dpll_nl.h | 2 +- > include/uapi/linux/dpll.h | 8 ++++++- > 4 files changed, 45 insertions(+), 6 deletions(-) > >diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml >index 8b86b28b47a6..dc057494101f 100644 >--- a/Documentation/netlink/specs/dpll.yaml >+++ b/Documentation/netlink/specs/dpll.yaml >@@ -1,7 +1,7 @@ > # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) > > name: dpll >- >+version: 2 I'm confused. Didn't you say you'll remove this? If not, my question from v1 still stands.
On Fri, 6 Oct 2023 14:30:00 +0200 Jiri Pirko wrote: > >+version: 2 > > I'm confused. Didn't you say you'll remove this? If not, my question > from v1 still stands. Perhaps we should dis-allow setting version in non-genetlink-legacy specs? I thought it may be a useful thing to someone, at some point, but so far the scoreboard is: legit uses: 0, confused uses: 1 :S Thoughts?
Fri, Oct 06, 2023 at 04:55:36PM CEST, kuba@kernel.org wrote: >On Fri, 6 Oct 2023 14:30:00 +0200 Jiri Pirko wrote: >> >+version: 2 >> >> I'm confused. Didn't you say you'll remove this? If not, my question >> from v1 still stands. > >Perhaps we should dis-allow setting version in non-genetlink-legacy >specs? I thought it may be a useful thing to someone, at some point, >but so far the scoreboard is: legit uses: 0, confused uses: 1 :S > >Thoughts? I don't know what the meaning of version is. I just never saw that being touched. Is there any semantics documented for it? Kuba, any opinion?
On Fri, 6 Oct 2023 18:53:04 +0200 Jiri Pirko wrote: > Fri, Oct 06, 2023 at 04:55:36PM CEST, kuba@kernel.org wrote: > >> I'm confused. Didn't you say you'll remove this? If not, my question > >> from v1 still stands. > > > >Perhaps we should dis-allow setting version in non-genetlink-legacy > >specs? I thought it may be a useful thing to someone, at some point, > >but so far the scoreboard is: legit uses: 0, confused uses: 1 :S > > > >Thoughts? > > I don't know what the meaning of version is. I just never saw that being > touched. Is there any semantics documented for it? > > Kuba, any opinion? /me switches the first name in From :P I think it basically predates the op / policy introspection, and allows people to break backward compat. drop_monitor bumped to 2 in 2009: 683703a26e46 ("drop_monitor: Update netlink protocol to include netlink attribute header in alert message") which breaks backward compat. genetlink ctrl went to 2 in 2006: 334c29a64507 ("[GENETLINK]: Move command capabilities to flags.") which moves some info around in attrs, also breaks backward compat if someone depended on the old placement. ovs did it in 2013: 44da5ae5fbea ("openvswitch: Drop user features if old user space attempted to create datapath") again, breaks backwards compat. I guess it may still make one day to bump the version for some proto which has very tight control over the user space. But it hasn't happened for 10 years.
Fri, Oct 06, 2023 at 09:44:57PM CEST, kuba@kernel.org wrote: >On Fri, 6 Oct 2023 18:53:04 +0200 Jiri Pirko wrote: >> Fri, Oct 06, 2023 at 04:55:36PM CEST, kuba@kernel.org wrote: >> >> I'm confused. Didn't you say you'll remove this? If not, my question >> >> from v1 still stands. >> > >> >Perhaps we should dis-allow setting version in non-genetlink-legacy >> >specs? I thought it may be a useful thing to someone, at some point, >> >but so far the scoreboard is: legit uses: 0, confused uses: 1 :S >> > >> >Thoughts? >> >> I don't know what the meaning of version is. I just never saw that being >> touched. Is there any semantics documented for it? >> >> Kuba, any opinion? > >/me switches the first name in From :P I messed up a bit. Kuba* confusion, sorry :) > >I think it basically predates the op / policy introspection, >and allows people to break backward compat. > >drop_monitor bumped to 2 in 2009: > > 683703a26e46 ("drop_monitor: Update netlink protocol to include >netlink attribute header in alert message") > >which breaks backward compat. > >genetlink ctrl went to 2 in 2006: > > 334c29a64507 ("[GENETLINK]: Move command capabilities to flags.") > >which moves some info around in attrs, also breaks backward compat >if someone depended on the old placement. > >ovs did it in 2013: > > 44da5ae5fbea ("openvswitch: Drop user features if old user space >attempted to create datapath") > >again, breaks backwards compat. > > >I guess it may still make one day to bump the version for some proto >which has very tight control over the user space. But it hasn't >happened for 10 years. But since by the policy we cannot break uapi compat, version should be never bumped. I wonder howcome it is legit in the examples you listed above... Let's forbid that in genetlink.yaml. I have a patch ready, please ack this approach. Thx!
On Sat, 7 Oct 2023 12:29:47 +0200 Jiri Pirko wrote: > But since by the policy we cannot break uapi compat, version should be > never bumped. I wonder howcome it is legit in the examples you listed > above... Yes, even it's the 0.0001% of the time when "breaking' uAPI is fine, the change in the family spec can these days be much more precisely detected using policy dump. > Let's forbid that in genetlink.yaml. I have a patch ready, please ack > this approach. Ack, please remember to move version into the # Start genetlink-legacy section in the genetlink-legacy schema.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, October 6, 2023 2:30 PM > >Fri, Oct 06, 2023 at 01:40:58PM CEST, arkadiusz.kubalewski@intel.com wrote: >>Add attributes for providing the user with: >>- measurement of signals phase offset between pin and dpll >>- ability to adjust the phase of pin signal >> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >> Documentation/netlink/specs/dpll.yaml | 33 ++++++++++++++++++++++++++- >> drivers/dpll/dpll_nl.c | 8 ++++--- >> drivers/dpll/dpll_nl.h | 2 +- >> include/uapi/linux/dpll.h | 8 ++++++- >> 4 files changed, 45 insertions(+), 6 deletions(-) >> >>diff --git a/Documentation/netlink/specs/dpll.yaml >>b/Documentation/netlink/specs/dpll.yaml >>index 8b86b28b47a6..dc057494101f 100644 >>--- a/Documentation/netlink/specs/dpll.yaml >>+++ b/Documentation/netlink/specs/dpll.yaml >>@@ -1,7 +1,7 @@ >> # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR >>BSD-3-Clause) >> >> name: dpll >>- >>+version: 2 > >I'm confused. Didn't you say you'll remove this? If not, my question from >v1 still stands. I am just moron who have sent wrong patch :s Now is fixed in v4. Thank you, Arkadiusz
diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml index 8b86b28b47a6..dc057494101f 100644 --- a/Documentation/netlink/specs/dpll.yaml +++ b/Documentation/netlink/specs/dpll.yaml @@ -1,7 +1,7 @@ # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) name: dpll - +version: 2 doc: DPLL subsystem. definitions: @@ -164,6 +164,18 @@ definitions: - name: state-can-change doc: pin state can be changed + - + type: const + name: phase-offset-divider + value: 1000 + doc: | + phase offset divider allows userspace to calculate a value of + measured signal phase difference between a pin and dpll device + as a fractional value with three digit decimal precision. + Value of (DPLL_A_PHASE_OFFSET / DPLL_PHASE_OFFSET_DIVIDER) is an + integer part of a measured phase offest value. + Value of (DPLL_A_PHASE_OFFSET % DPLL_PHASE_OFFSET_DIVIDER) is a + fractional part of a measured phase offest value. attribute-sets: - @@ -272,6 +284,18 @@ attribute-sets: type: nest multi-attr: true nested-attributes: pin-parent-pin + - + name: phase-adjust-min + type: s32 + - + name: phase-adjust-max + type: s32 + - + name: phase-adjust + type: s32 + - + name: phase-offset + type: s64 - name: pin-parent-device subset-of: pin @@ -288,6 +312,9 @@ attribute-sets: - name: state type: u32 + - + name: phase-offset + type: s64 - name: pin-parent-pin subset-of: pin @@ -439,6 +466,9 @@ operations: - capabilities - parent-device - parent-pin + - phase-adjust-min + - phase-adjust-max + - phase-adjust dump: pre: dpll-lock-dumpit @@ -466,6 +496,7 @@ operations: - state - parent-device - parent-pin + - phase-adjust - name: pin-create-ntf doc: Notification about pin appearing diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c index 14064c8c783b..eaee5be7aa64 100644 --- a/drivers/dpll/dpll_nl.c +++ b/drivers/dpll/dpll_nl.c @@ -11,11 +11,12 @@ #include <uapi/linux/dpll.h> /* Common nested types */ -const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_STATE + 1] = { +const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_PHASE_OFFSET + 1] = { [DPLL_A_PIN_PARENT_ID] = { .type = NLA_U32, }, [DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2), [DPLL_A_PIN_PRIO] = { .type = NLA_U32, }, [DPLL_A_PIN_STATE] = NLA_POLICY_RANGE(NLA_U32, 1, 3), + [DPLL_A_PIN_PHASE_OFFSET] = { .type = NLA_S64, }, }; const struct nla_policy dpll_pin_parent_pin_nl_policy[DPLL_A_PIN_STATE + 1] = { @@ -61,7 +62,7 @@ static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] = }; /* DPLL_CMD_PIN_SET - do */ -static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_PIN + 1] = { +static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = { [DPLL_A_PIN_ID] = { .type = NLA_U32, }, [DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, }, [DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2), @@ -69,6 +70,7 @@ static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_PIN + 1] [DPLL_A_PIN_STATE] = NLA_POLICY_RANGE(NLA_U32, 1, 3), [DPLL_A_PIN_PARENT_DEVICE] = NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy), [DPLL_A_PIN_PARENT_PIN] = NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy), + [DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, }, }; /* Ops table for dpll */ @@ -140,7 +142,7 @@ static const struct genl_split_ops dpll_nl_ops[] = { .doit = dpll_nl_pin_set_doit, .post_doit = dpll_pin_post_doit, .policy = dpll_pin_set_nl_policy, - .maxattr = DPLL_A_PIN_PARENT_PIN, + .maxattr = DPLL_A_PIN_PHASE_ADJUST, .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, }; diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h index 1f67aaed4742..92d4c9c4f788 100644 --- a/drivers/dpll/dpll_nl.h +++ b/drivers/dpll/dpll_nl.h @@ -12,7 +12,7 @@ #include <uapi/linux/dpll.h> /* Common nested types */ -extern const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_STATE + 1]; +extern const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_PHASE_OFFSET + 1]; extern const struct nla_policy dpll_pin_parent_pin_nl_policy[DPLL_A_PIN_STATE + 1]; int dpll_lock_doit(const struct genl_split_ops *ops, struct sk_buff *skb, diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h index 20ef0718f8dc..050f51b48ef8 100644 --- a/include/uapi/linux/dpll.h +++ b/include/uapi/linux/dpll.h @@ -7,7 +7,7 @@ #define _UAPI_LINUX_DPLL_H #define DPLL_FAMILY_NAME "dpll" -#define DPLL_FAMILY_VERSION 1 +#define DPLL_FAMILY_VERSION 2 /** * enum dpll_mode - working modes a dpll can support, differentiates if and how @@ -138,6 +138,8 @@ enum dpll_pin_capabilities { DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE = 4, }; +#define DPLL_PHASE_OFFSET_DIVIDER 1000 + enum dpll_a { DPLL_A_ID = 1, DPLL_A_MODULE_NAME, @@ -173,6 +175,10 @@ enum dpll_a_pin { DPLL_A_PIN_CAPABILITIES, DPLL_A_PIN_PARENT_DEVICE, DPLL_A_PIN_PARENT_PIN, + DPLL_A_PIN_PHASE_ADJUST_MIN, + DPLL_A_PIN_PHASE_ADJUST_MAX, + DPLL_A_PIN_PHASE_ADJUST, + DPLL_A_PIN_PHASE_OFFSET, __DPLL_A_PIN_MAX, DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
Add attributes for providing the user with: - measurement of signals phase offset between pin and dpll - ability to adjust the phase of pin signal Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- Documentation/netlink/specs/dpll.yaml | 33 ++++++++++++++++++++++++++- drivers/dpll/dpll_nl.c | 8 ++++--- drivers/dpll/dpll_nl.h | 2 +- include/uapi/linux/dpll.h | 8 ++++++- 4 files changed, 45 insertions(+), 6 deletions(-)