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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

On Fri, Nov 22, 2024 at 06:09:16PM +0100, Javier Martinez Canillas wrote:
> Radu Rendec <rrendec@redhat.com> writes:
> 
> Hello Radu,
> 
> > On Thu, 2024-11-21 at 10:13 +0100, Javier Martinez Canillas wrote:
> >> Viresh Kumar <viresh.kumar@linaro.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]