linux-kernel - Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf_giA@mail.gmail.com>
Date:   Tue, 9 Mar 2021 08:44:29 -0700
From:   Rob Herring <robh@...nel.org>
To:     Rasmus Villemoes <rasmus.villemoes@...vas.dk>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        linux-clk <linux-clk@...r.kernel.org>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter

On Tue, Mar 9, 2021 at 12:39 AM Rasmus Villemoes
<rasmus.villemoes@...vas.dk> wrote:
>
> On 08/03/2021 22.38, Rob Herring wrote:
> > On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote:
> >> On 08/03/2021 18.21, Rob Herring wrote:
> >>> On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote:
> >>>> While a ripple counter can not usually be interfaced with (directly)
> >>>> from software, it may still be a crucial component in a board
> >>>> layout. To prevent its input clock from being disabled by the clock
> >>>> core because it apparently has no consumer, one needs to be able to
> >>>> represent that consumer in DT.
> >>>
> >>> I'm okay with this as it is describing h/w, but we already
> >>> 'protected-clocks' property which should work.
> >>
> >> Hm. Unless
> >> https://lore.kernel.org/lkml/20200903040015.5627-2-samuel@sholland.org/
> >> gets merged, I don't see how this would work out-of-the-box.
> >
> > Hum, no really clear what the hold up is there given it seems it was
> > asked for. Letting it sit for 5 months is certainly not the way
> > to get it merged. Anyways, that's the kernel's problem, not mine as far
> > as DT bindings are concerned.
> >
> >>
> >> Note that I sent a completely different v2, which made the gpio-wdt the
> >> clock consumer based on feedback from Guenter and Arnd, but that v2
> >> isn't suitable for our case because it post-poned handling of the
> >> watchdog till after i2c is ready, which is too late. Somewhat similar to
> >> https://lore.kernel.org/lkml/20210222171247.97609-2-sebastian.reichel@collabora.com/
> >> it seems.
> >
> > Now at that one in my queue... I think 'protected-clocks' is the best
> > way to avoid any driver probe ordering issues. It's the only thing that
> > really captures don't turn off this clock.
>
> Agreed, and I did start by looking for a generic way to mark the clock
> as either "hands off, kernel" (relying on the bootloader to enable it),
> or better "make sure it's enabled". The closest I found was
> of_clk_detect_critical(), but the comment above that one says not to use
> it, so adding a call to some random RTC driver to support the
> clock-critical property just for my use case didn't seem like the right
> way to go.
>
> I didn't know about protected-clocks until you mentioned it, and it does
> seem to be the right way to handle these situations (which are
> apparently more common than I thought).
>
> The ripple counter binding
> > doesn't really capture that or what it is related to.
>
> Agreed, it was a "hail mary" and why I explained what I was really
> trying to achieve in the cover letter.
>
> Also, the
> > ripple-counter driver could be a module and you'd still have the same
> > issue as v2.
>
> Well, not quite. First of all, for a board like this, one always uses a
> tailor-made .config, where one would never set that to be a module (and
> even more obviously one wouldn't make the gpio-wdt driver a module).

Yes, I'd expect so in this case, but in general we really should try
to avoid things dependent on being built-in (and ordering of
initcalls).

The whole notion of disabling resources in late_initcall is also kind
of broken IMO and doesn't account for modules.

> Second, it wouldn't be the same issue as v2. Rather, if the clock only
> gets enabled later when the ripple counter module would get loaded,
> there would be a period of time where the watchdog was rendered useless
> - the problem with v2 was that the watchdog wouldn't be petted in time,
> so the board would be reset before it booted completely.
>
> >>>> +Required properties:
> >>>> +- compatible: Must be "linux,ripple-ctr".
> >>>
> >>> Nothing linux specific about this.
> >>
> >> True, but I was following the lead of the existing gpio-wdt binding. Is
> >> there some other "vendor" name one can and should use for completely
> >> generic and simple components like these? "generic"?
> >
> > Most 'generic' and GPIO based interfaces have no vendor prefix.
>
> Ah, I see. Can we add just plain "wdt-gpio" to the gpio-wdt binding, and
> deprecate the "linux,wdt-gpio"? It's a little awkward to handle a
> "linux,wdt-gpio" compatible in a U-Boot driver.

No, just leave it. We have a few of these, but let's just not add new
ones. In the end, it's just a string identifier.

Rob