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@xxxxxxx> 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