linux-kernel - Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when parent rate changes

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 for Android: free password hash cracker in your pocket

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

Message-ID: <zrjpbtf7qwaj2tjvfz2no534tmz5j4yudp45tung2w5x2zcl6y@bal3bclzze4e>
Date:   Fri, 25 Aug 2023 10:13:53 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Frank Oltmanns <frank@...manns.dev>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, Ondrej Jirman <x@...x.eu>,
        Icenowy Zheng <uwu@...nowy.me>, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...ts.linux.dev, dri-devel@...ts.freedesktop.org,
        Icenowy Zheng <icenowy@...c.io>
Subject: Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when
 parent rate changes

Hi,

On Fri, Aug 25, 2023 at 07:36:36AM +0200, Frank Oltmanns wrote:
> I would like to make the Allwinner A64's pll-mipi to keep its rate when
> its parent's (pll-video0) rate changes. Keeping pll-mipi's rate is
> required, to let the A64 drive both an LCD and HDMI display at the same
> time, because both have pll-video0 as an ancestor.
> 
> PATCH 1 adds this functionality as a feature into the clk framework (new
> flag: CLK_KEEP_RATE).
> 
> Cores that use this flag, store a rate as req_rate when it or one of its
> descendants requests a new rate.
> 
> That rate is then restored in the clk_change_rate recursion, which walks
> through the tree. It will reach the flagged core (e.g. pll-mipi) after
> the parent's rate (e.g. pll-video0) has already been set to the new
> rate. It will then call determine_rate (which requests the parent's
> current, i.e. new, rate) to determine a rate that is close to the
> flagged core's previous rate. Afterward it will re-calculate the rates
> for the flagged core's subtree.

I don't think it's the right way forward. It makes the core logic more
complicated, for something that is redundant with the notifiers
mechanism that has been the go-to for that kind of things so far.

It's not really obvious to me why the notifiers don't work there.

> This work is inspired by an out-of-tree patchset [1] [2] [3].
> Unfortunately, the patchset uses clk_set_rate() in a notifier callback,
> which the following comment on clk_notifier_register() forbids: "The
> callbacks associated with the notifier must not re-enter into the clk
> framework by calling any top-level clk APIs." [4] Furthermore, that
> out-of-tree patchset no longer works with the current linux-next,
> because setting pll-mipi is now also resetting pll-video0 [5].

Is it because of the "The callbacks associated with the notifier must
not re-enter into the clk framework by calling any top-level clk APIs."
comment?

If so, I think the thing we should emphasize is that it's about *any
top-level clk API*, as in clk_set_rate() or clk_set_parent().

The issue is that any consumer-facing API is taking the clk_prepare lock
and thus we would have reentrancy. But we're a provider there, and none
of the clk_hw_* functions are taking that lock. Neither do our own function.

So we could call in that notifier our set_rate callback directly, or we
could create a clk_hw_set_rate() function.

The first one will create cache issue between the actual rate that the
common clock framework is running and the one we actually enforced, but
we could create a function to flush the CCF cache.

The second one is probably simpler.

Another option could be that we turn clk_set_rate_exclusive into
something more subtle that allows to change a parent rate as long as the
clock rate doesn't. It would ease the requirement that
clk_set_rate_exclusive() has on a clock subtree (which I think prevents
its usage to some extent), but I have no issue on how that would work in
practice.

So yeah, I think adding a clk_hw_set_rate() that would be callable from
a notifier is the right way forward there.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)