Message-ID: <DM6PR11MB4657805DBAA2CCA70D233D2B9B792@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 11 Oct 2024 14:25:09 +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: Friday, October 11, 2024 8:46 AM
>
>Thu, Oct 10, 2024 at 06:02:56PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>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.
>
>Wait what? What do you mean by "which one to "use""?
>

Generally if you provide some information to user, he shall use it
somehow, right?

Maybe you could explain the use case as you are the one developing it?
Is this to tell user a quality level of your device itself - without
anything connected?
Does it change during runtime? (i.e. locking to a neighbor?)

I already explained my concerns: if device would be providing multiple
clock qualities for different specification options (i.e. ITU-T Option
1/2(/3)) and they would differ for single device with regards to the
meeting certain class spec requirements, the list of those would be
required. Since the names for both do overlap it would be not possible
to let the user know accurately without changes to the uapi.

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

See above and below :)

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

All I could find is that the same name of clock class the quality
shall not differ. But again I am not saying we shall differentiate
the clock class.. Rather an ability to provide each one, keeping in
mind that device can meet different O1 vs O2 (vs 03.

How would we extend the interface to provide Option 2 clock class?
I still see some entropy, I mean higher then expected..
Let's say device meets PRC for Option1 and PRS for Option2, how would
we provide this?
The same attribute twice? This seems also possibly confusing for
certain cases.

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

Well, true.

Thank you!
Arkadiusz

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