linux-kernel - Re: [RFC PATCH] cpufreq: dt-platdev: Fix module autoloading

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: <20241125-eminent-devious-zebu-ec2caf@houat>
Date: Mon, 25 Nov 2024 10:01:31 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Javier Martinez Canillas <javierm@...hat.com>
Cc: Radu Rendec <rrendec@...hat.com>, 
	Viresh Kumar <viresh.kumar@...aro.org>, robh@...nel.org, arnd@...aro.org, linux-kernel@...r.kernel.org, 
	Zhipeng Wang <zhipeng.wang_1@....com>, javier@...hile0.org, "Rafael J. Wysocki" <rafael@...nel.org>, 
	linux-pm@...r.kernel.org
Subject: Re: [RFC PATCH] cpufreq: dt-platdev: Fix module autoloading

On Fri, Nov 22, 2024 at 06:09:16PM +0100, Javier Martinez Canillas wrote:
> Radu Rendec <rrendec@...hat.com> writes:
> 
> Hello Radu,
> 
> > On Thu, 2024-11-21 at 10:13 +0100, Javier Martinez Canillas wrote:
> >> Viresh Kumar <viresh.kumar@...aro.org> writes:
> >> 
> >> > On 21-11-24, 09:52, Javier Martinez Canillas wrote:
> >> > > Will autload the driver for any platform that has a Device Tree node with a
> >> > > compatible = "operating-points-v2" (assuming that this node will be a phandle
> >> > > for the "operating-points-v2" property.
> >> > > 
> >> > > For example, in the case of the following DT snippet:
> >> > > 
> >> > > cpus {
> >> > >         cpu@0 {
> >> > >                 operating-points-v2     = <&cpu0_opp_table>;
> >> > >         };
> >> > > };
> >> > > 
> >> > > cpu0_opp_table: opp_table {
> >> > >         compatible = "operating-points-v2";
> >> > > ...
> >> > > };
> >> > > 
> >> > > It will autoload if OF finds the opp_table node, but it register the cpufreq-dt
> >> > > device only if there's a cpu@0 with a "operating-points-v2" property.
> >> > > 
> >> > > Yes, there may be false positives because the autload semantics don't exactly
> >> > > match the criteria for the driver to "match" but I believe is better to load it
> >> > > and not use for those cases, than needing the driver and not autoloading it.
> >> > > 
> >> > > > I am not sure what's the best way forward to fix this.
> >> > > > 
> >> > > 
> >> > > I couldn't find another way to solve it, if you have a better idea please let
> >> > > me know. But IMO we should either workaround like this or revert the commit 
> >> > > that changed the driver's Kconfig symbol to be tristate.
> >> > 
> >> > Yeah, this needs to be fixed and this patch is one of the ways. Lets see if Arnd
> >> > or Rob have something to add, else can apply this patch.
> >> > 
> >> 
> >> Ok. Please notice though that this is an RFC, since all my arm64 machines have
> >> their own CPUFreq driver and are not using cpufreq-dt-platdev. So I would not
> >> apply it until someone actually tested the patch.
> >
> > I tested the patch on a Renesas R-Car S4 Spider (r8a779f0-spider.dts)
> > board, and it didn't work. I think the problem is that the OPP table DT
> > node does not have a corresponding device instance that is registered,
> > and therefore no modalias uevent is reported to udev/kmod.
> >
> 
> Thanks for testing! Bummer that the workaround didn't work. But that's why
> I asked you to test. You know, like Donald Knuth said: "Beware of bugs in
> the above code; I have only proved it correct, not tried it" :)
> 
> > FWIW, the OPP table is defined at the top of r8a779f0.dtsi and
> > referenced just a few more lines below, where the CPU nodes are
> > defined.
> >
> > As far as I understand, there are two options to fix this:
> >    1. Revert the patch that allows the cpufreq-dt-platdev driver to be
> >       built as a module. There's little benefit in allowing that anyway
> >       because the overhead at init time is minimal when the driver is
> >       unused, and driver can't be unloaded.
> >    2. Modify the driver and create an explicit of_device_id table of
> >       supported platforms for v2 too (like the existing one for v1) and
> >       use that instead of the cpu0_node_has_opp_v2_prop() call and the
> >       blacklist. That would of course also eliminate the blacklist.
> >
> 
> Agreed with this. Likely (1) is the easiest path and (2) would make the
> driver more aligned with the rest of the kernel (that have a list of OF
> device IDs to autoload / match instead of some custom logic).
> 
> But I guess that (2) would be riskier, since not adding a platform that
> uses v2 will cause a regression.

Also, 2 probably wouldn't work. Devices under /cpus don't get a struct
device created for them, so it wouldn't probe.

Maxime

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