cpufreq: dt-platdev: Fix module autoloading

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

Posted by Javier Martinez Canillas 4 days, 1 hour ago

This driver can be built as a module since commit 3b062a086984 ("cpufreq:
dt-platdev: Support building as module"), but unfortunately this caused
a regression because the cputfreq-dt-platdev.ko module does not autoload.

Usually, this is solved by just using the MODULE_DEVICE_TABLE() macro to
export all the device IDs as module aliases. But this driver is special
due how matches with devices and decides what platform supports.

There are two of_device_id lists, an allow list that are for CPU devices
that always match and a deny list that's for devices that must not match.

The driver registers a cpufreq-dt platform device for all the CPU device
nodes that either are in the allow list or contain an operating-points-v2
property and are not in the deny list.

For the former just add a MODULE_DEVICE_TABLE(), and for the latter add a
module alias. That way the driver would always be autoloaded when needed.

Reported-by: Radu Rendec <rrendec@redhat.com>
Fixes: 3b062a086984 ("cpufreq: dt-platdev: Support building as module")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
Posting as an RFC because I don't have a platform that uses this driver
but I'll let Radu test since he reported by issue.

 drivers/cpufreq/cpufreq-dt-platdev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 2a3e8bd317c9..7ae7c897c249 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -97,6 +97,7 @@ static const struct of_device_id allowlist[] __initconst = {
 
 	{ }
 };
+MODULE_DEVICE_TABLE(of, allowlist);
 
 /*
  * Machines for which the cpufreq device is *not* created, mostly used for
@@ -236,4 +237,16 @@ static int __init cpufreq_dt_platdev_init(void)
 }
 core_initcall(cpufreq_dt_platdev_init);
 MODULE_DESCRIPTION("Generic DT based cpufreq platdev driver");
+/*
+ * The module alias is needed because the driver automatically registers a
+ * platform device for any CPU device node that has an operating-points-v2
+ * property and is not in the block list.
+ *
+ * For this reason the MODULE_DEVICE_TABLE() macro can only export aliases
+ * of the devices in the allow list, which means that the driver will not
+ * autoload for devices whose cpufreq-dt will be registered automatically.
+ *
+ * Adding an "of:N*T*Coperating-points-v2" alias is a workaround for this.
+ */
+MODULE_ALIAS("of:N*T*Coperating-points-v2");
 MODULE_LICENSE("GPL");
-- 
2.47.0

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

Posted by Viresh Kumar 2 days, 5 hours ago

+Rob/Arnd,

On 19-11-24, 12:18, Javier Martinez Canillas wrote:
> This driver can be built as a module since commit 3b062a086984 ("cpufreq:
> dt-platdev: Support building as module"), but unfortunately this caused
> a regression because the cputfreq-dt-platdev.ko module does not autoload.
> 
> Usually, this is solved by just using the MODULE_DEVICE_TABLE() macro to
> export all the device IDs as module aliases. But this driver is special
> due how matches with devices and decides what platform supports.
> 
> There are two of_device_id lists, an allow list that are for CPU devices
> that always match and a deny list that's for devices that must not match.
> 
> The driver registers a cpufreq-dt platform device for all the CPU device
> nodes that either are in the allow list or contain an operating-points-v2
> property and are not in the deny list.
> 
> For the former just add a MODULE_DEVICE_TABLE(), and for the latter add a
> module alias. That way the driver would always be autoloaded when needed.
> 
> Reported-by: Radu Rendec <rrendec@redhat.com>
> Fixes: 3b062a086984 ("cpufreq: dt-platdev: Support building as module")
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> Posting as an RFC because I don't have a platform that uses this driver
> but I'll let Radu test since he reported by issue.
> 
>  drivers/cpufreq/cpufreq-dt-platdev.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 2a3e8bd317c9..7ae7c897c249 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -97,6 +97,7 @@ static const struct of_device_id allowlist[] __initconst = {
>  
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(of, allowlist);

This is fine obviously.

>  /*
>   * Machines for which the cpufreq device is *not* created, mostly used for
> @@ -236,4 +237,16 @@ static int __init cpufreq_dt_platdev_init(void)
>  }
>  core_initcall(cpufreq_dt_platdev_init);
>  MODULE_DESCRIPTION("Generic DT based cpufreq platdev driver");
> +/*
> + * The module alias is needed because the driver automatically registers a
> + * platform device for any CPU device node that has an operating-points-v2
> + * property and is not in the block list.
> + *
> + * For this reason the MODULE_DEVICE_TABLE() macro can only export aliases
> + * of the devices in the allow list, which means that the driver will not
> + * autoload for devices whose cpufreq-dt will be registered automatically.
> + *
> + * Adding an "of:N*T*Coperating-points-v2" alias is a workaround for this.
> + */
> +MODULE_ALIAS("of:N*T*Coperating-points-v2");

How does this work? This will autoload the module for any platform with
"operating-points-v2" property in the DT ? The driver though works only if the
property is in the CPU node, while now we will try to load this driver even if a
non-CPU node has the property now.

I am not sure what's the best way forward to fix this.

Arnd, Rob, any inputs ?

>  MODULE_LICENSE("GPL");

-- 
viresh

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

Posted by Javier Martinez Canillas 2 days, 3 hours ago

Viresh Kumar <viresh.kumar@linaro.org> writes:

Hello Viresh,

> +Rob/Arnd,
>
> On 19-11-24, 12:18, Javier Martinez Canillas wrote:
>> This driver can be built as a module since commit 3b062a086984 ("cpufreq:
>> dt-platdev: Support building as module"), but unfortunately this caused
>> a regression because the cputfreq-dt-platdev.ko module does not autoload.
>> 
>> Usually, this is solved by just using the MODULE_DEVICE_TABLE() macro to
>> export all the device IDs as module aliases. But this driver is special
>> due how matches with devices and decides what platform supports.
>> 
>> There are two of_device_id lists, an allow list that are for CPU devices
>> that always match and a deny list that's for devices that must not match.
>> 
>> The driver registers a cpufreq-dt platform device for all the CPU device
>> nodes that either are in the allow list or contain an operating-points-v2
>> property and are not in the deny list.
>> 
>> For the former just add a MODULE_DEVICE_TABLE(), and for the latter add a
>> module alias. That way the driver would always be autoloaded when needed.
>> 
>> Reported-by: Radu Rendec <rrendec@redhat.com>
>> Fixes: 3b062a086984 ("cpufreq: dt-platdev: Support building as module")
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>> Posting as an RFC because I don't have a platform that uses this driver
>> but I'll let Radu test since he reported by issue.
>> 
>>  drivers/cpufreq/cpufreq-dt-platdev.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
>> index 2a3e8bd317c9..7ae7c897c249 100644
>> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
>> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
>> @@ -97,6 +97,7 @@ static const struct of_device_id allowlist[] __initconst = {
>>  
>>  	{ }
>>  };
>> +MODULE_DEVICE_TABLE(of, allowlist);
>
> This is fine obviously.
>

Yeah, this part is trivial.

>>  /*
>>   * Machines for which the cpufreq device is *not* created, mostly used for
>> @@ -236,4 +237,16 @@ static int __init cpufreq_dt_platdev_init(void)
>>  }
>>  core_initcall(cpufreq_dt_platdev_init);
>>  MODULE_DESCRIPTION("Generic DT based cpufreq platdev driver");
>> +/*
>> + * The module alias is needed because the driver automatically registers a
>> + * platform device for any CPU device node that has an operating-points-v2
>> + * property and is not in the block list.
>> + *
>> + * For this reason the MODULE_DEVICE_TABLE() macro can only export aliases
>> + * of the devices in the allow list, which means that the driver will not
>> + * autoload for devices whose cpufreq-dt will be registered automatically.
>> + *
>> + * Adding an "of:N*T*Coperating-points-v2" alias is a workaround for this.
>> + */
>> +MODULE_ALIAS("of:N*T*Coperating-points-v2");
>
> How does this work? This will autoload the module for any platform with
> "operating-points-v2" property in the DT ? The driver though works only if the
> property is in the CPU node, while now we will try to load this driver even if a
> non-CPU node has the property now.
>

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.

> Arnd, Rob, any inputs ?
>
>>  MODULE_LICENSE("GPL");
>
> -- 
> viresh
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

Posted by Viresh Kumar 2 days, 3 hours ago

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.

-- 
viresh

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

Posted by Javier Martinez Canillas 2 days, 3 hours ago

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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

Posted by Radu Rendec 20 hours ago

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.

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.

--
Best regards,
Radu Rendec

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

Posted by Javier Martinez Canillas 19 hours ago

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