[PATCH RFC] power: reset: restart-poweroff: convert to module

[PATCH RFC] power: reset: restart-poweroff: convert to module

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

The necessity of having a fake platform device for a generic, platform
independent functionality is not obvious.
Some platforms requre device tree modification for this, some would require
ACPI tables modification, while functionality may be useful even to
end-users without required expertise. Convert the platform driver to
a simple module.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
This RFC is merely to understand if this approach would be accepted.
Converting to "tristate" could follow or preceed this patch.

diff --git a/drivers/power/reset/restart-poweroff.c b/drivers/power/reset/restart-poweroff.c
index 28f1822db1626..e1d94109f6823 100644
--- a/drivers/power/reset/restart-poweroff.c
+++ b/drivers/power/reset/restart-poweroff.c
@@ -20,7 +20,7 @@ static void restart_poweroff_do_poweroff(void)
 	machine_restart(NULL);
 }
 
-static int restart_poweroff_probe(struct platform_device *pdev)
+static int __init restart_poweroff_init(void)
 {
 	/* If a pm_power_off function has already been added, leave it alone */
 	if (pm_power_off != NULL) {
@@ -33,12 +33,10 @@ static int restart_poweroff_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int restart_poweroff_remove(struct platform_device *pdev)
+static void __exit restart_poweroff_exit(void)
 {
 	if (pm_power_off == &restart_poweroff_do_poweroff)
 		pm_power_off = NULL;
-
-	return 0;
 }
 
 static const struct of_device_id of_restart_poweroff_match[] = {
@@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_restart_poweroff_match);
 
-static struct platform_driver restart_poweroff_driver = {
-	.probe = restart_poweroff_probe,
-	.remove = restart_poweroff_remove,
-	.driver = {
-		.name = "poweroff-restart",
-		.of_match_table = of_restart_poweroff_match,
-	},
-};
-module_platform_driver(restart_poweroff_driver);
+module_init(restart_poweroff_init);
+module_exit(restart_poweroff_exit);
 
 MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch");
 MODULE_DESCRIPTION("restart poweroff driver");
-- 
2.43.0

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3006 bytes --]

Hi,

On Wed, Feb 21, 2024 at 06:46:07PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> The necessity of having a fake platform device for a generic, platform
> independent functionality is not obvious.
> Some platforms requre device tree modification for this, some would require
> ACPI tables modification, while functionality may be useful even to
> end-users without required expertise. Convert the platform driver to
> a simple module.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
> This RFC is merely to understand if this approach would be accepted.
> Converting to "tristate" could follow or preceed this patch.

1. You cannot easily make this tristate, because of machine_restart().
2. This is already using module_platform_driver(), so this has
   nothing to do with making it a module like the subject suggests.
3. This no longer applies, since the driver is now properly using
   devm_register_sys_off_handler instead of pm_power_off.
4. It's intentional, that a device needs to be described. This is
   _not_ meant as a general purpose poweroff driver. It's intended
   to be used with bootloader support, which keeps the system off
   as described in the comment at the start of the file.

So: NAK

-- Sebastian

> diff --git a/drivers/power/reset/restart-poweroff.c b/drivers/power/reset/restart-poweroff.c
> index 28f1822db1626..e1d94109f6823 100644
> --- a/drivers/power/reset/restart-poweroff.c
> +++ b/drivers/power/reset/restart-poweroff.c
> @@ -20,7 +20,7 @@ static void restart_poweroff_do_poweroff(void)
>  	machine_restart(NULL);
>  }
>  
> -static int restart_poweroff_probe(struct platform_device *pdev)
> +static int __init restart_poweroff_init(void)
>  {
>  	/* If a pm_power_off function has already been added, leave it alone */
>  	if (pm_power_off != NULL) {
> @@ -33,12 +33,10 @@ static int restart_poweroff_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int restart_poweroff_remove(struct platform_device *pdev)
> +static void __exit restart_poweroff_exit(void)
>  {
>  	if (pm_power_off == &restart_poweroff_do_poweroff)
>  		pm_power_off = NULL;
> -
> -	return 0;
>  }
>  
>  static const struct of_device_id of_restart_poweroff_match[] = {
> @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, of_restart_poweroff_match);
>  
> -static struct platform_driver restart_poweroff_driver = {
> -	.probe = restart_poweroff_probe,
> -	.remove = restart_poweroff_remove,
> -	.driver = {
> -		.name = "poweroff-restart",
> -		.of_match_table = of_restart_poweroff_match,
> -	},
> -};
> -module_platform_driver(restart_poweroff_driver);
> +module_init(restart_poweroff_init);
> +module_exit(restart_poweroff_exit);
>  
>  MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch");
>  MODULE_DESCRIPTION("restart poweroff driver");
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Andrew Lunn]

On Wed, Feb 21, 2024 at 06:46:07PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> The necessity of having a fake platform device for a generic, platform
> independent functionality is not obvious.
> Some platforms requre device tree modification for this, some would require
> ACPI tables modification, while functionality may be useful even to
> end-users without required expertise. Convert the platform driver to
> a simple module.

> @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, of_restart_poweroff_match);
>  
> -static struct platform_driver restart_poweroff_driver = {
> -	.probe = restart_poweroff_probe,
> -	.remove = restart_poweroff_remove,
> -	.driver = {
> -		.name = "poweroff-restart",
> -		.of_match_table = of_restart_poweroff_match,
> -	},
> -};

of_restart_poweroff_match now seems to be disconnected from the
driver.

kirkwood-linkstation.dtsi:		compatible = "restart-poweroff";
kirkwood-lsxl.dtsi:		compatible = "restart-poweroff";
orion5x-linkstation.dtsi:		compatible = "restart-poweroff";
orion5x-lswsgl.dts:		compatible = "restart-poweroff";

How do these devices get this driver loaded?

This appears to be another reason to NACK it.

	Andrew

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Andrew Lunn]

On Wed, Feb 21, 2024 at 06:46:07PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> The necessity of having a fake platform device for a generic, platform
> independent functionality is not obvious.
> Some platforms requre device tree modification for this, some would require
> ACPI tables modification, while functionality may be useful even to
> end-users without required expertise. Convert the platform driver to
> a simple module.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
> This RFC is merely to understand if this approach would be accepted.
> Converting to "tristate" could follow or preceed this patch.

So that is you use case here? Why do you want to be able to just load
this driver, without using DT to indicate it is needed by the
hardware?

    Andrew

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Sverdlin, Alexander]

On Wed, 2024-02-21 at 22:56 +0100, Andrew Lunn wrote:
> > @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = {
> >   };
> >   MODULE_DEVICE_TABLE(of, of_restart_poweroff_match);
> >   
> > -static struct platform_driver restart_poweroff_driver = {
> > -       .probe = restart_poweroff_probe,
> > -       .remove = restart_poweroff_remove,
> > -       .driver = {
> > -               .name = "poweroff-restart",
> > -               .of_match_table = of_restart_poweroff_match,
> > -       },
> > -};
> 
> of_restart_poweroff_match now seems to be disconnected from the
> driver.
> 
> kirkwood-linkstation.dtsi:              compatible = "restart-poweroff";
> kirkwood-lsxl.dtsi:             compatible = "restart-poweroff";
> orion5x-linkstation.dtsi:               compatible = "restart-poweroff";
> orion5x-lswsgl.dts:             compatible = "restart-poweroff";
> 
> How do these devices get this driver loaded?
> 
> This appears to be another reason to NACK it.

That's why MODULE_DEVICE_TABLE() was preserved for backwards compatibility,
because *loading* happens via MODULE_DEVICE_TABLE(). But I didn't realize
it was never buildable as module as Sebastian pointed out, because of
machine_restart().

For your use case it would continue to work as before I believe, just
the callback would be installed because of the fact the code
was compiled-in, not because there was a fake platform device.

I also didn't understand what is so special about bootloader support
for this functionality if no data is passed to the bootloader.
After ARM-specifics was removed from the code quite some time ago
any platform could re-use the code for the deployments which meant
to be "always on".

But If the resistance is so serious, so be it.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Sverdlin, Alexander]

Hi Andrew!

On Wed, 2024-02-21 at 23:37 +0100, Andrew Lunn wrote:
> > The necessity of having a fake platform device for a generic, platform
> > independent functionality is not obvious.
> > Some platforms requre device tree modification for this, some would require
> > ACPI tables modification, while functionality may be useful even to
> > end-users without required expertise. Convert the platform driver to
> > a simple module.
> > 
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> > This RFC is merely to understand if this approach would be accepted.
> > Converting to "tristate" could follow or preceed this patch.
> 
> So that is you use case here? Why do you want to be able to just load
> this driver, without using DT to indicate it is needed by the
> hardware?

Yes, the code is platform-independent now and can be re-used for deployments
which meant to be "always on". One could actually even use it with
off-the-shelf x86 hardware. But instantiating a platform device there would
be a hack. Why not just control this code in the kernel config?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Andrew Lunn]

> I also didn't understand what is so special about bootloader support
> for this functionality if no data is passed to the bootloader.
> After ARM-specifics was removed from the code quite some time ago
> any platform could re-use the code for the deployments which meant
> to be "always on".

If i'm remembering correctly....

When the box cold boots, the bootloader can see the boot reason, and
chains into Linux. When the user does a shutdown, we re-enter the
bootloader, and it sees it is a warm boot. It then spins, rather than
chaining into Linux. The process of getting into the bootloader, a
watchdog triggered reset, probably disables a lot of clocks and other
bits of hardware, so spinning in the bootloader consumes less power
than if Linux was to spin.

So bootloader needs this functionality, to either chain, or to spin.

   Andrew

Re: [PATCH RFC] power: reset: restart-poweroff: convert to module

[Andrew Lunn]

> Yes, the code is platform-independent now and can be re-used for deployments
> which meant to be "always on".

You need to be careful with the meaning of "always on". It is always
on in that the hardware does not have any PMICs. It is impossible to
turn the power off. This is a poweroff driver, and it powers the
hardware off by dropping into the bootloader which then spins.

> One could actually even use it with off-the-shelf x86 hardware.

Is there off the shelf x86 which does not support turning the power
off? I'm not familiar with x86 that much, but it seems to be a feature
that has existed since the first IBM PC in 1980.

     Andrew