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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Oct 10, 2024 at 03:48:02PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Thursday, October 10, 2024 1:36 PM
>>
>>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?
>>
>
>This is relevant to user if the clock provides both.
>I.e., given clock meets requirements for both Option1:PRC and
>Option2:PRS.
>How would you provide both of those to the user?

Currently, the attr is single value. So you imply that there is usecase
to report multiple clock quality at a single time?

Even with that. "PRC" and "PRS" names are enough to differenciate.
option prefix is redundant.


>
>The patch implements only option1 but the attribute shall
>be named adequately. So the user doesn't have to look for it
>or guessing around.
>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.

Why exactly do you need to expose "option"? What's the usecase?


>mlx code in 2/2 indicates this is option 1.
>Why uapi shall be silent about it?

Why is that needed? Also, uapi should provide some sort of abstraction.
"option1/2" is very ITU/SyncE specific. The idea is to be able to reuse
"quality-level" attr for non-synce usecases.


>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>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