Re: [PATCH net-next 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko <jiri@resnulli.us>]

Thu, Oct 10, 2024 at 11:53:30AM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, October 9, 2024 4:07 PM
>>
>>Wed, Oct 09, 2024 at 03:38:38PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>
>>>>In order to allow driver expose quality level of the clock it is
>>>>running, introduce a new netlink attr with enum to carry it to the
>>>>userspace. Also, introduce an op the dpll netlink code calls into the
>>>>driver to obtain the value.
>>>>
>>>>Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>>>>---
>>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>> include/linux/dpll.h                  |  4 ++++
>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>> 4 files changed, 75 insertions(+)
>>>>
>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>@@ -85,6 +85,30 @@ definitions:
>>>>           This may happen for example if dpll device was previously
>>>>           locked on an input pin of type PIN_TYPE_SYNCE_ETH_PORT.
>>>>     render-max: true
>>>>+  -
>>>>+    type: enum
>>>>+    name: clock-quality-level
>>>>+    doc: |
>>>>+      level of quality of a clock device.
>>>
>>>Hi Jiri,
>>>
>>>Thanks for your work on this!
>>>
>>>I do like the idea, but this part is a bit tricky.
>>>
>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>spec "Table 11-7" of REC-G.8264?
>>
>>For now, yes. That is the usecase I have currently. But, if anyone will have a
>>need to introduce any sort of different quality, I don't see why not.
>>
>>>
>>>Then what about table 11-8?
>>
>>The names do not overlap. So if anyone need to add those, he is free to do it.
>>
>
>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>As you already pointed below :)

Yep, sure.

>
>>
>>>
>>>And in general about option 2(3?) networks?
>>>
>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>Quality Levels).
>>>
>>>Assuming 2(3?) network options shall be available, either user can
>>>select the one which is shown, or driver just provides all (if can,
>>>one/none otherwise)?
>>>
>>>If we don't want to give the user control and just let the driver to
>>>either provide this or not, my suggestion would be to name the
>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>provided attribute belongs to option 1 network.
>>
>>I was thinking about that but there are 2 groups of names in both
>>tables:
>>1) different quality levels and names. Then "o1/2" in the name is not
>>   really needed, as the name itself is the differentiator.
>>2) same quality leves in both options. Those are:
>>   PRTC
>>   ePRTC
>>   eEEC
>>   ePRC
>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>   for exactly the same quality level.
>>
>
>Those names overlap but corresponding SSM is different depending on
>the network option, providing one of those without network option will
>confuse users.

The ssm code is different, but that is irrelevant in context of this
UAPI. Clock quality levels are the same, that's what matters, isn't it?


>
>For me one enum list for clock types/quality sounds good.
>
>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to indicate
>>this is ITU standartized clock quality leaving option for some other clock
>>quality namespace to appear?
>>
>>[..]
>
>Sure, also makes sense.
>
>But I still believe the attribute name shall also contain the info that
>it conveys an option1 clock type. As the device can meet both specifications
>at once, we need to make sure user knows that.

As I described, I don't see any reason why. Just adds unnecessary
redundancy to uapi.


>
>Thank you!
>Arkadiusz