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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZISjMUcpmUTBXIOA@nanopsycho>
Date: Sat, 10 Jun 2023 18:22:09 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Cc: kuba@...nel.org, vadfed@...a.com, jonathan.lemon@...il.com,
	pabeni@...hat.com, corbet@....net, davem@...emloft.net,
	edumazet@...gle.com, vadfed@...com, jesse.brandeburg@...el.com,
	anthony.l.nguyen@...el.com, saeedm@...dia.com, leon@...nel.org,
	richardcochran@...il.com, sj@...nel.org, javierm@...hat.com,
	ricardo.canuelo@...labora.com, mst@...hat.com, tzimmermann@...e.de,
	michal.michalik@...el.com, gregkh@...uxfoundation.org,
	jacek.lawrynowicz@...ux.intel.com, airlied@...hat.com,
	ogabbay@...nel.org, arnd@...db.de, nipun.gupta@....com,
	axboe@...nel.dk, linux@...y.sk, masahiroy@...nel.org,
	benjamin.tissoires@...hat.com, geert+renesas@...der.be,
	milena.olech@...el.com, kuniyu@...zon.com, liuhangbin@...il.com,
	hkallweit1@...il.com, andy.ren@...cruise.com, razor@...ckwall.org,
	idosch@...dia.com, lucien.xin@...il.com, nicolas.dichtel@...nd.com,
	phil@....cc, claudiajkang@...il.com, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	intel-wired-lan@...ts.osuosl.org, linux-rdma@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, poros@...hat.com,
	mschmidt@...hat.com, linux-clk@...r.kernel.org,
	vadim.fedorenko@...ux.dev
Subject: Re: [RFC PATCH v8 02/10] dpll: spec: Add Netlink spec in YAML

Fri, Jun 09, 2023 at 02:18:45PM CEST, arkadiusz.kubalewski@...el.com wrote:
>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: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>---
> Documentation/netlink/specs/dpll.yaml | 466 ++++++++++++++++++++++++++
> drivers/dpll/dpll_nl.c                | 161 +++++++++
> drivers/dpll/dpll_nl.h                |  50 +++
> include/uapi/linux/dpll.h             | 184 ++++++++++
> 4 files changed, 861 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..f7317003d312
>--- /dev/null
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -0,0 +1,466 @@
>+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
>+
>+name: dpll
>+
>+doc: DPLL subsystem.
>+
>+definitions:
>+  -
>+    type: enum
>+    name: mode
>+    doc: |
>+      working-modes a dpll can support, differentiate if and how dpll selects

s/working-modes/working modes/
s/differentiate/differentiates/
?


>+      one of its inputs to syntonize with it, valid values for DPLL_A_MODE
>+      attribute
>+    entries:
>+      -
>+        name: manual
>+        doc: input can be only selected by sending a request to dpll
>+        value: 1
>+      -
>+        name: automatic
>+        doc: highest prio, valid input, auto selected by dpll

s/valid input, auto selected by dpll/input pin auto selected by dpll/
?


>+      -
>+        name: holdover
>+        doc: dpll forced into holdover mode
>+      -
>+        name: freerun
>+        doc: dpll driven on system clk

Thinking about modes "holdover" and "freerun".
1) You don't use them anywhere in this patchset, please remove them
   until they are needed. ptp_ocp and ice uses automatic, mlx5 uses
   manual. Btw, are there any other unused parts of UAPI? If yes, could
   you please remove them too?

2) I don't think it is correct to have them.
   a) to achieve holdover:
      if state is LOCKED_HO_ACQ you just disconnect all input pins.
   b) to achieve freerun:
      if state LOCKED you just disconnect all input pins.
   So don't mangle the mode with status.


>+    render-max: true
>+  -
>+    type: enum
>+    name: lock-status
>+    doc: |
>+      provides information of dpll device lock status, valid values for
>+      DPLL_A_LOCK_STATUS attribute
>+    entries:
>+      -
>+        name: unlocked
>+        doc: |
>+          dpll was not yet locked to any valid input (or is in mode:
>+            DPLL_MODE_FREERUN)

Don't forget to remove the mention of mode freerun from here.


>+        value: 1
>+      -
>+        name: locked
>+        doc: |
>+          dpll is locked to a valid signal, but no holdover available
>+      -
>+        name: locked-ho-acq
>+        doc: |
>+          dpll is locked and holdover acquired
>+      -
>+        name: holdover
>+        doc: |
>+          dpll is in holdover state - lost a valid lock or was forced
>+          by selecting DPLL_MODE_HOLDOVER mode (latter possible only
>+          when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED,
>+          if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the
>+          dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED
>+          even if DPLL_MODE_HOLDOVER was requested)

Don't forget to remove the mention of mode holdover from here.


>+    render-max: true
>+  -
>+    type: const
>+    name: temp-divider
>+    value: 1000
>+    doc: |
>+      temperature divider allowing userspace to calculate the
>+      temperature as float with three digit decimal precision.
>+      Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of
>+      temperature value.
>+      Value of (DPLL_A_TEMP % DPLL_TEMP_DIVIDER) is fractional part of
>+      temperature value.
>+  -
>+    type: enum
>+    name: type
>+    doc: type of dpll, valid values for DPLL_A_TYPE attribute
>+    entries:
>+      -
>+        name: pps
>+        doc: dpll produces Pulse-Per-Second signal
>+        value: 1
>+      -
>+        name: eec
>+        doc: dpll drives the Ethernet Equipment Clock
>+    render-max: true
>+  -
>+    type: enum
>+    name: pin-type
>+    doc: |
>+      defines possible types of a pin, valid values for DPLL_A_PIN_TYPE
>+      attribute
>+    entries:
>+      -
>+        name: mux
>+        doc: aggregates another layer of selectable pins
>+        value: 1
>+      -
>+        name: ext
>+        doc: external input
>+      -
>+        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-direction
>+    doc: |
>+      defines possible direction of a pin, valid values for
>+      DPLL_A_PIN_DIRECTION attribute
>+    entries:
>+      -
>+        name: input
>+        doc: pin used as a input of a signal

I don't think I have any objections against "input", but out of
curiosity, why you changed that from "source"?


>+        value: 1
>+      -
>+        name: output
>+        doc: pin used to output the signal
>+    render-max: true
>+  -
>+    type: const
>+    name: pin-frequency-1-hz
>+    value: 1
>+  -
>+    type: const
>+    name: pin-frequency-10-khz
>+    value: 10000
>+  -
>+    type: const
>+    name: pin-frequency-77_5-khz
>+    value: 77500
>+  -
>+    type: const
>+    name: pin-frequency-10-mhz
>+    value: 10000000
>+  -
>+    type: enum
>+    name: pin-state
>+    doc: |
>+      defines possible states of a pin, valid values for
>+      DPLL_A_PIN_STATE attribute
>+    entries:
>+      -
>+        name: connected
>+        doc: pin connected, active input of phase locked loop
>+        value: 1
>+      -
>+        name: disconnected
>+        doc: pin disconnected, not considered as a valid input
>+      -
>+        name: selectable
>+        doc: pin enabled for automatic input selection
>+    render-max: true
>+  -
>+    type: flags
>+    name: pin-caps
>+    doc: |
>+      defines possible capabilities of a pin, valid flags on
>+      DPLL_A_PIN_CAPS attribute
>+    entries:
>+      -
>+        name: direction-can-change
>+      -
>+        name: priority-can-change
>+      -
>+        name: state-can-change
>+
>+attribute-sets:
>+  -
>+    name: dpll
>+    enum-name: dpll_a
>+    attributes:
>+      -
>+        name: id
>+        type: u32
>+        value: 1
>+      -
>+        name: module-name
>+        type: string
>+      -
>+        name: clock-id
>+        type: u64
>+      -
>+        name: mode
>+        type: u8
>+        enum: mode
>+      -
>+        name: mode-supported
>+        type: u8
>+        enum: mode
>+        multi-attr: true
>+      -
>+        name: lock-status
>+        type: u8
>+        enum: lock-status
>+      -
>+        name: temp
>+        type: s32
>+      -
>+        name: type
>+        type: u8
>+        enum: type
>+      -
>+        name: pin-id
>+        type: u32
>+      -
>+        name: pin-board-label
>+        type: string
>+      -
>+        name: pin-panel-label
>+        type: string
>+      -
>+        name: pin-package-label
>+        type: string

Wouldn't it make sense to add some small documentation blocks to the
attrs? IDK.


>+      -
>+        name: pin-type
>+        type: u8
>+        enum: pin-type
>+      -
>+        name: pin-direction
>+        type: u8
>+        enum: pin-direction
>+      -
>+        name: pin-frequency
>+        type: u64
>+      -
>+        name: pin-frequency-supported
>+        type: nest
>+        multi-attr: true
>+        nested-attributes: pin-frequency-range
>+      -
>+        name: pin-frequency-min
>+        type: u64
>+      -
>+        name: pin-frequency-max
>+        type: u64
>+      -
>+        name: pin-prio
>+        type: u32
>+      -
>+        name: pin-state
>+        type: u8
>+        enum: pin-state
>+      -
>+        name: pin-dpll-caps
>+        type: u32
>+      -
>+        name: pin-parent
>+        type: nest
>+        multi-attr: true
>+        nested-attributes: pin-parent
>+  -
>+    name: pin-parent
>+    subset-of: dpll
>+    attributes:
>+      -
>+        name: id
>+        type: u32
>+      -
>+        name: pin-direction
>+        type: u8
>+      -
>+        name: pin-prio
>+        type: u32
>+      -
>+        name: pin-state
>+        type: u8
>+      -
>+        name: pin-id
>+        type: u32
>+
>+  -
>+    name: pin-frequency-range
>+    subset-of: dpll
>+    attributes:
>+      -
>+        name: pin-frequency-min
>+        type: u64
>+      -
>+        name: pin-frequency-max
>+        type: u64

[...]

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ