Message-ID: <DM6PR11MB4657140103B9C33B3899041E9B782@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Thu, 10 Oct 2024 16:02:56 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"donald.hunter@...il.com" <donald.hunter@...il.com>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "saeedm@...dia.com"
	<saeedm@...dia.com>, "leon@...nel.org" <leon@...nel.org>, "tariqt@...dia.com"
	<tariqt@...dia.com>
Subject: RE: [PATCH net-next 1/2] dpll: add clock quality level attribute and
 op

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Thursday, October 10, 2024 4:37 PM
>
>Thu, Oct 10, 2024 at 03:48:02PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Thursday, October 10, 2024 1:36 PM
>>>
>>>Thu, Oct 10, 2024 at 11:53:30AM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>>>From: Jiri Pirko <jiri@...nulli.us>
>>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>>
>>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, arkadiusz.kubalewski@...el.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@...nulli.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@...dia.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.

>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?

>
>>
>>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.

>
>>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?

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

Powered by blists - more mailing lists