| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR11MB465748880C94832C56FF20009B792@DM6PR11MB4657.namprd11.prod.outlook.com> Date: Fri, 11 Oct 2024 19:50: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: Friday, October 11, 2024 5:57 PM > >Fri, Oct 11, 2024 at 04:25:09PM CEST, arkadiusz.kubalewski@...el.com wrote: >>>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?) > >Nope. It is the quality of clock the device is running. Locking does not >change that. We propagate this info over SyncE ESMC message. > Ok, but we shall also add such explanation somewhere in docs. To make sure everyone knows that it is provided as kind of "default" clock quality. Whenever dpll is in unlocked state, just to make sure there is no confusion on userspace. > >> >>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. > >DPLL_A_CLOCK_QUALITY_LEVEL with value DPLL_CLOCK_QUALITY_LEVEL_ITU_PRC >DPLL_A_CLOCK_QUALITY_LEVEL with value DPLL_CLOCK_QUALITY_LEVEL_ITU_PRS > >same attribute 2 times with different value. > > >But, in the meantime, I learned that for example ePRTC certification >may differ for option 1 and option 2. So we need to provide it. I will >just put it into the enum name. Looks as the best solution, UAPI and >internal op api-wise. > Fine, but then we also shall have 'multi-attr: true' for this attribute within this patch? Thank you! Arkadiusz [..]
Powered by blists - more mailing lists