The PSCI cpuidle driver does not require the creation of a platform
device. Originally, this approach was chosen for simplicity when the
driver was first implemented.
With the introduction of the lightweight faux device interface, we now
have a more appropriate alternative. Migrate the driver to utilize the
faux bus, given that the platform device it previously created was not
a real one anyway. This will simplify the code, reducing its footprint
while maintaining functionality.
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/cpuidle/cpuidle-psci.c | 32 ++++----------------------------
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2562dc001fc1de69732ef28f383d2809262a3d96..5d4d6daed36d8540ba2ce3dc54a3180731b03d22 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -16,7 +16,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
-#include <linux/platform_device.h>
+#include <linux/device/faux.h>
#include <linux/psci.h>
#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
@@ -404,14 +404,14 @@ static int psci_idle_init_cpu(struct device *dev, int cpu)
* to register cpuidle driver then rollback to cancel all CPUs
* registration.
*/
-static int psci_cpuidle_probe(struct platform_device *pdev)
+static int psci_cpuidle_probe(struct faux_device *fdev)
{
int cpu, ret;
struct cpuidle_driver *drv;
struct cpuidle_device *dev;
for_each_possible_cpu(cpu) {
- ret = psci_idle_init_cpu(&pdev->dev, cpu);
+ ret = psci_idle_init_cpu(&fdev->dev, cpu);
if (ret)
goto out_fail;
}
@@ -431,28 +431,4 @@ static int psci_cpuidle_probe(struct platform_device *pdev)
return ret;
}
-static struct platform_driver psci_cpuidle_driver = {
- .probe = psci_cpuidle_probe,
- .driver = {
- .name = "psci-cpuidle",
- },
-};
-
-static int __init psci_idle_init(void)
-{
- struct platform_device *pdev;
- int ret;
-
- ret = platform_driver_register(&psci_cpuidle_driver);
- if (ret)
- return ret;
-
- pdev = platform_device_register_simple("psci-cpuidle", -1, NULL, 0);
- if (IS_ERR(pdev)) {
- platform_driver_unregister(&psci_cpuidle_driver);
- return PTR_ERR(pdev);
- }
-
- return 0;
-}
-device_initcall(psci_idle_init);
+module_faux_driver(psci_cpuidle, psci_cpuidle_probe, NULL, true);
--
2.34.1Hi Sudeep,
On 18/03/2025 17:01, Sudeep Holla wrote:
> The PSCI cpuidle driver does not require the creation of a platform
> device. Originally, this approach was chosen for simplicity when the
> driver was first implemented.
>
> With the introduction of the lightweight faux device interface, we now
> have a more appropriate alternative. Migrate the driver to utilize the
> faux bus, given that the platform device it previously created was not
> a real one anyway. This will simplify the code, reducing its footprint
> while maintaining functionality.
>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/cpuidle/cpuidle-psci.c | 32 ++++----------------------------
> 1 file changed, 4 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 2562dc001fc1de69732ef28f383d2809262a3d96..5d4d6daed36d8540ba2ce3dc54a3180731b03d22 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -16,7 +16,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> -#include <linux/platform_device.h>
> +#include <linux/device/faux.h>
> #include <linux/psci.h>
> #include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
> @@ -404,14 +404,14 @@ static int psci_idle_init_cpu(struct device *dev, int cpu)
> * to register cpuidle driver then rollback to cancel all CPUs
> * registration.
> */
> -static int psci_cpuidle_probe(struct platform_device *pdev)
> +static int psci_cpuidle_probe(struct faux_device *fdev)
> {
> int cpu, ret;
> struct cpuidle_driver *drv;
> struct cpuidle_device *dev;
>
> for_each_possible_cpu(cpu) {
> - ret = psci_idle_init_cpu(&pdev->dev, cpu);
> + ret = psci_idle_init_cpu(&fdev->dev, cpu);
> if (ret)
> goto out_fail;
> }
> @@ -431,28 +431,4 @@ static int psci_cpuidle_probe(struct platform_device *pdev)
> return ret;
> }
>
> -static struct platform_driver psci_cpuidle_driver = {
> - .probe = psci_cpuidle_probe,
> - .driver = {
> - .name = "psci-cpuidle",
> - },
> -};
> -
> -static int __init psci_idle_init(void)
> -{
> - struct platform_device *pdev;
> - int ret;
> -
> - ret = platform_driver_register(&psci_cpuidle_driver);
> - if (ret)
> - return ret;
> -
> - pdev = platform_device_register_simple("psci-cpuidle", -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - platform_driver_unregister(&psci_cpuidle_driver);
> - return PTR_ERR(pdev);
> - }
> -
> - return 0;
> -}
> -device_initcall(psci_idle_init);
> +module_faux_driver(psci_cpuidle, psci_cpuidle_probe, NULL, true);
>
I have noticed the following error messages on some of our Tegra devices ...
ERR KERN faux psci-cpuidle: probe did not succeed, tearing down the device
ERR KERN CPUidle PSCI: Failed to create psci-cpuidle device
I had a quick look at this and this occurs because of the following code
in the probe cpuidle-psci driver ...
/*
* If no DT idle states are detected (ret == 0) let the driver
* initialization fail accordingly since there is no reason to
* initialize the idle driver if only wfi is supported, the
* default archictectural back-end already executes wfi
* on idle entry.
*/
ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
if (ret <= 0)
return ret ? : -ENODEV;
So although it could be argued that the error message is valid, I am not
sure if there is anything that mandates that we need to have the
idle-states present.
We are always checking for new kernel errors and so if something new
occurs, I am trying to figure out what is the correct way to fix. For
this case I am not sure what is best.
Thanks
Jon
--
nvpublic
On Thu, May 01, 2025 at 02:01:19PM +0100, Jon Hunter wrote:
> Hi Sudeep,
>
> On 18/03/2025 17:01, Sudeep Holla wrote:
> > The PSCI cpuidle driver does not require the creation of a platform
> > device. Originally, this approach was chosen for simplicity when the
> > driver was first implemented.
> >
> > With the introduction of the lightweight faux device interface, we now
> > have a more appropriate alternative. Migrate the driver to utilize the
> > faux bus, given that the platform device it previously created was not
> > a real one anyway. This will simplify the code, reducing its footprint
> > while maintaining functionality.
> >
> > Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > drivers/cpuidle/cpuidle-psci.c | 32 ++++----------------------------
> > 1 file changed, 4 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 2562dc001fc1de69732ef28f383d2809262a3d96..5d4d6daed36d8540ba2ce3dc54a3180731b03d22 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -16,7 +16,7 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > -#include <linux/platform_device.h>
> > +#include <linux/device/faux.h>
> > #include <linux/psci.h>
> > #include <linux/pm_domain.h>
> > #include <linux/pm_runtime.h>
> > @@ -404,14 +404,14 @@ static int psci_idle_init_cpu(struct device *dev, int cpu)
> > * to register cpuidle driver then rollback to cancel all CPUs
> > * registration.
> > */
> > -static int psci_cpuidle_probe(struct platform_device *pdev)
> > +static int psci_cpuidle_probe(struct faux_device *fdev)
> > {
> > int cpu, ret;
> > struct cpuidle_driver *drv;
> > struct cpuidle_device *dev;
> > for_each_possible_cpu(cpu) {
> > - ret = psci_idle_init_cpu(&pdev->dev, cpu);
> > + ret = psci_idle_init_cpu(&fdev->dev, cpu);
> > if (ret)
> > goto out_fail;
> > }
> > @@ -431,28 +431,4 @@ static int psci_cpuidle_probe(struct platform_device *pdev)
> > return ret;
> > }
> > -static struct platform_driver psci_cpuidle_driver = {
> > - .probe = psci_cpuidle_probe,
> > - .driver = {
> > - .name = "psci-cpuidle",
> > - },
> > -};
> > -
> > -static int __init psci_idle_init(void)
> > -{
> > - struct platform_device *pdev;
> > - int ret;
> > -
> > - ret = platform_driver_register(&psci_cpuidle_driver);
> > - if (ret)
> > - return ret;
> > -
> > - pdev = platform_device_register_simple("psci-cpuidle", -1, NULL, 0);
> > - if (IS_ERR(pdev)) {
> > - platform_driver_unregister(&psci_cpuidle_driver);
> > - return PTR_ERR(pdev);
> > - }
> > -
> > - return 0;
> > -}
> > -device_initcall(psci_idle_init);
> > +module_faux_driver(psci_cpuidle, psci_cpuidle_probe, NULL, true);
> >
>
>
> I have noticed the following error messages on some of our Tegra devices ...
>
> ERR KERN faux psci-cpuidle: probe did not succeed, tearing down the device
> ERR KERN CPUidle PSCI: Failed to create psci-cpuidle device
>
> I had a quick look at this and this occurs because of the following code in
> the probe cpuidle-psci driver ...
>
> /*
> * If no DT idle states are detected (ret == 0) let the driver
> * initialization fail accordingly since there is no reason to
> * initialize the idle driver if only wfi is supported, the
> * default archictectural back-end already executes wfi
> * on idle entry.
> */
> ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
> if (ret <= 0)
> return ret ? : -ENODEV;
>
>
> So although it could be argued that the error message is valid, I am not
> sure if there is anything that mandates that we need to have the idle-states
> present.
>
> We are always checking for new kernel errors and so if something new occurs,
> I am trying to figure out what is the correct way to fix. For this case I am
> not sure what is best.
>
This is another case where probe was failing before too just that faux
device probe throws the error. I will take a look and see what can be done.
But yes, we shouldn't throw error if no idle-states are present in the DT.
--
Regards,
Sudeep
On 01/05/2025 17:07, Sudeep Holla wrote: ... >> I have noticed the following error messages on some of our Tegra devices ... >> >> ERR KERN faux psci-cpuidle: probe did not succeed, tearing down the device >> ERR KERN CPUidle PSCI: Failed to create psci-cpuidle device >> >> I had a quick look at this and this occurs because of the following code in >> the probe cpuidle-psci driver ... >> >> /* >> * If no DT idle states are detected (ret == 0) let the driver >> * initialization fail accordingly since there is no reason to >> * initialize the idle driver if only wfi is supported, the >> * default archictectural back-end already executes wfi >> * on idle entry. >> */ >> ret = dt_init_idle_driver(drv, psci_idle_state_match, 1); >> if (ret <= 0) >> return ret ? : -ENODEV; >> >> >> So although it could be argued that the error message is valid, I am not >> sure if there is anything that mandates that we need to have the idle-states >> present. >> >> We are always checking for new kernel errors and so if something new occurs, >> I am trying to figure out what is the correct way to fix. For this case I am >> not sure what is best. >> > > This is another case where probe was failing before too just that faux > device probe throws the error. I will take a look and see what can be done. > But yes, we shouldn't throw error if no idle-states are present in the DT. Yes exactly this was already failing. Thanks for taking a look! Cheers Jon -- nvpublic
On Tue, Mar 18, 2025 at 05:01:40PM +0000, Sudeep Holla wrote:
> The PSCI cpuidle driver does not require the creation of a platform
> device. Originally, this approach was chosen for simplicity when the
> driver was first implemented.
>
> With the introduction of the lightweight faux device interface, we now
> have a more appropriate alternative. Migrate the driver to utilize the
> faux bus, given that the platform device it previously created was not
> a real one anyway. This will simplify the code, reducing its footprint
> while maintaining functionality.
>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/cpuidle/cpuidle-psci.c | 32 ++++----------------------------
> 1 file changed, 4 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 2562dc001fc1de69732ef28f383d2809262a3d96..5d4d6daed36d8540ba2ce3dc54a3180731b03d22 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -16,7 +16,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> -#include <linux/platform_device.h>
> +#include <linux/device/faux.h>
> #include <linux/psci.h>
> #include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
> @@ -404,14 +404,14 @@ static int psci_idle_init_cpu(struct device *dev, int cpu)
> * to register cpuidle driver then rollback to cancel all CPUs
> * registration.
> */
> -static int psci_cpuidle_probe(struct platform_device *pdev)
> +static int psci_cpuidle_probe(struct faux_device *fdev)
> {
> int cpu, ret;
> struct cpuidle_driver *drv;
> struct cpuidle_device *dev;
>
> for_each_possible_cpu(cpu) {
> - ret = psci_idle_init_cpu(&pdev->dev, cpu);
> + ret = psci_idle_init_cpu(&fdev->dev, cpu);
> if (ret)
> goto out_fail;
> }
> @@ -431,28 +431,4 @@ static int psci_cpuidle_probe(struct platform_device *pdev)
> return ret;
> }
>
> -static struct platform_driver psci_cpuidle_driver = {
> - .probe = psci_cpuidle_probe,
> - .driver = {
> - .name = "psci-cpuidle",
> - },
> -};
> -
> -static int __init psci_idle_init(void)
> -{
> - struct platform_device *pdev;
> - int ret;
> -
> - ret = platform_driver_register(&psci_cpuidle_driver);
> - if (ret)
> - return ret;
> -
> - pdev = platform_device_register_simple("psci-cpuidle", -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - platform_driver_unregister(&psci_cpuidle_driver);
> - return PTR_ERR(pdev);
> - }
> -
> - return 0;
> -}
> -device_initcall(psci_idle_init);
> +module_faux_driver(psci_cpuidle, psci_cpuidle_probe, NULL, true);
See, what does "true" mean here?
Why would you ever want "false"?
thanks,
greg k-h
On Tue, Apr 15, 2025 at 02:21:33PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Mar 18, 2025 at 05:01:40PM +0000, Sudeep Holla wrote:
> > The PSCI cpuidle driver does not require the creation of a platform
> > device. Originally, this approach was chosen for simplicity when the
> > driver was first implemented.
> >
> > With the introduction of the lightweight faux device interface, we now
> > have a more appropriate alternative. Migrate the driver to utilize the
> > faux bus, given that the platform device it previously created was not
> > a real one anyway. This will simplify the code, reducing its footprint
> > while maintaining functionality.
> >
> > Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > drivers/cpuidle/cpuidle-psci.c | 32 ++++----------------------------
> > 1 file changed, 4 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 2562dc001fc1de69732ef28f383d2809262a3d96..5d4d6daed36d8540ba2ce3dc54a3180731b03d22 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -16,7 +16,7 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > -#include <linux/platform_device.h>
> > +#include <linux/device/faux.h>
> > #include <linux/psci.h>
> > #include <linux/pm_domain.h>
> > #include <linux/pm_runtime.h>
> > @@ -404,14 +404,14 @@ static int psci_idle_init_cpu(struct device *dev, int cpu)
> > * to register cpuidle driver then rollback to cancel all CPUs
> > * registration.
> > */
> > -static int psci_cpuidle_probe(struct platform_device *pdev)
> > +static int psci_cpuidle_probe(struct faux_device *fdev)
> > {
> > int cpu, ret;
> > struct cpuidle_driver *drv;
> > struct cpuidle_device *dev;
> >
> > for_each_possible_cpu(cpu) {
> > - ret = psci_idle_init_cpu(&pdev->dev, cpu);
> > + ret = psci_idle_init_cpu(&fdev->dev, cpu);
> > if (ret)
> > goto out_fail;
> > }
> > @@ -431,28 +431,4 @@ static int psci_cpuidle_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > -static struct platform_driver psci_cpuidle_driver = {
> > - .probe = psci_cpuidle_probe,
> > - .driver = {
> > - .name = "psci-cpuidle",
> > - },
> > -};
> > -
> > -static int __init psci_idle_init(void)
> > -{
> > - struct platform_device *pdev;
> > - int ret;
> > -
> > - ret = platform_driver_register(&psci_cpuidle_driver);
> > - if (ret)
> > - return ret;
> > -
> > - pdev = platform_device_register_simple("psci-cpuidle", -1, NULL, 0);
> > - if (IS_ERR(pdev)) {
> > - platform_driver_unregister(&psci_cpuidle_driver);
> > - return PTR_ERR(pdev);
> > - }
> > -
> > - return 0;
> > -}
> > -device_initcall(psci_idle_init);
> > +module_faux_driver(psci_cpuidle, psci_cpuidle_probe, NULL, true);
>
> See, what does "true" mean here?
>
> Why would you ever want "false"?
>
There were few efi platform devices that were created conditionally and
the idea with this true/false was to pass that condition. I agree it was
not clean. Anyways since efi platform devices can't be moved to faux
devices, this flag becomes useless as it is most true for all other users.
Also as mention in the other thread, the need for macro also become very
weak as efi devices can't be moved into faux.
So all the patches in v1 except efi and trng are now queued via respective
trees using faux device apis directly without this weird macro 😄.
--
Regards,
Sudeep
© 2016 - 2025 Red Hat, Inc.