[PATCH 0/2] This patch introduces a feature to force gpio-poweroff module

[PATCH 0/2] This patch introduces a feature to force gpio-poweroff module

[Oleksandr Suvorov]

to register its own pm_power_off handler even if someone has registered
this handler earlier.
Useful to change a way to power off the system using DT files.


Oleksandr Suvorov (2):
  power: reset: gpio-poweroff: add force mode
  dt-bindings: power: reset: gpio-poweroff: Add 'force-mode' property

 .../bindings/power/reset/gpio-poweroff.txt    |  3 +++
 drivers/power/reset/gpio-poweroff.c           | 27 ++++++++++++++-----
 2 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/2] power: reset: gpio-poweroff: add force mode

[Oleksandr Suvorov]

Property "force-mode" tells the driver to replace previously
initialized power-off kernel hook and allows gpio-poweroff to
probe and operate successfully in any case.

Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
---

 drivers/power/reset/gpio-poweroff.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/power/reset/gpio-poweroff.c b/drivers/power/reset/gpio-poweroff.c
index 6a4bbb506551..cca9f36fdce6 100644
--- a/drivers/power/reset/gpio-poweroff.c
+++ b/drivers/power/reset/gpio-poweroff.c
@@ -24,6 +24,7 @@ static struct gpio_desc *reset_gpio;
 static u32 timeout = DEFAULT_TIMEOUT_MS;
 static u32 active_delay = 100;
 static u32 inactive_delay = 100;
+static void *prev_pm_power_off;
 
 static void gpio_poweroff_do_poweroff(void)
 {
@@ -49,14 +50,28 @@ static void gpio_poweroff_do_poweroff(void)
 static int gpio_poweroff_probe(struct platform_device *pdev)
 {
 	bool input = false;
+	bool force = false;
 	enum gpiod_flags flags;
 
-	/* If a pm_power_off function has already been added, leave it alone */
+
+	force = device_property_read_bool(&pdev->dev, "force-mode");
+
+	/*
+	 * If a pm_power_off function has already been added, leave it alone,
+	 * if force-mode is not enabled.
+	 */
 	if (pm_power_off != NULL) {
-		dev_err(&pdev->dev,
-			"%s: pm_power_off function already registered",
-		       __func__);
-		return -EBUSY;
+		if (force) {
+			dev_warn(&pdev->dev,
+				 "%s: pm_power_off function replaced",
+				 __func__);
+			prev_pm_power_off = pm_power_off;
+		} else {
+			dev_err(&pdev->dev,
+				"%s: pm_power_off function already registered",
+				__func__);
+			return -EBUSY;
+		}
 	}
 
 	input = device_property_read_bool(&pdev->dev, "input");
@@ -81,7 +96,7 @@ static int gpio_poweroff_probe(struct platform_device *pdev)
 static int gpio_poweroff_remove(struct platform_device *pdev)
 {
 	if (pm_power_off == &gpio_poweroff_do_poweroff)
-		pm_power_off = NULL;
+		pm_power_off = prev_pm_power_off;
 
 	return 0;
 }
-- 
2.20.1

[PATCH 2/2] dt-bindings: power: reset: gpio-poweroff: Add 'force-mode' property

[Oleksandr Suvorov]

Add 'force-mode' property to allow the driver to load even if
someone has registered the pm_power_off hook earlier.

Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

---

 .../devicetree/bindings/power/reset/gpio-poweroff.txt          | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt b/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt
index 3e56c1b34a4c..2056e299a472 100644
--- a/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt
+++ b/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt
@@ -31,6 +31,9 @@ Optional properties:
 - inactive-delay-ms: Delay (default 100) to wait after driving gpio inactive
 - timeout-ms: Time to wait before asserting a WARN_ON(1). If nothing is
               specified, 3000 ms is used.
+- force-mode: Force replacing pm_power_off kernel hook.
+  If this optional property is not specified, the driver will fail to
+  load if someone has registered the pm_power_off hook earlier.
 
 Examples:
 
-- 
2.20.1

Re: [PATCH 0/2] This patch introduces a feature to force gpio-poweroff module

[Andrew Lunn]

On Mon, Sep 30, 2019 at 10:35:36AM +0000, Oleksandr Suvorov wrote:
> to register its own pm_power_off handler even if someone has registered
> this handler earlier.
> Useful to change a way to power off the system using DT files.

Hi Oleksandr

I'm not sure this is a good idea. What happens when there are two
drivers using forced mode? You then get which ever is register last.
Non deterministic behaviour.

What is the other driver which is causing you problems? How is it
getting probed? DT?

Thanks
	Andrew

Re: [PATCH 0/2] This patch introduces a feature to force gpio-poweroff module

[Oleksandr Suvorov]

Hi Andrew,

On Mon, Sep 30, 2019 at 3:16 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Sep 30, 2019 at 10:35:36AM +0000, Oleksandr Suvorov wrote:
> > to register its own pm_power_off handler even if someone has registered
> > this handler earlier.
> > Useful to change a way to power off the system using DT files.
>
> Hi Oleksandr
>
> I'm not sure this is a good idea. What happens when there are two
> drivers using forced mode? You then get which ever is register last.
> Non deterministic behaviour.

You're right, we have to handle a case when gpio-poweroff fails to
power the system off. Please look at the
2nd version of the patchset.

There are 3 only drivers that forcibly register its own pm_power_off
handler even if it has been registered before.

drivers/firmware/efi/reboot.c - supports chained call of next
pm_power_off handler if its own handler fails.

arch/x86/platform/iris/iris.c, drivers/char/ipmi/ipmi_poweroff.c -
don't support calling of next pm_power_off handler.
Looks like these drivers should be fixed too.

All other drivers don't change already initialized pm_power_off handler.

> What is the other driver which is causing you problems? How is it
> getting probed? DT?

There are several PMUs, RTCs, watchdogs that register their own pm_power_off.
Most of them, probably not all, are probed from DT.

>
> Thanks
>         Andrew

-- 
Best regards
Oleksandr Suvorov

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)

Re: [PATCH 0/2] This patch introduces a feature to force gpio-poweroff module

[Andrew Lunn]

On Mon, Sep 30, 2019 at 02:11:59PM +0000, Oleksandr Suvorov wrote:
> Hi Andrew,
> 
> On Mon, Sep 30, 2019 at 3:16 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Sep 30, 2019 at 10:35:36AM +0000, Oleksandr Suvorov wrote:
> > > to register its own pm_power_off handler even if someone has registered
> > > this handler earlier.
> > > Useful to change a way to power off the system using DT files.
> >
> > Hi Oleksandr
> >
> > I'm not sure this is a good idea. What happens when there are two
> > drivers using forced mode? You then get which ever is register last.
> > Non deterministic behaviour.
> 
> You're right, we have to handle a case when gpio-poweroff fails to
> power the system off. Please look at the
> 2nd version of the patchset.
> 
> There are 3 only drivers that forcibly register its own pm_power_off
> handler even if it has been registered before.
> 
> drivers/firmware/efi/reboot.c - supports chained call of next
> pm_power_off handler if its own handler fails.
> 
> arch/x86/platform/iris/iris.c, drivers/char/ipmi/ipmi_poweroff.c -
> don't support calling of next pm_power_off handler.
> Looks like these drivers should be fixed too.
> 
> All other drivers don't change already initialized pm_power_off handler.
> 
> > What is the other driver which is causing you problems? How is it
> > getting probed? DT?
> 
> There are several PMUs, RTCs, watchdogs that register their own pm_power_off.
> Most of them, probably not all, are probed from DT.

And which specific one is causing you problems.

I don't like this forced parameter. No other driver is using it.

Maybe we should change this driver to support chained pm_power_off
handlers?

   Andrew

Re: [PATCH 0/2] This patch introduces a feature to force gpio-poweroff module

[Jamie Lentin]

On Mon, 30 Sep 2019, Andrew Lunn wrote:

> On Mon, Sep 30, 2019 at 02:11:59PM +0000, Oleksandr Suvorov wrote:
>> Hi Andrew,
>>
>> On Mon, Sep 30, 2019 at 3:16 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>>
>>> On Mon, Sep 30, 2019 at 10:35:36AM +0000, Oleksandr Suvorov wrote:
>>>> to register its own pm_power_off handler even if someone has registered
>>>> this handler earlier.
>>>> Useful to change a way to power off the system using DT files.
>>>
>>> Hi Oleksandr
>>>
>>> I'm not sure this is a good idea. What happens when there are two
>>> drivers using forced mode? You then get which ever is register last.
>>> Non deterministic behaviour.
>>
>> You're right, we have to handle a case when gpio-poweroff fails to
>> power the system off. Please look at the
>> 2nd version of the patchset.
>>
>> There are 3 only drivers that forcibly register its own pm_power_off
>> handler even if it has been registered before.
>>
>> drivers/firmware/efi/reboot.c - supports chained call of next
>> pm_power_off handler if its own handler fails.
>>
>> arch/x86/platform/iris/iris.c, drivers/char/ipmi/ipmi_poweroff.c -
>> don't support calling of next pm_power_off handler.
>> Looks like these drivers should be fixed too.
>>
>> All other drivers don't change already initialized pm_power_off handler.
>>
>>> What is the other driver which is causing you problems? How is it
>>> getting probed? DT?
>>
>> There are several PMUs, RTCs, watchdogs that register their own pm_power_off.
>> Most of them, probably not all, are probed from DT.
>
> And which specific one is causing you problems.
>
> I don't like this forced parameter. No other driver is using it.
>
> Maybe we should change this driver to support chained pm_power_off
> handlers?

There's still scope for non-deterministic behaviour though, as the 
chaining would take place depending on the probe ordering. Admittedly if 
the gpio-poweroff works it's unlikely to be a problem, but still seems 
messy.

Without knowing specifics, disabling the devices that can't turn the 
device off seems like a better bet. If they'd be otherwise useful, I see 
there's a of_device_is_system_power_controller(), see:

/Documentation/devicetree/bindings/power/power-controller.txt
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/max77620.c#L566

...maybe that can be added to the devices getting in the way?

Cheers,

[0] https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/bcm2835_wdt.c#L152
(chosen at random)


>
>   Andrew
>

-- 
Jamie Lentin