mbox series

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

Message ID 20190930103531.13764-1-oleksandr.suvorov@toradex.com
Headers show
Series This patch introduces a feature to force gpio-poweroff module | expand

Message

Oleksandr Suvorov Sept. 30, 2019, 10:35 a.m. UTC 
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(-)

Comments

Andrew Lunn Sept. 30, 2019, 12:14 p.m. UTC | #1 
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
Oleksandr Suvorov Sept. 30, 2019, 2:11 p.m. UTC | #2 
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
Andrew Lunn Sept. 30, 2019, 4:32 p.m. UTC | #3 
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
Jamie Lentin Sept. 30, 2019, 7:42 p.m. UTC | #4 
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
>