| Message ID | 20241119111918.1732531-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [RFC] cpufreq: dt-platdev: Fix module autoloading | expand |
+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 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 >
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 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.
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");
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(+)