| Message ID | 20190930103531.13764-1-oleksandr.suvorov@toradex.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | This patch introduces a feature to force gpio-poweroff module | expand |
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
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
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
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 >