[RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Thursday, May 11, 2023 10:14 AM
>
>Thu, May 11, 2023 at 09:38:04AM CEST, arkadiusz.kubalewski at intel.com wrote:

[...]

>>>>+      -
>>>>+        name: holdover
>>>>+        doc: |
>>>>+          dpll is in holdover state - lost a valid lock or was forced by
>>>>+          selecting DPLL_MODE_HOLDOVER mode
>>>
>>>Is it needed to mention the holdover mode. It's slightly confusing,
>>>because user might understand that the lock-status is always "holdover"
>>>in case of "holdover" mode. But it could be "unlocked", can't it?
>>>Perhaps I don't understand the flows there correctly :/
>>>
>>
>>Yes, it could be unlocked even when user requests the 'holdover' mode,
>i.e.
>>when the dpll was not locked to a valid source before requesting the mode.
>>Improved the docs:
>>        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 it was not, the dpll's lock-status will remain
>>          DPLL_LOCK_STATUS_UNLOCKED even if user requests
>>          DPLL_MODE_HOLDOVER)
>>Is that better?
>
>See my comment to this in the other branch of this thread.
>

Sure, gonna reply there.

[...]

>>>>+      -
>>>>+        name: int-oscillator
>>>>+        doc: device internal oscillator
>>>
>>>Is this somehow related to the mode "nco" (Numerically Controlled
>>>Oscillator)?
>>>
>>
>>Yes.
>
>How? Why do we need to expose it as a pin then?
>

Sorry, I messed up something with that answer..
It is not related to NCO (as NCO uses SYSTEM CLOCK to produce frequency).

It is a type of a pin which source or output is somehow internal. I.e.
our dpll's can syntonize to the 1 PPS clock signal produced by network chip.
As for other use-cases it could serve as way of having one or more oscillators
on board connected to input pins.

[...]

>>>>+    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
>>>
>>>Please have a separate create/delete/change values for pins.
>>>
>>
>>Makes sense, but details, pin creation doesn't occur from uAPI perspective,
>>as the pins itself are not visible to the user. They are visible after they
>>are registered with a device, thus we would have to do something like:
>>- pin-register
>>- pin-unregister
>>- pin-change
>>
>>Does it make sense?
>
>From perspective of user, it is "creation/new" or "deletion/del".
>Object appears of dissapears in UAPI, no matter how this is implemented
>in kernel. If you call it register/unregister, that exposes unnecessary
>internal kernel notation.
>
>No strong feeling though if you insist on register/unregister, it just
>sounds odd and funny.
>
>Anyway, one way or another, be in-sync naming wise with device events.
>

Sure going in sync with device event names seems best option, will fix as
suggested:
- pin-create
- pin-delete
- pin-change

>
>
>>
>>>
>>>>+        doc: |
>>>>+          attribute of dpll device or pin changed, reason is to be
>found
>>>>with
>>>>+          an attribute type (DPLL_A_*) received with the event
>>>>+
>>>>+
>>>>+attribute-sets:
>>>>+  -
>>>>+    name: dpll
>>>>+    enum-name: dplla
>>>>+    attributes:
>>>>+      -
>>>>+        name: device
>>>>+        type: nest
>>>>+        value: 1
>>>
>>>Why not 0?
>>>
>>
>>Sorry I don't recall what exact technical reasons are behind it, but all
>>netlink attributes I have found have 0 value attribute unused/unspec.
>
>I don't see why that is needed, I may be missing something though.
>Up to you.
>

Will leave it as is, if no other comments.

[...]

>>>>+      attribute-set: dpll
>>>>+      flags: [ admin-perm ]
>>>
>>>I may be missing something, but why do you enforce adming perm for
>>>get/dump cmds?
>>>
>>
>>Yes, security reasons, we don't want regular users to spam-query the driver
>>ops. Also explained in docs:
>>All netlink commands require ``GENL_ADMIN_PERM``. This is to prevent
>>any spamming/D.o.S. from unauthorized userspace applications.
>
>Hmm, I wonder why other read cmds usually don't need this. In fact,
>is there some read netlink cmd in kernel now which needs it?
>

Seems drivers/net/team/team.c uses it for get, but anyway this is a "least
privilege" security principle, if regular user cannot do anything with that
information, there is no point on providing it.

>
>>
>>>
>>>>+
>>>>+      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
>>>
>>>I might be missing something, but this means "device" netdev attribute
>>>DPLL_A_DEVICE, right? If yes, that is incorrect and you should list all
>>>the device attrs.
>>>
>>
>>Actually this means that attributes expected in response to this command are
>>from `device` subset.
>>But I see your point, will make `device` subset only for pin's nested
>>attributes, and here will list device attributes.
>
>Yes, that is my point. The fix you describes sounds fine.
>
>

Thank you!
Arkadiusz

[...]