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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Oct 10, 2024 at 06:02:56PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Thursday, October 10, 2024 4:37 PM
>>
>>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?
>>
>
>Yes, correct. The userspace would decide which one to use.

Wait what? What do you mean by "which one to "use""?


>
>>Even with that. "PRC" and "PRS" names are enough to differenciate.
>>option prefix is redundant.
>>
>
>I do not ask for option prefix in the enum names, but specify somehow
>the option you do provide, and ability easily expand the uapi to provide
>both at the same time.. Backend can wait for someone to actually
>implement the option2, but we don't want to change uapi later, right?

So far, I fail to see what is the need for exposing the "option" info. I
may be missing something.


>
>>
>>>
>>>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?
>>
>
>The use case is to simply provide accurate information.
>With proposed changes the user will not know if provided class of
>ePRC is option 1 or 2.

How exactly does those 2 differ in terms of clock quality? If they
don't, why to differenciate them?


>
>>
>>>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.
>>
>
>Well, actually great point, makes most sense to me.
>Then the design shall be some kind of list of tuples?
>
>Like:
>--dump get-device
>{
>  'clock-id': 4658613174691233804,
>  'id':1,
>  'type':eec,
>  ...
>
>  'clock_spec':
>  [
>    {
>      "type": itu-option1,
>      "quality-level": eprc
>    },
>    {
>      "type": itu-option2,
>      "quality-level": eprc
>    },
>    ...
>  ]
>  ...
>}
>
>With assumption that for now only one "type" of itu-option1, but with
>ability to easily expand the uapi.
>
>The "quality-level" is already defined, and seems fine to me.
>
>Does it make sense?

Sort of. I would still very much like to avoid exposing multiple values
at a time as it complicates the implementation, namely driver op.




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