| 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.
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
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 >
On 22-11-24, 18:09, Javier Martinez Canillas wrote: > 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. I am inclined to go with (1) here and have applied a patch from Andreas Kemnade, which he sent sometime back. I have used your commit log though, since it was more descriptive.
Viresh Kumar <viresh.kumar@linaro.org> writes: Hello Viresh, > On 22-11-24, 18:09, Javier Martinez Canillas wrote: >> 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. > > I am inclined to go with (1) here and have applied a patch from Andreas Kemnade, > which he sent sometime back. I have used your commit log though, since it was > more descriptive. > That sounds good to me, thanks for solving this issue! > -- > viresh >
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
Maxime Ripard <mripard@kernel.org> writes: Hello Maxime, > On Fri, Nov 22, 2024 at 06:09:16PM +0100, Javier Martinez Canillas wrote: [...] >> > 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. > Yeah, but the cpufreq-dt-platdev driver has a of_device_id table for the OPPv1 that matches the actual SoC, not the cpu device nodes. So for (2) I was thinking to do the same. But yes, agreed tha (1) is the best option here. > Maxime
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(+)