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

[Javier Martinez Canillas <javierm@redhat.com>]

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.

> --
> Best regards,
> Radu Rendec
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat