| 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: <ZGYwA/gUzNKt02MK@nanopsycho> Date: Thu, 18 May 2023 16:02:43 +0200 From: Jiri Pirko <jiri@...nulli.us> To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com> Cc: Jakub Kicinski <kuba@...nel.org>, Vadim Fedorenko <vadfed@...a.com>, Jonathan Lemon <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>, "Olech, Milena" <milena.olech@...el.com>, "Michalik, Michal" <michal.michalik@...el.com>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Vadim Fedorenko <vadim.fedorenko@...ux.dev> Subject: Re: [RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML Thu, May 18, 2023 at 03:24:45PM CEST, arkadiusz.kubalewski@...el.com wrote: > >>From: Jiri Pirko <jiri@...nulli.us> >>Sent: Tuesday, May 16, 2023 4:34 PM >> >>Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@...el.com wrote: >>>>From: Jiri Pirko <jiri@...nulli.us> >>>>Sent: Monday, May 15, 2023 11:31 AM >>>> >>>>Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@...el.com wrote: >>>>>>From: Jiri Pirko <jiri@...nulli.us> >>>>>>Sent: Thursday, May 11, 2023 10:00 AM >>>>>> >>>>>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@...el.com >>>>>>wrote: >>>>>>>>From: Jakub Kicinski <kuba@...nel.org> >>>>>>>>Sent: Thursday, May 4, 2023 11:25 PM >>>>>>>> >>>>>>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>>>>>>> >+definitions: >>>>>>>>> >+ - >>>>>>>>> >+ type: enum >>>>>>>>> >+ name: mode >>>>>>>>> >+ doc: | >>>>>>>>> >+ working-modes a dpll can support, differentiate if and how >>>>>>>>> >dpll selects >>>>>>>>> >+ one of its sources to syntonize with it, valid values for >>>>>>>>> >DPLL_A_MODE >>>>>>>>> >+ attribute >>>>>>>>> >+ entries: >>>>>>>>> >+ - >>>>>>>>> >+ name: unspec >>>>>>>>> >>>>>>>>> In general, why exactly do we need unspec values in enums and CMDs? >>>>>>>>> What is the usecase. If there isn't please remove. >>>>>>>> >>>>>>>>+1 >>>>>>>> >>>>>>> >>>>>>>Sure, fixed. >>>>>>> >>>>>>>>> >+ doc: unspecified value >>>>>>>>> >+ - >>>>>>>>> >+ name: manual >>>>>>>> >>>>>>>>I think the documentation calls this "forced", still. >>>>>>>> >>>>>>> >>>>>>>Yes, good catch, fixed docs. >>>>>>> >>>>>>>>> >+ doc: source can be only selected by sending a request to >>>>>>>>> >dpll >>>>>>>>> >+ - >>>>>>>>> >+ name: automatic >>>>>>>>> >+ doc: highest prio, valid source, auto selected by dpll >>>>>>>>> >+ - >>>>>>>>> >+ name: holdover >>>>>>>>> >+ doc: dpll forced into holdover mode >>>>>>>>> >+ - >>>>>>>>> >+ name: freerun >>>>>>>>> >+ doc: dpll driven on system clk, no holdover available >>>>>>>>> >>>>>>>>> Remove "no holdover available". This is not a state, this is a mode >>>>>>>>> configuration. If holdover is or isn't available, is a runtime info. >>>>>>>> >>>>>>>>Agreed, seems a little confusing now. Should we expose the system clk >>>>>>>>as a pin to be able to force lock to it? Or there's some extra magic >>>>>>>>at play here? >>>>>>> >>>>>>>In freerun you cannot lock to anything it, it just uses system clock from >>>>>>>one of designated chip wires (which is not a part of source pins pool) to >>>>>>>feed the dpll. Dpll would only stabilize that signal and pass it further. >>>>>>>Locking itself is some kind of magic, as it usually takes at least ~15 >>>>>>>seconds before it locks to a signal once it is selected. >>>>>>> >>>>>>>> >>>>>>>>> >+ - >>>>>>>>> >+ name: nco >>>>>>>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>>>>>>> >>>>>>>>Noob question, what is NCO in terms of implementation? >>>>>>>>We source the signal from an arbitrary pin and FW / driver does >>>>>>>>the control? Or we always use system refclk and then tune? >>>>>>>> >>>>>>> >>>>>>>Documentation of chip we are using, stated NCO as similar to FREERUN, and >>>>>>>it >>>>>> >>>>>>So how exactly this is different to freerun? Does user care or he would >>>>>>be fine with "freerun" in this case? My point is, isn't "NCO" some >>>>>>device specific thing that should be abstracted out here? >>>>>> >>>>> >>>>>Sure, it is device specific, some synchronizing circuits would have this >>>>>capability, while others would not. >>>>>Should be abstracted out? It is a good question.. shall user know that he >>>>>is in >>>>>freerun with possibility to control the frequency or not? >>>>>Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins >>>>>supporting multiple output frequencies. >>>>>How the one would know if those frequencies are supported only in >>>>>MANUAL/AUTOMATIC modes or also in the FREERUN mode? >>>>>In other words: As the user can I change a frequency of a dpll if active >>>>>mode is FREERUN? >>>> >>>>Okay, I think I'm deep in the DPLL infra you are pushing, but my >>>>understanding that you can control frequency in NCO mode is not present >>>>:/ That only means it may be confusing and not described properly. >>>>How do you control this frequency exactly? I see no such knob. >>>> >>> >>>The set frequency is there already, although we miss phase offset I guess. >> >>Yeah, but on a pin, right? >> >> > >Yes frequency of an output pin is configurable, phase offset for a dpll or >output is not there, we might think of adding it.. But wait, we are talking about controllig freq of NCO, that has to be a different knob. Anyway, it is dropped, so the discussion is poitless now. > >> >>> >>>But I have changed my mind on having this in the kernel.. >>>Initially I have added this mode as our HW supports it, while thinking that >>>dpll subsystem shall have this, and we will implement it one day.. >>>But as we have not implemented it yet, let's leave work and discussion on >>>this mode for the future, when someone will actually try to implement it. >> >>Yeah, let's drop it then. One less confusing thing to wrap a head around :) >> > >Dropped. > >Thank you! >Arkadiusz > >> >>> >>>>Can't the oscilator be modeled as a pin and then you are not in freerun >>>>but locked this "internal pin"? We know how to control frequency there. >>>> >>> >>>Hmm, yeah probably could work this way. >>> >>> >>>Thank you! >>>Arkadiusz >>> >>>> >>>>> >>>>>I would say it is better to have such mode, we could argue on naming >>>>>>though. >>>>> >>>>>> >>>>>>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>>>>>>dividers before it reaches the output). >>>>>>>It doesn't count as an source pin, it uses signal form dedicated wire for >>>>>>>SYSTEM CLOCK. >>>>>>>In this case control over output frequency is done by synchronizer chip >>>>>>>firmware, but still it will not lock to any source pin signal. >>>>>>> >>>>>>>>> >+ render-max: true >>>>>>>>> >+ - >>>>>>>>> >+ type: enum >>>>>>>>> >+ name: lock-status >>>>>>>>> >+ doc: | >>>>>>>>> >+ provides information of dpll device lock status, valid >>>>>>>>> >>values for >>>>>>>>> >+ DPLL_A_LOCK_STATUS attribute >>>>>>>>> >+ entries: >>>>>>>>> >+ - >>>>>>>>> >+ name: unspec >>>>>>>>> >+ doc: unspecified value >>>>>>>>> >+ - >>>>>>>>> >+ name: unlocked >>>>>>>>> >+ doc: | >>>>>>>>> >+ dpll was not yet locked to any valid source (or is in one >>>>>>>>> >of >>>>>>>>> >+ modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO) >>>>>>>>> >+ - >>>>>>>>> >+ name: calibrating >>>>>>>>> >+ doc: dpll is trying to lock to a valid signal >>>>>>>>> >+ - >>>>>>>>> >+ name: locked >>>>>>>>> >+ doc: dpll is locked >>>>>>>>> >+ - >>>>>>>>> >+ name: holdover >>>>>>>>> >+ doc: | >>>>>>>>> >+ dpll is in holdover state - lost a valid lock or was >>>>>>>>> >forced by >>>>>>>>> >+ selecting DPLL_MODE_HOLDOVER mode >>>>>>>>> >>>>>>>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>>>>>>> because user might understand that the lock-status is always "holdover" >>>>>>>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>>>>>>> Perhaps I don't understand the flows there correctly :/ >>>>>>>> >>>>>>>>Hm, if we want to make sure that holdover mode must result in holdover >>>>>>>>state then we need some extra atomicity requirements on the SET >>>>>>>>operation. To me it seems logical enough that after setting holdover >>>>>>>>mode we'll end up either in holdover or unlocked status, depending on >>>>>>>>lock status when request reached the HW. >>>>>>>> >>>>>>> >>>>>>>Improved the docs: >>>>>>> name: holdover >>>>>>> doc: | >>>>>>> dpll is in holdover state - lost a valid lock or was forced >>>>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>>>>> if it was not, the dpll's lock-status will remain >>>>>> >>>>>>"if it was not" does not really cope with the sentence above that. Could >>>>>>you iron-out the phrasing a bit please? >>>>> >>>>> >>>>>Hmmm, >>>>> name: holdover >>>>> doc: | >>>>> dpll is in holdover state - lost a valid lock or was forced >>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>>> if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the >>>>> dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED >>>>> even if DPLL_MODE_HOLDOVER was requested) >>>>> >>>>>Hope this is better? >>>> >>>>Okay. >>>> >>>>> >>>>> >>>>>Thank you! >>>>>Arkadiusz >>>>> >>>>>[...]
Powered by blists - more mailing lists