| Message ID | 20211027211715.12671-28-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce power-off+restart call chain API | expand |
Hi, On Thu, Oct 28, 2021 at 12:16:57AM +0300, Dmitry Osipenko wrote: > Use devm_register_power_handler() that replaces global pm_power_off > variable and allows to register multiple power-off handlers. It also > provides restart-handler support, i.e. all in one API. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- When I boot with (most of) this patchset applied, I get the warning at kernel/reboot.c:187: /* * Handler must have unique priority. Otherwise call order is * determined by registration order, which is unreliable. */ WARN_ON(!atomic_notifier_has_unique_priority(&restart_handler_list, nb)); As the NTXEC driver doesn't specify a priority, I think this is an issue to be fixed elsewhere. Other than that, it works and looks good, as far as I can tell. For this patch: Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> Tested-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> Best regards, Jonathan --- Full Oops log: [ 3.523294] ------------[ cut here ]------------ [ 3.528193] WARNING: CPU: 0 PID: 1 at kernel/reboot.c:187 register_restart_handler+0x4c/0x58 [ 3.536975] Modules linked in: [ 3.540312] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.0-00021-gcb24c628b307 #622 [ 3.548214] Hardware name: Freescale i.MX50 (Device Tree Support) [ 3.554357] [<c0111540>] (unwind_backtrace) from [<c010cdd0>] (show_stack+0x10/0x14) [ 3.562183] [<c010cdd0>] (show_stack) from [<c0bf240c>] (dump_stack_lvl+0x58/0x70) [ 3.569824] [<c0bf240c>] (dump_stack_lvl) from [<c0127604>] (__warn+0xd4/0x154) [ 3.577191] [<c0127604>] (__warn) from [<c0bec844>] (warn_slowpath_fmt+0x74/0xa8) [ 3.584727] [<c0bec844>] (warn_slowpath_fmt) from [<c01593c8>] (register_restart_handler+0x4c/0x58) [ 3.593823] [<c01593c8>] (register_restart_handler) from [<c08676c8>] (__watchdog_register_device+0x13c/0x27c) [ 3.603889] [<c08676c8>] (__watchdog_register_device) from [<c0867868>] (watchdog_register_device+0x60/0xb4) [ 3.613764] [<c0867868>] (watchdog_register_device) from [<c08678f8>] (devm_watchdog_register_device+0x3c/0x84) [ 3.623898] [<c08678f8>] (devm_watchdog_register_device) from [<c1146454>] (imx2_wdt_probe+0x254/0x2ac) [ 3.633346] [<c1146454>] (imx2_wdt_probe) from [<c06feb74>] (platform_probe+0x58/0xb8) [ 3.641314] [<c06feb74>] (platform_probe) from [<c06fb2f8>] (call_driver_probe+0x24/0x108) [ 3.649636] [<c06fb2f8>] (call_driver_probe) from [<c06fbe08>] (really_probe.part.0+0xa8/0x358) [ 3.658384] [<c06fbe08>] (really_probe.part.0) from [<c06fc1c4>] (__driver_probe_device+0x94/0x208) [ 3.667470] [<c06fc1c4>] (__driver_probe_device) from [<c06fc368>] (driver_probe_device+0x30/0xc8) [ 3.676468] [<c06fc368>] (driver_probe_device) from [<c06fcb0c>] (__driver_attach+0xe0/0x1c4) [ 3.685032] [<c06fcb0c>] (__driver_attach) from [<c06f9a20>] (bus_for_each_dev+0x74/0xc0) [ 3.693253] [<c06f9a20>] (bus_for_each_dev) from [<c06faeb8>] (bus_add_driver+0x100/0x208) [ 3.701563] [<c06faeb8>] (bus_add_driver) from [<c06fd8a0>] (driver_register+0x88/0x118) [ 3.709696] [<c06fd8a0>] (driver_register) from [<c06fe920>] (__platform_driver_probe+0x44/0xdc) [ 3.718522] [<c06fe920>] (__platform_driver_probe) from [<c01022ac>] (do_one_initcall+0x78/0x388) [ 3.727444] [<c01022ac>] (do_one_initcall) from [<c1101708>] (do_initcalls+0xcc/0x110) [ 3.735413] [<c1101708>] (do_initcalls) from [<c110198c>] (kernel_init_freeable+0x1ec/0x250) [ 3.743896] [<c110198c>] (kernel_init_freeable) from [<c0bfe724>] (kernel_init+0x10/0x128) [ 3.752224] [<c0bfe724>] (kernel_init) from [<c010011c>] (ret_from_fork+0x14/0x38) [ 3.759844] Exception stack(0xc40adfb0 to 0xc40adff8) [ 3.764933] dfa0: 00000000 00000000 00000000 00000000 [ 3.773143] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 3.781351] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 3.788347] irq event stamp: 143613 [ 3.792102] hardirqs last enabled at (143623): [<c01a3ebc>] __up_console_sem+0x50/0x60 [ 3.800397] hardirqs last disabled at (143632): [<c01a3ea8>] __up_console_sem+0x3c/0x60 [ 3.808491] softirqs last enabled at (143612): [<c0101518>] __do_softirq+0x2f8/0x5b0 [ 3.816591] softirqs last disabled at (143603): [<c01307dc>] __irq_exit_rcu+0x160/0x1d8 [ 3.825014] ---[ end trace 7f6709d2c89774b4 ]---
06.11.2021 23:54, Jonathan Neuschäfer пишет: > Hi, > > On Thu, Oct 28, 2021 at 12:16:57AM +0300, Dmitry Osipenko wrote: >> Use devm_register_power_handler() that replaces global pm_power_off >> variable and allows to register multiple power-off handlers. It also >> provides restart-handler support, i.e. all in one API. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- > > When I boot with (most of) this patchset applied, I get the warning at > kernel/reboot.c:187: > > /* > * Handler must have unique priority. Otherwise call order is > * determined by registration order, which is unreliable. > */ > WARN_ON(!atomic_notifier_has_unique_priority(&restart_handler_list, nb)); > > As the NTXEC driver doesn't specify a priority, I think this is an issue > to be fixed elsewhere. > > Other than that, it works and looks good, as far as I can tell. > > > For this patch: > > Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > Tested-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> Thank you. You have conflicting restart handlers, apparently NTXEC driver should have higher priority than the watchdog driver. It should be a common problem for the watchdog drivers, I will lower watchdog's default priority to fix it.
On 11/7/21 8:53 AM, Dmitry Osipenko wrote: > 06.11.2021 23:54, Jonathan Neuschäfer пишет: >> Hi, >> >> On Thu, Oct 28, 2021 at 12:16:57AM +0300, Dmitry Osipenko wrote: >>> Use devm_register_power_handler() that replaces global pm_power_off >>> variable and allows to register multiple power-off handlers. It also >>> provides restart-handler support, i.e. all in one API. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >> >> When I boot with (most of) this patchset applied, I get the warning at >> kernel/reboot.c:187: >> >> /* >> * Handler must have unique priority. Otherwise call order is >> * determined by registration order, which is unreliable. >> */ >> WARN_ON(!atomic_notifier_has_unique_priority(&restart_handler_list, nb)); >> >> As the NTXEC driver doesn't specify a priority, I think this is an issue >> to be fixed elsewhere. >> >> Other than that, it works and looks good, as far as I can tell. >> >> >> For this patch: >> >> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >> Tested-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > Thank you. You have conflicting restart handlers, apparently NTXEC > driver should have higher priority than the watchdog driver. It should > be a common problem for the watchdog drivers, I will lower watchdog's > default priority to fix it. > The watchdog subsystem already uses "0" as default priority, which was intended as priority of last resort for restart handlers. I do not see a reason to change that. Guenter
07.11.2021 20:08, Guenter Roeck пишет: > On 11/7/21 8:53 AM, Dmitry Osipenko wrote: >> 06.11.2021 23:54, Jonathan Neuschäfer пишет: >>> Hi, >>> >>> On Thu, Oct 28, 2021 at 12:16:57AM +0300, Dmitry Osipenko wrote: >>>> Use devm_register_power_handler() that replaces global pm_power_off >>>> variable and allows to register multiple power-off handlers. It also >>>> provides restart-handler support, i.e. all in one API. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>> >>> When I boot with (most of) this patchset applied, I get the warning at >>> kernel/reboot.c:187: >>> >>> /* >>> * Handler must have unique priority. Otherwise call order is >>> * determined by registration order, which is unreliable. >>> */ >>> WARN_ON(!atomic_notifier_has_unique_priority(&restart_handler_list, >>> nb)); >>> >>> As the NTXEC driver doesn't specify a priority, I think this is an issue >>> to be fixed elsewhere. >>> >>> Other than that, it works and looks good, as far as I can tell. >>> >>> >>> For this patch: >>> >>> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >>> Tested-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >> >> Thank you. You have conflicting restart handlers, apparently NTXEC >> driver should have higher priority than the watchdog driver. It should >> be a common problem for the watchdog drivers, I will lower watchdog's >> default priority to fix it. >> > > The watchdog subsystem already uses "0" as default priority, which was > intended as priority of last resort for restart handlers. I do not see > a reason to change that. Right, I meant that watchdog drivers which use restart handler set the level to the default 128 [1]. Although, maybe it's a problem only for i.MX drivers in practice, I'll take a closer look at the other drivers. [1] https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/imx2_wdt.c#L318
On 11/7/21 9:16 AM, Dmitry Osipenko wrote: > 07.11.2021 20:08, Guenter Roeck пишет: >> On 11/7/21 8:53 AM, Dmitry Osipenko wrote: >>> 06.11.2021 23:54, Jonathan Neuschäfer пишет: >>>> Hi, >>>> >>>> On Thu, Oct 28, 2021 at 12:16:57AM +0300, Dmitry Osipenko wrote: >>>>> Use devm_register_power_handler() that replaces global pm_power_off >>>>> variable and allows to register multiple power-off handlers. It also >>>>> provides restart-handler support, i.e. all in one API. >>>>> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> --- >>>> >>>> When I boot with (most of) this patchset applied, I get the warning at >>>> kernel/reboot.c:187: >>>> >>>> /* >>>> * Handler must have unique priority. Otherwise call order is >>>> * determined by registration order, which is unreliable. >>>> */ >>>> WARN_ON(!atomic_notifier_has_unique_priority(&restart_handler_list, >>>> nb)); >>>> >>>> As the NTXEC driver doesn't specify a priority, I think this is an issue >>>> to be fixed elsewhere. >>>> >>>> Other than that, it works and looks good, as far as I can tell. >>>> >>>> >>>> For this patch: >>>> >>>> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >>>> Tested-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >>> >>> Thank you. You have conflicting restart handlers, apparently NTXEC >>> driver should have higher priority than the watchdog driver. It should >>> be a common problem for the watchdog drivers, I will lower watchdog's >>> default priority to fix it. >>> >> >> The watchdog subsystem already uses "0" as default priority, which was >> intended as priority of last resort for restart handlers. I do not see >> a reason to change that. > > Right, I meant that watchdog drivers which use restart handler set the > level to the default 128 [1]. Although, maybe it's a problem only for > i.MX drivers in practice, I'll take a closer look at the other drivers. > They don't have to do that. The default is priority 0. It is the decision of the driver author to set the watchdog's restart priority. So it is wrong to claim that this would be "a common problem for the watchdog drivers", because it isn't. Presumably there was a reason for the driver author to select the default priority of 128. If there is a platform which has a better means to restart the system, it should select a priority of 129 or higher instead of affecting _all_ platforms using the imx watchdog to reset the system. Sure, you can negotiate that with the driver author, but the default should really be to change the priority for less affected platforms. Guenter
07.11.2021 20:32, Guenter Roeck пишет: > On 11/7/21 9:16 AM, Dmitry Osipenko wrote: >> 07.11.2021 20:08, Guenter Roeck пишет: >>> On 11/7/21 8:53 AM, Dmitry Osipenko wrote: >>>> 06.11.2021 23:54, Jonathan Neuschäfer пишет: >>>>> Hi, >>>>> >>>>> On Thu, Oct 28, 2021 at 12:16:57AM +0300, Dmitry Osipenko wrote: >>>>>> Use devm_register_power_handler() that replaces global pm_power_off >>>>>> variable and allows to register multiple power-off handlers. It also >>>>>> provides restart-handler support, i.e. all in one API. >>>>>> >>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>> --- >>>>> >>>>> When I boot with (most of) this patchset applied, I get the warning at >>>>> kernel/reboot.c:187: >>>>> >>>>> /* >>>>> * Handler must have unique priority. Otherwise call order is >>>>> * determined by registration order, which is unreliable. >>>>> */ >>>>> WARN_ON(!atomic_notifier_has_unique_priority(&restart_handler_list, >>>>> >>>>> nb)); >>>>> >>>>> As the NTXEC driver doesn't specify a priority, I think this is an >>>>> issue >>>>> to be fixed elsewhere. >>>>> >>>>> Other than that, it works and looks good, as far as I can tell. >>>>> >>>>> >>>>> For this patch: >>>>> >>>>> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >>>>> Tested-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >>>> >>>> Thank you. You have conflicting restart handlers, apparently NTXEC >>>> driver should have higher priority than the watchdog driver. It should >>>> be a common problem for the watchdog drivers, I will lower watchdog's >>>> default priority to fix it. >>>> >>> >>> The watchdog subsystem already uses "0" as default priority, which was >>> intended as priority of last resort for restart handlers. I do not see >>> a reason to change that. >> >> Right, I meant that watchdog drivers which use restart handler set the >> level to the default 128 [1]. Although, maybe it's a problem only for >> i.MX drivers in practice, I'll take a closer look at the other drivers. >> > > They don't have to do that. The default is priority 0. It is the decision > of the driver author to set the watchdog's restart priority. So it is wrong > to claim that this would be "a common problem for the watchdog drivers", > because it isn't. Presumably there was a reason for the driver author > to select the default priority of 128. If there is a platform which has > a better means to restart the system, it should select a priority of > 129 or higher instead of affecting _all_ platforms using the imx watchdog > to reset the system. > > Sure, you can negotiate that with the driver author, but the default should > really be to change the priority for less affected platforms. Yes, looks like there is no common problem for watchdog drivers. Initially I was recalling that watchdog core uses 128 by default and typed the message without verifying it. I see now that it's incorrect, my bad. EC drivers tend to use higher priority in general. Jonathan, could you please confirm that NTXEC driver is a more preferable restart method than the watchdog?
On Sun, Nov 07, 2021 at 08:42:33PM +0300, Dmitry Osipenko wrote: [...] > EC drivers tend to use higher priority in general. Jonathan, could you > please confirm that NTXEC driver is a more preferable restart method > than the watchdog? Yes. The original firmware uses the NTXEC to restart, and it works well, so I do think it's preferable. Best regards, Jonathan
08.11.2021 14:22, Jonathan Neuschäfer пишет: > On Sun, Nov 07, 2021 at 08:42:33PM +0300, Dmitry Osipenko wrote: > [...] >> EC drivers tend to use higher priority in general. Jonathan, could you >> please confirm that NTXEC driver is a more preferable restart method >> than the watchdog? > > Yes. The original firmware uses the NTXEC to restart, and it works well, > so I do think it's preferable. Thank you, then I'll update the NTXEC patch like this: https://github.com/grate-driver/linux/commit/22da3d91f1734d9a0ed036220ad4ea28465af988
On Mon, Nov 08, 2021 at 02:36:42PM +0300, Dmitry Osipenko wrote: > 08.11.2021 14:22, Jonathan Neuschäfer пишет: > > On Sun, Nov 07, 2021 at 08:42:33PM +0300, Dmitry Osipenko wrote: > > [...] > >> EC drivers tend to use higher priority in general. Jonathan, could you > >> please confirm that NTXEC driver is a more preferable restart method > >> than the watchdog? > > > > Yes. The original firmware uses the NTXEC to restart, and it works well, > > so I do think it's preferable. > > Thank you, then I'll update the NTXEC patch like this: > > https://github.com/grate-driver/linux/commit/22da3d91f1734d9a0ed036220ad4ea28465af988 I tested again, but sys_off_handler_reboot called a bogus pointer (probably reboot_prepare_cb). I think it was left uninitialized in ntxec_probe, which uses devm_kmalloc. I guess we could switch it to devm_kzalloc: diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c index 1f55dfce14308..30364beb4b1d0 100644 --- a/drivers/mfd/ntxec.c +++ b/drivers/mfd/ntxec.c @@ -144,7 +144,7 @@ static int ntxec_probe(struct i2c_client *client) const struct mfd_cell *subdevs; size_t n_subdevs; - ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL); + ec = devm_kzalloc(&client->dev, sizeof(*ec), GFP_KERNEL); if (!ec) return -ENOMEM; With that done, it works flawlessly. Thanks, Jonathan
10.11.2021 13:43, Jonathan Neuschäfer пишет: > On Mon, Nov 08, 2021 at 02:36:42PM +0300, Dmitry Osipenko wrote: >> 08.11.2021 14:22, Jonathan Neuschäfer пишет: >>> On Sun, Nov 07, 2021 at 08:42:33PM +0300, Dmitry Osipenko wrote: >>> [...] >>>> EC drivers tend to use higher priority in general. Jonathan, could you >>>> please confirm that NTXEC driver is a more preferable restart method >>>> than the watchdog? >>> >>> Yes. The original firmware uses the NTXEC to restart, and it works well, >>> so I do think it's preferable. >> >> Thank you, then I'll update the NTXEC patch like this: >> >> https://github.com/grate-driver/linux/commit/22da3d91f1734d9a0ed036220ad4ea28465af988 > > I tested again, but sys_off_handler_reboot called a bogus pointer > (probably reboot_prepare_cb). I think it was left uninitialized in > ntxec_probe, which uses devm_kmalloc. I guess we could switch it to > devm_kzalloc: > > diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c > index 1f55dfce14308..30364beb4b1d0 100644 > --- a/drivers/mfd/ntxec.c > +++ b/drivers/mfd/ntxec.c > @@ -144,7 +144,7 @@ static int ntxec_probe(struct i2c_client *client) > const struct mfd_cell *subdevs; > size_t n_subdevs; > > - ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL); > + ec = devm_kzalloc(&client->dev, sizeof(*ec), GFP_KERNEL); > if (!ec) > return -ENOMEM; > > > > With that done, it works flawlessly. Good catch, thank you! I'll correct this patch and add yours t-b.
diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c index b711e73eedcb..fd6410cbe153 100644 --- a/drivers/mfd/ntxec.c +++ b/drivers/mfd/ntxec.c @@ -32,12 +32,11 @@ #define NTXEC_POWERKEEP_VALUE 0x0800 #define NTXEC_RESET_VALUE 0xff00 -static struct i2c_client *poweroff_restart_client; - -static void ntxec_poweroff(void) +static void ntxec_poweroff(struct power_off_data *data) { int res; u8 buf[3] = { NTXEC_REG_POWEROFF }; + struct i2c_client *poweroff_restart_client = data->cb_data; struct i2c_msg msgs[] = { { .addr = poweroff_restart_client->addr, @@ -62,8 +61,7 @@ static void ntxec_poweroff(void) msleep(5000); } -static int ntxec_restart(struct notifier_block *nb, - unsigned long action, void *data) +static void ntxec_restart(struct restart_data *data) { int res; u8 buf[3] = { NTXEC_REG_RESET }; @@ -72,6 +70,7 @@ static int ntxec_restart(struct notifier_block *nb, * it causes an I2C error. (The reset handler in the downstream driver * does send the full two-byte value, but doesn't check the result). */ + struct i2c_client *poweroff_restart_client = data->cb_data; struct i2c_msg msgs[] = { { .addr = poweroff_restart_client->addr, @@ -87,13 +86,11 @@ static int ntxec_restart(struct notifier_block *nb, if (res < 0) dev_warn(&poweroff_restart_client->dev, "Failed to restart (err = %d)\n", res); - - return NOTIFY_DONE; } -static struct notifier_block ntxec_restart_handler = { - .notifier_call = ntxec_restart, - .priority = 128, +static struct power_handler ntxec_power_handler = { + .restart_cb = ntxec_restart, + .power_off_cb = ntxec_poweroff, }; static int regmap_ignore_write(void *context, @@ -208,25 +205,12 @@ static int ntxec_probe(struct i2c_client *client) if (res < 0) return res; - if (poweroff_restart_client) - /* - * Another instance of the driver already took - * poweroff/restart duties. - */ - dev_err(ec->dev, "poweroff_restart_client already assigned\n"); - else - poweroff_restart_client = client; - - if (pm_power_off) - /* Another driver already registered a poweroff handler. */ - dev_err(ec->dev, "pm_power_off already assigned\n"); - else - pm_power_off = ntxec_poweroff; - - res = register_restart_handler(&ntxec_restart_handler); + ntxec_power_handler.cb_data = client; + + res = devm_register_power_handler(ec->dev, &ntxec_power_handler); if (res) dev_err(ec->dev, - "Failed to register restart handler: %d\n", res); + "Failed to register power handler: %d\n", res); } i2c_set_clientdata(client, ec); @@ -239,17 +223,6 @@ static int ntxec_probe(struct i2c_client *client) return res; } -static int ntxec_remove(struct i2c_client *client) -{ - if (client == poweroff_restart_client) { - poweroff_restart_client = NULL; - pm_power_off = NULL; - unregister_restart_handler(&ntxec_restart_handler); - } - - return 0; -} - static const struct of_device_id of_ntxec_match_table[] = { { .compatible = "netronix,ntxec", }, {} @@ -262,7 +235,6 @@ static struct i2c_driver ntxec_driver = { .of_match_table = of_ntxec_match_table, }, .probe_new = ntxec_probe, - .remove = ntxec_remove, }; module_i2c_driver(ntxec_driver);
Use devm_register_power_handler() that replaces global pm_power_off variable and allows to register multiple power-off handlers. It also provides restart-handler support, i.e. all in one API. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/mfd/ntxec.c | 50 ++++++++++----------------------------------- 1 file changed, 11 insertions(+), 39 deletions(-)