linux-kernel - Re: [PATCH 2/3] clk: keystone: don't cache clock rate

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] [day] [month] [year] [list]

Message-Id: <DCZ5D3DB786G.3JANU1P578TCY@kernel.org>
Date: Mon, 22 Sep 2025 09:23:01 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: "Randolph Sapp" <rs@...com>, "Frank Binns" <frank.binns@...tec.com>,
 "Matt Coster" <matt.coster@...tec.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Nishanth Menon" <nm@...com>, "Vignesh
 Raghavendra" <vigneshr@...com>, "Tero Kristo" <kristo@...nel.org>, "Santosh
 Shilimkar" <ssantosh@...nel.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Stephen Boyd" <sboyd@...nel.org>
Cc: "Andrew Davis" <afd@...com>, <dri-devel@...ts.freedesktop.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>, <linux-clk@...r.kernel.org>
Subject: Re: [PATCH 2/3] clk: keystone: don't cache clock rate

Hi,

> > The TISCI firmware will return 0 if the clock or consumer is not
> > enabled although there is a stored value in the firmware. IOW a call to
> > set rate will work but at get rate will always return 0 if the clock is
> > disabled.
> > The clk framework will try to cache the clock rate when it's requested
> > by a consumer. If the clock or consumer is not enabled at that point,
> > the cached value is 0, which is wrong. Thus, disable the cache
> > altogether.
> >
> > Signed-off-by: Michael Walle <mwalle@...nel.org>
> > ---
> > I guess to make it work correctly with the caching of the linux
> > subsystem a new flag to query the real clock rate is needed. That
> > way, one could also query the default value without having to turn
> > the clock and consumer on first. That can be retrofitted later and
> > the driver could query the firmware capabilities.
> >
> > Regarding a Fixes: tag. I didn't include one because it might have a
> > slight performance impact because the firmware has to be queried
> > every time now and it doesn't have been a problem for now. OTOH I've
> > enabled tracing during boot and there were just a handful
> > clock_{get/set}_rate() calls.
> > ---
> >  drivers/clk/keystone/sci-clk.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> > index c5894fc9395e..d73858b5ca7a 100644
> > --- a/drivers/clk/keystone/sci-clk.c
> > +++ b/drivers/clk/keystone/sci-clk.c
> > @@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
> >  
> >  	init.ops = &sci_clk_ops;
> >  	init.num_parents = sci_clk->num_parents;
> > +
> > +	/*
> > +	 * A clock rate query to the SCI firmware will return 0 if either the
> > +	 * clock itself is disabled or the attached device/consumer is disabled.
> > +	 * This makes it inherently unsuitable for the caching of the clk
> > +	 * framework.
> > +	 */
> > +	init.flags = CLK_GET_RATE_NOCACHE;
> >  	sci_clk->hw.init = &init;
> >  
> >  	ret = devm_clk_hw_register(provider->dev, &sci_clk->hw);
>
>
> Thanks for looking into this Michael. I'm still convinced that it's unusual to
> report 0 for a clock rate when the device is powered down. In most cases it's
> not actually 0 and is actually just in bypass mode.

Yeah. And you can't query the clock rate you've just set if the
clock is off (and if enabled the clock will have the frequency of
the last set_rate). I still think that is a gap in the firmware
interface.

> I was told it's a way to indicate clock status and probably won't be changing
> any time soon though. Ignore the fact that we also already have a separate way
> to query clock status. :)
>
> This series looks good, but won't quite result in a functional GPU without the
> following patch: https://lore.kernel.org/all/20250808232522.1296240-1-rs@ti.com/

Ahh, I was puzzled why it was working for me. But then I've noticed
that your patch is for the am62p. I'm working with the am67a and
there the ranges are correct for the GPU.

> I suppose I'll submit that again on it's own.
>
> Reviewed-by: Randolph Sapp <rs@...com>

Thanks.

-michael

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