Problem
-------
SoC devices require power-off call chaining functionality from kernel.
We have a widely used restart chaining provided by restart notifier API,
but nothing for power-off.
Solution
--------
Introduce new API that provides both restart and power-off call chains.
Why combine restart with power-off? Because drivers often do both.
More practical to have API that provides both under the same roof.
The new API is designed with simplicity and extensibility in mind.
It's built upon the existing restart and reboot APIs. The simplicity
is in new helper functions that are convenient for drivers. The
extensibility is in the design that doesn't hardcode callback
arguments, making easy to add new parameters and remove old.
This is a third attempt to introduce the new API. First was made by
Guenter Roeck back in 2014, second was made by Thierry Reding in 2017.
In fact the work didn't stop and recently arm_pm_restart() was removed
from v5.14 kernel, which was a part of preparatory work started by
Guenter Roeck. I took into account experience and ideas from the
previous attempts, extended and polished them.
Adoption plan
-------------
This patchset introduces the new API. It also converts multiple drivers
and arch code to the new API to demonstrate how it all looks in practice.
The plan is:
1. Merge new API (patches 1-8). This API will co-exist with the old APIs.
2. Convert arch code to do_kernel_power_off() (patches 9-21).
3. Convert drivers and platform code to the new API.
4. Remove obsolete pm_power_off and pm_power_off_prepare variables.
5. Make restart-notifier API private to kernel/reboot.c once no users left.
It's fully implemented here:
[1] https://github.com/grate-driver/linux/commits/sys-off-handler
For now I'm sending only the first 25 base patches out of ~180. It's
preferable to squash 1-2, partially 3 and 4 points of the plan into a
single patchset to ease and speed up applying of the rest of the patches.
Majority of drivers and platform patches depend on the base, hence they
will come later (and per subsystem), once base will land.
All [1] patches are compile-tested. Tegra and x86 ACPI patches are tested
on hardware. The remaining should be covered by unit tests (unpublished).
Results
-------
1. Devices can be powered off properly.
2. Global variables are removed from drivers.
3. Global pm_power_off and pm_power_off_prepare callback variables are
removed once all users are converted to the new API. The latter callback
is removed by patch #25 of this series.
4. Ambiguous call chain ordering is prohibited. See patch #5 which adds
verification of restart handlers priorities, ensuring that they are unique.
Changelog:
v4: - Made a very minor improvement to doc comments, clarifying couple
default values.
- Corrected list of emails recipient by adding Linus, Sebastian,
Philipp and more NDS people. Removed bouncing emails.
- Added acks that were given to v3.
v3: - Renamed power_handler to sys_off_handler as was suggested by
Rafael Wysocki.
- Improved doc-comments as was suggested by Rafael Wysocki. Added more
doc-comments.
- Implemented full set of 180 patches which convert whole kernel in
accordance to the plan, see link [1] above. Slightly adjusted API to
better suit for the remaining converted drivers.
* Added unregister_sys_off_handler() that is handy for a couple old
platform drivers.
* Dropped devm_register_trivial_restart_handler(), 'simple' variant
is enough to have.
- Improved "Add atomic/blocking_notifier_has_unique_priority()" patch,
as was suggested by Andy Shevchenko. Also replaced down_write() with
down_read() and factored out common notifier_has_unique_priority().
- Added stop_chain field to struct restart_data and reboot_prep_data
after discovering couple drivers wanting that feature.
- Added acks that were given to v2.
v2: - Replaced standalone power-off call chain demo-API with the combined
power-off+restart API because this is what drivers want. It's a more
comprehensive solution.
- Converted multiple drivers and arch code to the new API. Suggested by
Andy Shevchenko. I skimmed through the rest of drivers, verifying that
new API suits them. The rest of the drivers will be converted once we
will settle on the new API, otherwise will be too many patches here.
- v2 API doesn't expose notifier to users and require handlers to
have unique priority. Suggested by Guenter Roeck.
- v2 API has power-off chaining disabled by default and require
drivers to explicitly opt-in to the chaining. This preserves old
behaviour for existing drivers once they are converted to the new
API.
Dmitry Osipenko (25):
notifier: Remove extern annotation from function prototypes
notifier: Add blocking_notifier_call_chain_is_empty()
notifier: Add atomic/blocking_notifier_has_unique_priority()
reboot: Correct typo in a comment
reboot: Warn if restart handler has duplicated priority
reboot: Warn if unregister_restart_handler() fails
reboot: Remove extern annotation from function prototypes
kernel: Add combined power-off+restart handler call chain API
ARM: Use do_kernel_power_off()
csky: Use do_kernel_power_off()
riscv: Use do_kernel_power_off()
arm64: Use do_kernel_power_off()
parisc: Use do_kernel_power_off()
xen/x86: Use do_kernel_power_off()
powerpc: Use do_kernel_power_off()
m68k: Switch to new sys-off handler API
sh: Use do_kernel_power_off()
x86: Use do_kernel_power_off()
ia64: Use do_kernel_power_off()
mips: Use do_kernel_power_off()
nds32: Use do_kernel_power_off()
memory: emif: Use kernel_can_power_off()
ACPI: power: Switch to sys-off handler API
regulator: pfuze100: Use devm_register_sys_off_handler()
reboot: Remove pm_power_off_prepare()
arch/arm/kernel/reboot.c | 4 +-
arch/arm64/kernel/process.c | 3 +-
arch/csky/kernel/power.c | 6 +-
arch/ia64/kernel/process.c | 4 +-
arch/m68k/emu/natfeat.c | 3 +-
arch/m68k/include/asm/machdep.h | 1 -
arch/m68k/kernel/process.c | 5 +-
arch/m68k/kernel/setup_mm.c | 1 -
arch/m68k/kernel/setup_no.c | 1 -
arch/m68k/mac/config.c | 4 +-
arch/mips/kernel/reset.c | 3 +-
arch/nds32/kernel/process.c | 3 +-
arch/parisc/kernel/process.c | 4 +-
arch/powerpc/kernel/setup-common.c | 4 +-
arch/powerpc/xmon/xmon.c | 3 +-
arch/riscv/kernel/reset.c | 12 +-
arch/sh/kernel/reboot.c | 3 +-
arch/x86/kernel/reboot.c | 4 +-
arch/x86/xen/enlighten_pv.c | 4 +-
drivers/acpi/sleep.c | 25 +-
drivers/memory/emif.c | 2 +-
drivers/regulator/pfuze100-regulator.c | 38 +-
include/linux/notifier.h | 37 +-
include/linux/pm.h | 1 -
include/linux/reboot.h | 305 ++++++++++++--
kernel/notifier.c | 83 ++++
kernel/power/hibernate.c | 2 +-
kernel/reboot.c | 556 ++++++++++++++++++++++++-
28 files changed, 985 insertions(+), 136 deletions(-)
--
2.33.1
There is no need to annotate function prototypes with 'extern', it makes
code less readable. Remove unnecessary annotations from <notifier.h>.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
include/linux/notifier.h | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
kernel/reboot.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
Emit warning if unregister_restart_handler() fails since it never should
fail. This will ease further API development by catching mistakes early.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
kernel/reboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
There is no need to annotate function prototypes with 'extern', it makes
code less readable. Remove unnecessary annotations from <reboot.h>.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
include/linux/reboot.h | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
@@ -40,36 +40,36 @@ extern int reboot_cpu;externintreboot_force;-externintregister_reboot_notifier(structnotifier_block*);-externintunregister_reboot_notifier(structnotifier_block*);+intregister_reboot_notifier(structnotifier_block*);+intunregister_reboot_notifier(structnotifier_block*);-externintdevm_register_reboot_notifier(structdevice*,structnotifier_block*);+intdevm_register_reboot_notifier(structdevice*,structnotifier_block*);-externintregister_restart_handler(structnotifier_block*);-externintunregister_restart_handler(structnotifier_block*);-externvoiddo_kernel_restart(char*cmd);+intregister_restart_handler(structnotifier_block*);+intunregister_restart_handler(structnotifier_block*);+voiddo_kernel_restart(char*cmd);/**Architecture-specificimplementationsofsys_rebootcommands.*/-externvoidmigrate_to_reboot_cpu(void);-externvoidmachine_restart(char*cmd);-externvoidmachine_halt(void);-externvoidmachine_power_off(void);+voidmigrate_to_reboot_cpu(void);+voidmachine_restart(char*cmd);+voidmachine_halt(void);+voidmachine_power_off(void);-externvoidmachine_shutdown(void);+voidmachine_shutdown(void);structpt_regs;-externvoidmachine_crash_shutdown(structpt_regs*);+voidmachine_crash_shutdown(structpt_regs*);/**Architectureindependentimplementationsofsys_rebootcommands.*/-externvoidkernel_restart_prepare(char*cmd);-externvoidkernel_restart(char*cmd);-externvoidkernel_halt(void);-externvoidkernel_power_off(void);+voidkernel_restart_prepare(char*cmd);+voidkernel_restart(char*cmd);+voidkernel_halt(void);+voidkernel_power_off(void);externintC_A_D;/* for sysctl */voidctrl_alt_del(void);
SoC platforms often have multiple ways of how to perform system's
power-off and restart operations. Meanwhile today's kernel is limited to
a single option. Add combined power-off+restart handler call chain API,
which is inspired by the restart API. The new API provides both power-off
and restart functionality.
The old pm_power_off method will be kept around till all users are
converted to the new API.
Current restart API will be replaced by the new unified API since
new API is its superset. The restart functionality of the sys-off handler
API is built upon the existing restart-notifier APIs.
In order to ease conversion to the new API, convenient helpers are added
for the common use-cases. They will reduce amount of boilerplate code and
remove global variables. These helpers preserve old behaviour for cases
where only one power-off handler is expected, this is what all existing
drivers want, and thus, they could be easily converted to the new API.
Users of the new API should explicitly enable power-off chaining by
setting corresponding flag of the power_handler structure.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
include/linux/reboot.h | 265 ++++++++++++++++++-
kernel/power/hibernate.c | 2 +-
kernel/reboot.c | 536 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 795 insertions(+), 8 deletions(-)
@@ -8,10 +8,35 @@structdevice;-#define SYS_DOWN 0x0001 /* Notify of system down */-#define SYS_RESTART SYS_DOWN-#define SYS_HALT 0x0002 /* Notify of system halt */-#define SYS_POWER_OFF 0x0003 /* Notify of system power off */+enumreboot_prepare_mode{+SYS_DOWN=1,/* Notify of system down */+SYS_RESTART=SYS_DOWN,+SYS_HALT,/* Notify of system halt */+SYS_POWER_OFF,/* Notify of system power off */+};++/*+*Standardrestartprioritylevels.Intendedtobesetinthe+*sys_off_handler.restart_priorityfield.+*+*Use`RESTART_PRIO_ABC+-prio`styleforadditionallevels.+*+*RESTART_PRIO_RESERVED:FallsbacktoRESTART_PRIO_DEFAULT.+*Driversmayleavepriorityinitialized+*tozero,toauto-setittothedefaultlevel.+*+*RESTART_PRIO_LOW:Usethisforhandleroflastresort.+*+*RESTART_PRIO_DEFAULT:Usethisfordefault/generichandler.+*+*RESTART_PRIO_HIGH:Usethisifyouhavemultiplehandlersand+*thishandlerhashigherprioritythanthe+*defaulthandler.+*/+#define RESTART_PRIO_RESERVED 0+#define RESTART_PRIO_LOW 8+#define RESTART_PRIO_DEFAULT 128+#define RESTART_PRIO_HIGH 192enumreboot_mode{REBOOT_UNDEFINED=-1,
@@ -294,6 +294,527 @@ void kernel_halt(void)}EXPORT_SYMBOL_GPL(kernel_halt);+/*+*Notifierlistforkernelcodewhichwantstobecalled+*topoweroffthesystem.+*/+staticBLOCKING_NOTIFIER_HEAD(power_off_handler_list);++staticvoiddummy_pm_power_off(void)+{+/* temporary stub until pm_power_off() is gone, see more below */+}++staticstructnotifier_block*pm_power_off_nb;++/**+*register_power_off_handler-Registerfunctiontobecalledtopoweroff+*thesystem+*@nb:Infoabouthandlerfunctiontobecalled+*@nb->priority:Handlerpriority.Handlersshouldfollowthe+*followingguidelinesforsettingpriorities.+*0:Reserved+*1:Power-offhandleroflastresort,+*withlimitedpower-offcapabilities+*128:Defaultpower-offhandler;useifnoother+*power-offhandlerisexpectedtobeavailable,+*and/orifpower-offfunctionalityis+*sufficienttopower-offtheentiresystem+*255:Highestprioritypower-offhandler,will+*preemptallotherpower-offhandlers+*+*Registersafunctionwithcodetobecalledtopoweroffthe+*system.+*+*Registeredfunctionswillbecalledaslaststepofthepower-off+*sequence.+*+*Registeredfunctionsareexpectedtopoweroffthesystemimmediately.+*Ifmorethanonefunctionisregistered,thepower-offhandlerpriority+*selectswhichfunctionwillbecalledfirst.+*+*Power-offhandlersareexpectedtoberegisteredfromnon-architecture+*code,typicallyfromdrivers.Atypicalusecasewouldbeasystem+*wherepower-offfunctionalityisprovidedthroughaPMIC.Multiple+*power-offhandlersmayexist;forexample,onepower-offhandlermight+*turnofftheentiresystem,whileanotheronlyturnsoffpartof+*system.Insuchcases,thepower-offhandlerwhichonlydisablespart+*ofthehardwareisexpectedtoregisterwithlowprioritytoensure+*thatitonlyrunsifnoothermeanstopoweroffthesystemis+*available.+*+*Currentlyalwaysreturnszero,asblocking_notifier_chain_register()+*alwaysreturnszero.+*/+staticintregister_power_off_handler(structnotifier_block*nb)+{+intret;++ret=blocking_notifier_chain_register(&power_off_handler_list,nb);+if(ret)+returnret;++/*+*Handlermusthaveuniquepriority.Otherwisecallorderorderis+*determinedbyregistrationorder,whichisunreliable.+*/+WARN(!blocking_notifier_has_unique_priority(&power_off_handler_list,nb),+"power-off handler must have unique priority\n");++/*+*Somedriverscheckwhetherpm_power_offwasalreadyinstalled.+*InstalldummycallbackusingnewAPItopreserveoldbehaviour+*forthosedriversduringperiodoftransitiontothenewAPI.+*/+if(!pm_power_off){+pm_power_off=dummy_pm_power_off;+pm_power_off_nb=nb;+}++return0;+}++staticintunregister_power_off_handler(structnotifier_block*nb)+{+intret;++if(nb==pm_power_off_nb){+/*+*Checkwhethersomebodyreplacedpm_power_offbehind+*outback.+*/+if(!WARN_ON(pm_power_off!=dummy_pm_power_off))+pm_power_off=NULL;++pm_power_off_nb=NULL;+}++ret=blocking_notifier_chain_unregister(&power_off_handler_list,nb);++returnWARN_ON(ret);+}++staticvoiddevm_unregister_power_off_handler(void*data)+{+structnotifier_block*nb=data;++unregister_power_off_handler(nb);+}++staticintdevm_register_power_off_handler(structdevice*dev,+structnotifier_block*nb)+{+interr;++err=register_power_off_handler(nb);+if(err)+returnerr;++returndevm_add_action_or_reset(dev,devm_unregister_power_off_handler,+nb);+}++staticintsys_off_handler_power_off(structnotifier_block*nb,+unsignedlongmode,void*unused)+{+structpower_off_prep_dataprep_data={};+structsys_off_handler_private_data*priv;+structpower_off_datadata={};+structsys_off_handler*h;+intret=NOTIFY_DONE;++priv=container_of(nb,structsys_off_handler_private_data,power_off_nb);+h=container_of(priv,structsys_off_handler,priv);+prep_data.cb_data=h->cb_data;+data.cb_data=h->cb_data;++switch(mode){+casePOWEROFF_NORMAL:+if(h->power_off_cb)+h->power_off_cb(&data);++if(priv->simple_power_off_cb)+priv->simple_power_off_cb(priv->simple_power_off_cb_data);++if(priv->platform_power_off_cb)+priv->platform_power_off_cb();++if(!h->power_off_chaining_allowed)+ret=NOTIFY_STOP;++break;++casePOWEROFF_PREPARE:+if(h->power_off_prepare_cb)+h->power_off_prepare_cb(&prep_data);++break;++default:+unreachable();+}++returnret;+}++staticintsys_off_handler_restart(structnotifier_block*nb,+unsignedlongmode,void*cmd)+{+structsys_off_handler_private_data*priv;+structrestart_datadata={};+structsys_off_handler*h;++priv=container_of(nb,structsys_off_handler_private_data,restart_nb);+h=container_of(priv,structsys_off_handler,priv);++data.stop_chain=h->restart_chaining_disallowed;+data.cb_data=h->cb_data;+data.mode=mode;+data.cmd=cmd;++h->restart_cb(&data);++returndata.stop_chain?NOTIFY_STOP:NOTIFY_DONE;+}++staticintsys_off_handler_reboot(structnotifier_block*nb,+unsignedlongmode,void*cmd)+{+structsys_off_handler_private_data*priv;+structreboot_prep_datadata={};+structsys_off_handler*h;++priv=container_of(nb,structsys_off_handler_private_data,reboot_nb);+h=container_of(priv,structsys_off_handler,priv);++data.cb_data=h->cb_data;+data.stop_chain=false;+data.mode=mode;+data.cmd=cmd;++h->reboot_prepare_cb(&data);++returndata.stop_chain?NOTIFY_STOP:NOTIFY_DONE;+}++staticstructsys_off_handler_private_data*+sys_off_handler_private_data(structsys_off_handler*handler)+{+return(structsys_off_handler_private_data*)&handler->priv;+}++/**+*devm_register_sys_off_handler-Registersystempower-off/restarthandler+*@dev:Devicethatregistershandler+*@handler:System-offhandler+*+*Registershandlerthatwillbecalledaslaststepofthepower-off+*andrestartsequences.+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intregister_sys_off_handler(structsys_off_handler*handler)+{+structsys_off_handler_private_data*priv;+interr,priority;++priv=sys_off_handler_private_data(handler);++/* sanity-check whether handler is registered twice */+if(WARN_ON(priv->registered))+return-EBUSY;++if(handler->power_off_cb||handler->power_off_prepare_cb){+if(handler->power_off_priority==POWEROFF_PRIO_RESERVED)+priority=POWEROFF_PRIO_DEFAULT;+else+priority=handler->power_off_priority;++priv->power_off_nb.notifier_call=sys_off_handler_power_off;+priv->power_off_nb.priority=priority;++err=register_power_off_handler(&priv->power_off_nb);+if(err)+gotoreset_sys_off_handler;+}++if(handler->restart_cb){+if(handler->restart_priority==RESTART_PRIO_RESERVED)+priority=RESTART_PRIO_DEFAULT;+else+priority=handler->restart_priority;++priv->restart_nb.notifier_call=sys_off_handler_restart;+priv->restart_nb.priority=priority;++err=register_restart_handler(&priv->restart_nb);+if(err)+gotounreg_power_off_handler;+}++if(handler->reboot_prepare_cb){+priv->reboot_nb.notifier_call=sys_off_handler_reboot;+priv->reboot_nb.priority=handler->reboot_priority;++err=register_reboot_notifier(&priv->reboot_nb);+if(err)+gotounreg_restart_handler;+}++priv->registered=true;++return0;++unreg_restart_handler:+if(handler->restart_cb)+unregister_restart_handler(&priv->restart_nb);++unreg_power_off_handler:+if(handler->power_off_cb)+unregister_power_off_handler(&priv->power_off_nb);++reset_sys_off_handler:+memset(priv,0,sizeof(*priv));++returnerr;+}+EXPORT_SYMBOL(register_sys_off_handler);++/**+*unregister_sys_off_handler-Unregistersystempower-off/restarthandler+*@handler:System-offhandler+*+*Unregisterssys-offhandler.Doesnothingandreturnszeroifhandler+*isNULL.+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intunregister_sys_off_handler(structsys_off_handler*handler)+{+structsys_off_handler_private_data*priv;+interr=0;++if(!handler)+return0;++priv=sys_off_handler_private_data(handler);++/* sanity-check whether handler is unregistered twice */+if(WARN_ON(!priv->registered))+return-EINVAL;++if(handler->reboot_prepare_cb)+err|=unregister_reboot_notifier(&priv->reboot_nb);++if(handler->restart_cb)+err|=unregister_restart_handler(&priv->restart_nb);++if(handler->power_off_cb)+err|=unregister_power_off_handler(&priv->power_off_nb);++memset(priv,0,sizeof(*priv));++returnerr?-EINVAL:0;+}+EXPORT_SYMBOL(unregister_sys_off_handler);++staticvoiddevm_unregister_sys_off_handler(void*data)+{+structsys_off_handler*handler=data;++unregister_sys_off_handler(handler);+}++/**+*devm_register_sys_off_handler-Registersystempower-off/restarthandler+*@dev:Devicethatregistershandler+*@handler:System-offhandler+*+*Resource-managedvariantofregister_sys_off_handler().+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intdevm_register_sys_off_handler(structdevice*dev,+structsys_off_handler*handler)+{+interr;++err=register_sys_off_handler(handler);+if(err)+returnerr;++returndevm_add_action_or_reset(dev,devm_unregister_sys_off_handler,+handler);+}+EXPORT_SYMBOL(devm_register_sys_off_handler);++/**+*devm_register_prioritized_power_off_handler-Registerprioritizedpower-offcallback+*@dev:Devicethatregisterscallback+*@priority:Callback'spriority+*@callback:Callbackfunction+*@cb_data:Callback'sargument+*+*Registersresource-managedpower-offcallbackwithagivenpriority.+*Itwillbecalledaslaststepofthepower-offsequence.Further+*lowerprioritycallbackswon'tbeexecutedifthis@callbackfails.+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intdevm_register_prioritized_power_off_handler(structdevice*dev,+intpriority,+void(*callback)(void*data),+void*cb_data)+{+structsys_off_handler_private_data*priv;+structsys_off_handler*handler;++handler=devm_kzalloc(dev,sizeof(*handler),GFP_KERNEL);+if(!handler)+return-ENOMEM;++priv=sys_off_handler_private_data(handler);++priv->power_off_nb.notifier_call=sys_off_handler_power_off;+priv->power_off_nb.priority=priority;+priv->simple_power_off_cb_data=cb_data;+priv->simple_power_off_cb=callback;++returndevm_register_power_off_handler(dev,&priv->power_off_nb);+}+EXPORT_SYMBOL(devm_register_prioritized_power_off_handler);++/**+*devm_register_prioritized_restart_handler-Registerprioritizedrestartcallback+*@dev:Devicethatregisterscallback+*@priority:Callback'spriority+*@callback:Callbackfunction+*@cb_data:Callback'sargument+*+*Registersresource-managedrestartcallbackwithagivenpriority.+*Itwillbecalledasapartoftherestartsequence.Further+*lowerprioritycallbackwillbeexecutedifthis@callbackfails.+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intdevm_register_prioritized_restart_handler(structdevice*dev,+intpriority,+void(*callback)(structrestart_data*data),+void*cb_data)+{+structsys_off_handler*handler;++handler=devm_kzalloc(dev,sizeof(*handler),GFP_KERNEL);+if(!handler)+return-ENOMEM;++handler->restart_priority=priority;+handler->restart_cb=callback;+handler->cb_data=cb_data;++returndevm_register_sys_off_handler(dev,handler);+}+EXPORT_SYMBOL(devm_register_prioritized_restart_handler);++staticstructsys_off_handlerplatform_power_off_handler={+.priv={+.power_off_nb={+.notifier_call=sys_off_handler_power_off,+.priority=POWEROFF_PRIO_PLATFORM,+},+},+};++staticDEFINE_SPINLOCK(platform_power_off_lock);++/**+*register_platform_power_off-Registerplatform-levelpower-offcallback+*@power_off:Power-offcallback+*+*Registerspower-offcallbackthatwillbecalledaslaststep+*ofthepower-offsequence.Thiscallbackisexpectedtobeinvoked+*forthelastresort.Furtherlowerprioritycallbackswon'tbe+*executedif@power_offfails.Onlyoneplatformpower-offcallback+*isallowedtoberegisteredatatime.+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intregister_platform_power_off(void(*power_off)(void))+{+structsys_off_handler_private_data*priv;+interr=0;++priv=sys_off_handler_private_data(&platform_power_off_handler);++spin_lock(&platform_power_off_lock);+if(priv->platform_power_off_cb)+err=-EBUSY;+else+priv->platform_power_off_cb=power_off;+spin_unlock(&platform_power_off_lock);++if(WARN_ON(err))+returnerr;++returnregister_power_off_handler(&priv->power_off_nb);+}+EXPORT_SYMBOL_GPL(register_platform_power_off);++/**+*unregister_platform_power_off-Unregisterplatform-levelpower-offcallback+*@power_off:Power-offcallback+*+*Unregisterspreviouslyregisteredplatformpower-offcallback.+*+*Returnszeroonsuccess,orerrorcodeonfailure.+*/+intunregister_platform_power_off(void(*power_off)(void))+{+structsys_off_handler_private_data*priv;+intret;++priv=sys_off_handler_private_data(&platform_power_off_handler);++if(WARN_ON(priv->platform_power_off_cb!=power_off))+return-EINVAL;++ret=unregister_power_off_handler(&priv->power_off_nb);+priv->platform_power_off_cb=NULL;++returnret;+}+EXPORT_SYMBOL_GPL(unregister_platform_power_off);++/**+*do_kernel_power_off-Executekernelpower-offhandlercallchain+*+*Callsfunctionsregisteredwithregister_power_off_handler.+*+*Expectedtobecalledaslaststepofthepower-offsequence.+*+*Powersoffthesystemimmediatelyifapower-offhandlerfunctionhas+*beenregistered.Otherwisedoesnothing.+*/+voiddo_kernel_power_off(void)+{+/* legacy pm_power_off() is unchained and has highest priority */+if(pm_power_off&&pm_power_off!=dummy_pm_power_off)+returnpm_power_off();++blocking_notifier_call_chain(&power_off_handler_list,POWEROFF_NORMAL,+NULL);+}++staticvoiddo_kernel_power_off_prepare(void)+{+/* legacy pm_power_off_prepare() is unchained and has highest priority */+if(pm_power_off_prepare)+returnpm_power_off_prepare();++blocking_notifier_call_chain(&power_off_handler_list,POWEROFF_PREPARE,+NULL);+}+/***kernel_power_off-power_offthesystem*
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Reviewed-by: Russell King (Oracle) <redacted>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/arm/kernel/reboot.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Acked-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/csky/kernel/power.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Acked-by: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/riscv/kernel/reset.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/arm64/kernel/process.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Acked-by: Helge Deller <deller@gmx.de> # parisc
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/parisc/kernel/process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -114,8 +115,7 @@ void machine_power_off(void)pdc_chassis_send_status(PDC_CHASSIS_DIRECT_SHUTDOWN);/* ipmi_poweroff may have been installed. */-if(pm_power_off)-pm_power_off();+do_kernel_power_off();/* It seems we have no way to power the system off via*software.Theuserhastopressthebuttonhimself.*/
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/powerpc/kernel/setup-common.c | 4 +---
arch/powerpc/xmon/xmon.c | 3 +--
2 files changed, 2 insertions(+), 5 deletions(-)
Kernel now supports chained power-off handlers. Use
register_power_off_handler() that registers power-off handlers and
do_kernel_power_off() that invokes chained power-off handlers. Legacy
pm_power_off() will be removed once all drivers will be converted to
the new power-off API.
Normally arch code should adopt only the do_kernel_power_off() at first,
but m68k is a special case because it uses pm_power_off() "inside out",
i.e. pm_power_off() invokes machine_power_off() [in fact it does nothing],
while it's machine_power_off() that should invoke the pm_power_off(), and
thus, we can't convert platforms to the new API separately. There are only
two platforms changed here, so it's not a big deal.
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/m68k/emu/natfeat.c | 3 ++-
arch/m68k/include/asm/machdep.h | 1 -
arch/m68k/kernel/process.c | 5 ++---
arch/m68k/kernel/setup_mm.c | 1 -
arch/m68k/kernel/setup_no.c | 1 -
arch/m68k/mac/config.c | 4 +++-
6 files changed, 7 insertions(+), 8 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/sh/kernel/reboot.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Acked-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/x86/xen/enlighten_pv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/x86/kernel/reboot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -747,10 +747,10 @@ static void native_machine_halt(void)staticvoidnative_machine_power_off(void){-if(pm_power_off){+if(kernel_can_power_off()){if(!reboot_force)machine_shutdown();-pm_power_off();+do_kernel_power_off();}/* A fallback in case there is no PM info available */tboot_shutdown(TB_SHUTDOWN_HALT);
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/ia64/kernel/process.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/mips/kernel/reset.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/nds32/kernel/process.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -630,7 +630,7 @@ static irqreturn_t emif_threaded_isr(int irq, void *dev_id)dev_emerg(emif->dev,"SDRAM temperature exceeds operating limit.. Needs shut down!!!\n");/* If we have Power OFF ability, use it, else try restarting */-if(pm_power_off){+if(kernel_can_power_off()){kernel_power_off();}else{WARN(1,"FIXME: NO pm_power_off!!! trying restart\n");
@@ -1020,7 +1012,7 @@ static void acpi_sleep_hibernate_setup(void)staticinlinevoidacpi_sleep_hibernate_setup(void){}#endif /* !CONFIG_HIBERNATION */-staticvoidacpi_power_off_prepare(void)+staticvoidacpi_power_off_prepare(structpower_off_prep_data*data){/* Prepare to power off the system */acpi_sleep_prepare(ACPI_STATE_S5);
@@ -1028,7 +1020,7 @@ static void acpi_power_off_prepare(void)acpi_os_wait_events_complete();}-staticvoidacpi_power_off(void)+staticvoidacpi_power_off(structpower_off_data*data){/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */pr_debug("%s called\n",__func__);
Use devm_register_sys_off_handler() that replaces global
pm_power_off_prepare variable and allows to register multiple
power-off handlers.
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/regulator/pfuze100-regulator.c | 38 ++++++++++----------------
1 file changed, 14 insertions(+), 24 deletions(-)
@@ -569,10 +571,10 @@ static inline struct device_node *match_of_node(int index)returnpfuze_matches[index].of_node;}-staticstructpfuze_chip*syspm_pfuze_chip;--staticvoidpfuze_power_off_prepare(void)+staticvoidpfuze_power_off_prepare(structpower_off_prep_data*data){+structpfuze_chip*syspm_pfuze_chip=data->cb_data;+dev_info(syspm_pfuze_chip->dev,"Configure standby mode for power off");/* Switch from default mode: APS/APS to APS/Off */
@@ -611,24 +613,23 @@ static void pfuze_power_off_prepare(void)staticintpfuze_power_off_prepare_init(structpfuze_chip*pfuze_chip){+interr;+if(pfuze_chip->chip_id!=PFUZE100){dev_warn(pfuze_chip->dev,"Requested pm_power_off_prepare handler for not supported chip\n");return-ENODEV;}-if(pm_power_off_prepare){-dev_warn(pfuze_chip->dev,"pm_power_off_prepare is already registered.\n");-return-EBUSY;-}+pfuze_chip->sys_off.power_off_prepare_cb=pfuze_power_off_prepare;+pfuze_chip->sys_off.cb_data=pfuze_chip;-if(syspm_pfuze_chip){-dev_warn(pfuze_chip->dev,"syspm_pfuze_chip is already set.\n");-return-EBUSY;+err=devm_register_sys_off_handler(pfuze_chip->dev,&pfuze_chip->sys_off);+if(err){+dev_err(pfuze_chip->dev,+"failed to register sys-off handler: %d\n",err);+returnerr;}-syspm_pfuze_chip=pfuze_chip;-pm_power_off_prepare=pfuze_power_off_prepare;-return0;}
@@ -837,23 +838,12 @@ static int pfuze100_regulator_probe(struct i2c_client *client,return0;}-staticintpfuze100_regulator_remove(structi2c_client*client)-{-if(syspm_pfuze_chip){-syspm_pfuze_chip=NULL;-pm_power_off_prepare=NULL;-}--return0;-}-staticstructi2c_driverpfuze_driver={.driver={.name="pfuze100-regulator",.of_match_table=pfuze_dt_ids,},.probe=pfuze100_regulator_probe,-.remove=pfuze100_regulator_remove,};module_i2c_driver(pfuze_driver);
@@ -20,7 +20,6 @@*Callbacksforplatformdriverstoimplement.*/externvoid(*pm_power_off)(void);-externvoid(*pm_power_off_prepare)(void);structdevice;/* we have a circular dep with device.h */#ifdef CONFIG_VT_CONSOLE_SLEEP
@@ -48,13 +48,6 @@ int reboot_cpu;enumreboot_typereboot_type=BOOT_ACPI;intreboot_force;-/*-*Ifset,thisisusedforpreparingthesystemtopoweroff.-*/--void(*pm_power_off_prepare)(void);-EXPORT_SYMBOL_GPL(pm_power_off_prepare);-/***emergency_restart-rebootthesystem*
@@ -807,10 +800,6 @@ void do_kernel_power_off(void)staticvoiddo_kernel_power_off_prepare(void){-/* legacy pm_power_off_prepare() is unchained and has highest priority */-if(pm_power_off_prepare)-returnpm_power_off_prepare();-blocking_notifier_call_chain(&power_off_handler_list,POWEROFF_PREPARE,NULL);}
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-28 00:31:08
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
Best Regards
Michał Mirosław
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-28 00:45:36
On Fri, Nov 26, 2021 at 09:00:44PM +0300, Dmitry Osipenko wrote:
SoC platforms often have multiple ways of how to perform system's
power-off and restart operations. Meanwhile today's kernel is limited to
a single option. Add combined power-off+restart handler call chain API,
which is inspired by the restart API. The new API provides both power-off
and restart functionality.
The old pm_power_off method will be kept around till all users are
converted to the new API.
Current restart API will be replaced by the new unified API since
new API is its superset. The restart functionality of the sys-off handler
API is built upon the existing restart-notifier APIs.
In order to ease conversion to the new API, convenient helpers are added
for the common use-cases. They will reduce amount of boilerplate code and
remove global variables. These helpers preserve old behaviour for cases
where only one power-off handler is expected, this is what all existing
drivers want, and thus, they could be easily converted to the new API.
Users of the new API should explicitly enable power-off chaining by
setting corresponding flag of the power_handler structure.
[...]
Hi,
A general question: do we really need three distinct chains for this?
Can't there be only one that chain of callbacks that get a stage
(RESTART_PREPARE, RESTART, POWER_OFF_PREPARE, POWER_OFF) and can ignore
them at will? Calling through POWER_OFF_PREPARE would also return
whether that POWER_OFF is possible (for kernel_can_power_off()).
I would also split this patch into preparation cleanups (like wrapping
pm_power_off call with a function) and adding the notifier-based
implementation.
Best Regards
Michał Mirosław
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-28 01:18:14
On Fri, Nov 26, 2021 at 09:00:54PM +0300, Dmitry Osipenko wrote:
quoted hunk
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/x86/kernel/reboot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Judging from an old commit from 2006 [1], this can be rewritten as:
if (!reboot_force && kernel_can_power_off())
machine_shutdown();
do_kernel_power_off();
And maybe later reworked so it doesn't need kernel_can_power_off().
[1] http://lkml.iu.edu/hypermail//linux/kernel/0511.3/0681.html
Best Regards
Michał Mirosław
@@ -630,7 +630,7 @@ static irqreturn_t emif_threaded_isr(int irq, void *dev_id)dev_emerg(emif->dev,"SDRAM temperature exceeds operating limit.. Needs shut down!!!\n");/* If we have Power OFF ability, use it, else try restarting */-if(pm_power_off){+if(kernel_can_power_off()){kernel_power_off();}else{WARN(1,"FIXME: NO pm_power_off!!! trying restart\n");
BTW, this part of the code seems to be better moved to generic code that
could replace POWER_OFF request with REBOOT like it is done for reboot()
syscall.
Best Regards
Michał Mirosław
On Fri, Nov 26, 2021 at 09:00:44PM +0300, Dmitry Osipenko wrote:
quoted
SoC platforms often have multiple ways of how to perform system's
power-off and restart operations. Meanwhile today's kernel is limited to
a single option. Add combined power-off+restart handler call chain API,
which is inspired by the restart API. The new API provides both power-off
and restart functionality.
The old pm_power_off method will be kept around till all users are
converted to the new API.
Current restart API will be replaced by the new unified API since
new API is its superset. The restart functionality of the sys-off handler
API is built upon the existing restart-notifier APIs.
In order to ease conversion to the new API, convenient helpers are added
for the common use-cases. They will reduce amount of boilerplate code and
remove global variables. These helpers preserve old behaviour for cases
where only one power-off handler is expected, this is what all existing
drivers want, and thus, they could be easily converted to the new API.
Users of the new API should explicitly enable power-off chaining by
setting corresponding flag of the power_handler structure.
[...]
Hi,
A general question: do we really need three distinct chains for this?
Hello Michał,
At minimum this makes code easier to follow.
Can't there be only one that chain of callbacks that get a stage
(RESTART_PREPARE, RESTART, POWER_OFF_PREPARE, POWER_OFF) and can ignore
them at will? Calling through POWER_OFF_PREPARE would also return
whether that POWER_OFF is possible (for kernel_can_power_off()).
I'm having trouble with parsing this comment. Could you please try to
rephrase it? I don't see how you could check whether power-off handler
is available if you'll mix all handlers together.
I would also split this patch into preparation cleanups (like wrapping
pm_power_off call with a function) and adding the notifier-based
implementation.
What's the benefit of this split up will be? Are you suggesting that it
will ease reviewing of this patch or something else?
@@ -630,7 +630,7 @@ static irqreturn_t emif_threaded_isr(int irq, void *dev_id)dev_emerg(emif->dev,"SDRAM temperature exceeds operating limit.. Needs shut down!!!\n");/* If we have Power OFF ability, use it, else try restarting */-if(pm_power_off){+if(kernel_can_power_off()){kernel_power_off();}else{WARN(1,"FIXME: NO pm_power_off!!! trying restart\n");
BTW, this part of the code seems to be better moved to generic code that
could replace POWER_OFF request with REBOOT like it is done for reboot()
syscall.
Not sure that it can be done. Somebody will have to verify that it won't
break all those platform power-off handlers. Better to keep this code
as-is in the context of this patchset.
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
On Fri, Nov 26, 2021 at 09:00:54PM +0300, Dmitry Osipenko wrote:
quoted
Kernel now supports chained power-off handlers. Use do_kernel_power_off()
that invokes chained power-off handlers. It also invokes legacy
pm_power_off() for now, which will be removed once all drivers will
be converted to the new power-off API.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
arch/x86/kernel/reboot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Judging from an old commit from 2006 [1], this can be rewritten as:
if (!reboot_force && kernel_can_power_off())
machine_shutdown();
do_kernel_power_off();
And maybe later reworked so it doesn't need kernel_can_power_off().
[1] http://lkml.iu.edu/hypermail//linux/kernel/0511.3/0681.html
It could be rewritten like you're suggesting, but I'd prefer to keep the
old variant, for clarity.
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-28 21:19:28
On Mon, Nov 29, 2021 at 12:04:01AM +0300, Dmitry Osipenko wrote:
28.11.2021 03:43, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:44PM +0300, Dmitry Osipenko wrote:
quoted
SoC platforms often have multiple ways of how to perform system's
power-off and restart operations. Meanwhile today's kernel is limited to
a single option. Add combined power-off+restart handler call chain API,
which is inspired by the restart API. The new API provides both power-off
and restart functionality.
The old pm_power_off method will be kept around till all users are
converted to the new API.
Current restart API will be replaced by the new unified API since
new API is its superset. The restart functionality of the sys-off handler
API is built upon the existing restart-notifier APIs.
In order to ease conversion to the new API, convenient helpers are added
for the common use-cases. They will reduce amount of boilerplate code and
remove global variables. These helpers preserve old behaviour for cases
where only one power-off handler is expected, this is what all existing
drivers want, and thus, they could be easily converted to the new API.
Users of the new API should explicitly enable power-off chaining by
setting corresponding flag of the power_handler structure.
[...]
Hi,
A general question: do we really need three distinct chains for this?
Hello Michał,
At minimum this makes code easier to follow.
quoted
Can't there be only one that chain of callbacks that get a stage
(RESTART_PREPARE, RESTART, POWER_OFF_PREPARE, POWER_OFF) and can ignore
them at will? Calling through POWER_OFF_PREPARE would also return
whether that POWER_OFF is possible (for kernel_can_power_off()).
I'm having trouble with parsing this comment. Could you please try to
rephrase it? I don't see how you could check whether power-off handler
is available if you'll mix all handlers together.
If notify_call_chain() would be fixed to return NOTIFY_OK if any call
returned NOTIFY_OK, then this would be a clear way to gather the
answer if any of the handlers will attempt the final action (reboot or
power off).
quoted
I would also split this patch into preparation cleanups (like wrapping
pm_power_off call with a function) and adding the notifier-based
implementation.
What's the benefit of this split up will be? Are you suggesting that it
will ease reviewing of this patch or something else?
Mainly to ease review, as the wrapping will be a no-op, but the addition
of notifier chain changes semantics a bit.
Best Regards
Michał Mirosław
I'm having trouble with parsing this comment. Could you please try to
rephrase it? I don't see how you could check whether power-off handler
is available if you'll mix all handlers together.
If notify_call_chain() would be fixed to return NOTIFY_OK if any call
returned NOTIFY_OK, then this would be a clear way to gather the
answer if any of the handlers will attempt the final action (reboot or
power off).
Could you please show a code snippet that implements your suggestion?
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-29 00:29:00
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
Best Regards
Michał Mirosław
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-29 00:38:45
On Mon, Nov 29, 2021 at 12:53:51AM +0300, Dmitry Osipenko wrote:
29.11.2021 00:17, Michał Mirosław пишет:
quoted
quoted
I'm having trouble with parsing this comment. Could you please try to
rephrase it? I don't see how you could check whether power-off handler
is available if you'll mix all handlers together.
If notify_call_chain() would be fixed to return NOTIFY_OK if any call
returned NOTIFY_OK, then this would be a clear way to gather the
answer if any of the handlers will attempt the final action (reboot or
power off).
Could you please show a code snippet that implements your suggestion?
A rough idea is this:
static int notifier_call_chain(struct notifier_block **nl,
unsigned long val, void *v,
int nr_to_call, int *nr_calls)
{
- int ret = NOTIFY_DONE;
+ int ret, result = NOTIFY_DONE;
struct notifier_block *nb, *next_nb;
nb = rcu_dereference_raw(*nl);
while (nb && nr_to_call) {
...
ret = nb->notifier_call(nb, val, v);
+
+ /* Assuming NOTIFY_STOP-carrying return is always greater than non-stopping one. */
+ if (result < ret)
+ result = ret;
...
}
- return ret;
+ return result;
}
Then:
bool prepare_reboot()
{
int ret = xx_notifier_call_chain(&shutdown_notifier, PREPARE_REBOOT, ...);
return ret == NOTIFY_OK;
}
And the return value would signify whether the reboot will be attempted
when calling the chain for the REBOOT action. (Analogously for powering off.)
Best Regards
Michał Mirosław
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
quoted
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
We can't just change the registration order because invocation order of
the call chain depends on the registration order and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
On Mon, Nov 29, 2021 at 12:53:51AM +0300, Dmitry Osipenko wrote:
quoted
29.11.2021 00:17, Michał Mirosław пишет:
quoted
quoted
I'm having trouble with parsing this comment. Could you please try to
rephrase it? I don't see how you could check whether power-off handler
is available if you'll mix all handlers together.
If notify_call_chain() would be fixed to return NOTIFY_OK if any call
returned NOTIFY_OK, then this would be a clear way to gather the
answer if any of the handlers will attempt the final action (reboot or
power off).
Could you please show a code snippet that implements your suggestion?
A rough idea is this:
static int notifier_call_chain(struct notifier_block **nl,
unsigned long val, void *v,
int nr_to_call, int *nr_calls)
{
- int ret = NOTIFY_DONE;
+ int ret, result = NOTIFY_DONE;
struct notifier_block *nb, *next_nb;
nb = rcu_dereference_raw(*nl);
while (nb && nr_to_call) {
...
ret = nb->notifier_call(nb, val, v);
+
+ /* Assuming NOTIFY_STOP-carrying return is always greater than non-stopping one. */
+ if (result < ret)
+ result = ret;
...
}
- return ret;
+ return result;
}
Then:
bool prepare_reboot()
{
int ret = xx_notifier_call_chain(&shutdown_notifier, PREPARE_REBOOT, ...);
return ret == NOTIFY_OK;
}
And the return value would signify whether the reboot will be attempted
when calling the chain for the REBOOT action. (Analogously for powering off.)
If you started to execute call chain, then you began the power-off /
restart sequence, this is a point of no return. Sorry, I still don't
understand what you're trying to achieve.
The approach of having separate call chains is simple and intuitive, I
don't see reasons to change it.
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-12-10 18:09:35
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
There is no need to annotate function prototypes with 'extern', it makes
code less readable. Remove unnecessary annotations from <reboot.h>.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
I'm not sure that this is really useful.
Personally, I tend to respect the existing conventions like this.
Surely, this change is not required for the rest of the series to work.
@@ -40,36 +40,36 @@ extern int reboot_cpu;externintreboot_force;-externintregister_reboot_notifier(structnotifier_block*);-externintunregister_reboot_notifier(structnotifier_block*);+intregister_reboot_notifier(structnotifier_block*);+intunregister_reboot_notifier(structnotifier_block*);-externintdevm_register_reboot_notifier(structdevice*,structnotifier_block*);+intdevm_register_reboot_notifier(structdevice*,structnotifier_block*);-externintregister_restart_handler(structnotifier_block*);-externintunregister_restart_handler(structnotifier_block*);-externvoiddo_kernel_restart(char*cmd);+intregister_restart_handler(structnotifier_block*);+intunregister_restart_handler(structnotifier_block*);+voiddo_kernel_restart(char*cmd);/**Architecture-specificimplementationsofsys_rebootcommands.*/-externvoidmigrate_to_reboot_cpu(void);-externvoidmachine_restart(char*cmd);-externvoidmachine_halt(void);-externvoidmachine_power_off(void);+voidmigrate_to_reboot_cpu(void);+voidmachine_restart(char*cmd);+voidmachine_halt(void);+voidmachine_power_off(void);-externvoidmachine_shutdown(void);+voidmachine_shutdown(void);structpt_regs;-externvoidmachine_crash_shutdown(structpt_regs*);+voidmachine_crash_shutdown(structpt_regs*);/**Architectureindependentimplementationsofsys_rebootcommands.*/-externvoidkernel_restart_prepare(char*cmd);-externvoidkernel_restart(char*cmd);-externvoidkernel_halt(void);-externvoidkernel_power_off(void);+voidkernel_restart_prepare(char*cmd);+voidkernel_restart(char*cmd);+voidkernel_halt(void);+voidkernel_power_off(void);externintC_A_D;/* for sysctl */voidctrl_alt_del(void);
@@ -322,6 +322,20 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,}EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);+/**+*blocking_notifier_call_chain_is_empty-Checkwhethernotifierchainisempty+*@nh:Pointertoheadoftheblockingnotifierchain+*+*Checkswhethernotifierchainisempty.+*+*Returnstrueisnotifierchainisempty,falseotherwise.+*/+boolblocking_notifier_call_chain_is_empty(structblocking_notifier_head*nh)+{+return!rcu_access_pointer(nh->head);+}+EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_is_empty);
The check is not reliable (racy) without locking, so I wouldn't export
anything like this to modules.
At least IMO it should be added along with a user.
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted
There is no need to annotate function prototypes with 'extern', it makes
code less readable. Remove unnecessary annotations from <reboot.h>.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
I'm not sure that this is really useful.
Personally, I tend to respect the existing conventions like this.
Surely, this change is not required for the rest of the series to work.
Problem that such things start to spread all over the kernel with a
copy-paste approach if there is nobody to clean up the code.
This is not a common convention and sometimes it's getting corrected [1].
[1] https://git.kernel.org/linus/6d7434931
@@ -322,6 +322,20 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,}EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);+/**+*blocking_notifier_call_chain_is_empty-Checkwhethernotifierchainisempty+*@nh:Pointertoheadoftheblockingnotifierchain+*+*Checkswhethernotifierchainisempty.+*+*Returnstrueisnotifierchainisempty,falseotherwise.+*/+boolblocking_notifier_call_chain_is_empty(structblocking_notifier_head*nh)+{+return!rcu_access_pointer(nh->head);+}+EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_is_empty);
The check is not reliable (racy) without locking, so I wouldn't export
anything like this to modules.
At least IMO it should be added along with a user.
I'll remove the export since it's indeed not obvious how other users may
want to use this function.
@@ -122,6 +122,19 @@ static int notifier_call_chain_robust(struct notifier_block **nl,returnret;}+staticintnotifier_has_unique_priority(structnotifier_block**nl,+structnotifier_block*n)+{+while(*nl&&(*nl)->priority>=n->priority){+if((*nl)->priority==n->priority&&*nl!=n)+returnfalse;++nl=&((*nl)->next);+}++returntrue;+}+/**Atomicnotifierchainroutines.Registrationandunregistration*useaspinlock,andcall_chainissynchronizedbyRCU(nolocks).
@@ -203,6 +216,30 @@ int atomic_notifier_call_chain(struct atomic_notifier_head *nh,EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);NOKPROBE_SYMBOL(atomic_notifier_call_chain);+/**+*atomic_notifier_has_unique_priority-Checkswhethernotifier'spriorityisunique+*@nh:Pointertoheadoftheatomicnotifierchain+*@n:Entryinnotifierchaintocheck+*+*Checkswhetherthereisanothernotifierinthechainwiththesamepriority.+*Mustbecalledinprocesscontext.+*+*Returnstrueifpriorityisunique,falseotherwise.+*/+boolatomic_notifier_has_unique_priority(structatomic_notifier_head*nh,+structnotifier_block*n)+{+unsignedlongflags;+boolret;++spin_lock_irqsave(&nh->lock,flags);+ret=notifier_has_unique_priority(&nh->head,n);+spin_unlock_irqrestore(&nh->lock,flags);
This only works if the caller can prevent new entries from being added
to the list at this point or if the caller knows that they cannot be
added for some reason, but the kerneldoc doesn't mention this
limitation.
quoted hunk
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(atomic_notifier_has_unique_priority);
+
/*
* Blocking notifier chain routines. All access to the chain is
* synchronized by an rwsem.
@@ -336,6 +373,38 @@ bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh) } EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_is_empty);+/**+ * blocking_notifier_has_unique_priority - Checks whether notifier's priority is unique+ * @nh: Pointer to head of the blocking notifier chain+ * @n: Entry in notifier chain to check+ *+ * Checks whether there is another notifier in the chain with the same priority.+ * Must be called in process context.+ *+ * Returns true if priority is unique, false otherwise.+ */+bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,+ struct notifier_block *n)+{+ bool ret;++ /*+ * This code gets used during boot-up, when task switching is+ * not yet working and interrupts must remain disabled. At such+ * times we must not call down_read().+ */+ if (system_state != SYSTEM_BOOTING)
No, please don't do this, it makes the whole thing error-prone.
+ down_read(&nh->rwsem);
+
+ ret = notifier_has_unique_priority(&nh->head, n);
+
+ if (system_state != SYSTEM_BOOTING)
+ up_read(&nh->rwsem);
And still what if a new entry with a non-unique priority is added to
the chain at this point?
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(blocking_notifier_has_unique_priority);
+
/*
* Raw notifier chain routines. There is no protection;
* the caller must provide it. Use at your own risk!
--
2.33.1
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-12-10 18:28:10
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko [off-list ref] wrote:
29.11.2021 03:26, Michał Mirosław пишет:
quoted
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
quoted
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
We can't just change the registration order because invocation order of
the call chain depends on the registration order
It doesn't if unique priorities are required and isn't that what you want?
and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
Well, the WARN() doesn't help much then.
Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-12-10 18:32:42
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted hunk
Emit warning if unregister_restart_handler() fails since it never should
fail. This will ease further API development by catching mistakes early.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
kernel/reboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The only reason why it can fail is if the object pointed to by nb is
not in the chain. Why WARN() about this? And what about systems with
panic_on_warn set?
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-12-10 18:35:33
On Fri, Dec 10, 2021 at 7:16 PM Dmitry Osipenko [off-list ref] wrote:
10.12.2021 21:09, Rafael J. Wysocki пишет:
quoted
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted
There is no need to annotate function prototypes with 'extern', it makes
code less readable. Remove unnecessary annotations from <reboot.h>.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
I'm not sure that this is really useful.
Personally, I tend to respect the existing conventions like this.
Surely, this change is not required for the rest of the series to work.
Problem that such things start to spread all over the kernel with a
copy-paste approach if there is nobody to clean up the code.
This is not a common convention and sometimes it's getting corrected [1].
[1] https://git.kernel.org/linus/6d7434931
In separate patches outside of series adding new features, if one is
so inclined.
This only works if the caller can prevent new entries from being added
to the list at this point or if the caller knows that they cannot be
added for some reason, but the kerneldoc doesn't mention this
limitation.
I'll update the comment.
..
quoted
+bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
+ struct notifier_block *n)
+{
+ bool ret;
+
+ /*
+ * This code gets used during boot-up, when task switching is
+ * not yet working and interrupts must remain disabled. At such
+ * times we must not call down_read().
+ */
+ if (system_state != SYSTEM_BOOTING)
No, please don't do this, it makes the whole thing error-prone.
What should I do then?
quoted
+ down_read(&nh->rwsem);
+
+ ret = notifier_has_unique_priority(&nh->head, n);
+
+ if (system_state != SYSTEM_BOOTING)
+ up_read(&nh->rwsem);
And still what if a new entry with a non-unique priority is added to
the chain at this point?
If entry with a non-unique priority is added after the check, then
obviously it won't be detected. I don't understand the question. These
down/up_read() are the locks that prevent the race, if that's the question.
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted
Emit warning if unregister_restart_handler() fails since it never should
fail. This will ease further API development by catching mistakes early.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
kernel/reboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The only reason why it can fail is if the object pointed to by nb is
not in the chain.
I had exactly this case where object wasn't in the chain due to a bug
and this warning was very helpful.
Why WARN() about this? And what about systems with
panic_on_warn set?
That warning condition will never happen normally, only when something
is seriously wrong.
Those systems with panic_on_warn will get what was they asked for.
On Fri, Dec 10, 2021 at 7:16 PM Dmitry Osipenko [off-list ref] wrote:
quoted
10.12.2021 21:09, Rafael J. Wysocki пишет:
quoted
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted
There is no need to annotate function prototypes with 'extern', it makes
code less readable. Remove unnecessary annotations from <reboot.h>.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
I'm not sure that this is really useful.
Personally, I tend to respect the existing conventions like this.
Surely, this change is not required for the rest of the series to work.
Problem that such things start to spread all over the kernel with a
copy-paste approach if there is nobody to clean up the code.
This is not a common convention and sometimes it's getting corrected [1].
[1] https://git.kernel.org/linus/6d7434931
In separate patches outside of series adding new features, if one is
so inclined.
Alright, I'll drop this patch then because it can't be done in parallel
without creating the merge conflict. I'll try not to forget to come back
to this later on.
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko [off-list ref] wrote:
quoted
29.11.2021 03:26, Michał Mirosław пишет:
quoted
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
quoted
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
We can't just change the registration order because invocation order of
the call chain depends on the registration order
It doesn't if unique priorities are required and isn't that what you want?
quoted
and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
Well, the WARN() doesn't help much then.
Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
The potential problem is on the user's side and the warning is intended
to aid the user.
We can make it a strong requirement, but only after converting and
testing all kernel drivers. I'll consider to add patches for that.
This only works if the caller can prevent new entries from being added
to the list at this point or if the caller knows that they cannot be
added for some reason, but the kerneldoc doesn't mention this
limitation.
I'll update the comment.
..
quoted
quoted
+bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
+ struct notifier_block *n)
+{
+ bool ret;
+
+ /*
+ * This code gets used during boot-up, when task switching is
+ * not yet working and interrupts must remain disabled. At such
+ * times we must not call down_read().
+ */
+ if (system_state != SYSTEM_BOOTING)
No, please don't do this, it makes the whole thing error-prone.
What should I do then?
First of all, do you know of any users who may want to call this
during early initialization? If so, then why may they want to do
that?
Depending on the above, I would consider adding a special mechanism for them.
quoted
quoted
+ down_read(&nh->rwsem);
+
+ ret = notifier_has_unique_priority(&nh->head, n);
+
+ if (system_state != SYSTEM_BOOTING)
+ up_read(&nh->rwsem);
And still what if a new entry with a non-unique priority is added to
the chain at this point?
If entry with a non-unique priority is added after the check, then
obviously it won't be detected.
Why isn't this a problem?
I don't understand the question. These
down/up_read() are the locks that prevent the race, if that's the question.
Not really, they only prevent the race from occurring while
notifier_has_unique_priority() is running.
If anyone depends on this check for correctness, they need to lock the
rwsem, do the check, do the thing depending on the check while holding
the rwsem and then release the rwsem. Otherwise it is racy.
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-12-10 19:08:36
On Fri, Dec 10, 2021 at 7:54 PM Dmitry Osipenko [off-list ref] wrote:
10.12.2021 21:32, Rafael J. Wysocki пишет:
quoted
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted
Emit warning if unregister_restart_handler() fails since it never should
fail. This will ease further API development by catching mistakes early.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
kernel/reboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The only reason why it can fail is if the object pointed to by nb is
not in the chain.
I had exactly this case where object wasn't in the chain due to a bug
and this warning was very helpful.
During the development. In production it would be rather annoying.
quoted
Why WARN() about this? And what about systems with
panic_on_warn set?
That warning condition will never happen normally, only when something
is seriously wrong.
Those systems with panic_on_warn will get what was they asked for.
They may not be asking for panicking on bugs in the reboot notifier
code, though. That's what your change is making them panic on.
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-12-10 19:14:49
On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko [off-list ref] wrote:
10.12.2021 21:27, Rafael J. Wysocki пишет:
quoted
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko [off-list ref] wrote:
quoted
29.11.2021 03:26, Michał Mirosław пишет:
quoted
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
quoted
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
We can't just change the registration order because invocation order of
the call chain depends on the registration order
It doesn't if unique priorities are required and isn't that what you want?
quoted
and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
Well, the WARN() doesn't help much then.
Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
In which case adding the WARN() is not appropriate IMV.
Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter. I'm not sure what
the purpose of this patch is TBH.
The potential problem is on the user's side and the warning is intended
to aid the user.
Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about. IOW,
WARN() helps developers, not users.
We can make it a strong requirement, but only after converting and
testing all kernel drivers.
Right.
I'll consider to add patches for that.
But can you avoid adding more patches to this series?
This only works if the caller can prevent new entries from being added
to the list at this point or if the caller knows that they cannot be
added for some reason, but the kerneldoc doesn't mention this
limitation.
I'll update the comment.
..
quoted
quoted
+bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
+ struct notifier_block *n)
+{
+ bool ret;
+
+ /*
+ * This code gets used during boot-up, when task switching is
+ * not yet working and interrupts must remain disabled. At such
+ * times we must not call down_read().
+ */
+ if (system_state != SYSTEM_BOOTING)
No, please don't do this, it makes the whole thing error-prone.
What should I do then?
First of all, do you know of any users who may want to call this
during early initialization? If so, then why may they want to do
that?
I'll need to carefully review all those dozens of platform restart
handlers to answer this question.
Depending on the above, I would consider adding a special mechanism for them.
Please notice that every blocking_notifier_*() function has this
SYSTEM_BOOTING check, it's not my invention. Notifier API needs to be
generic.
quoted
quoted
quoted
+ down_read(&nh->rwsem);
+
+ ret = notifier_has_unique_priority(&nh->head, n);
+
+ if (system_state != SYSTEM_BOOTING)
+ up_read(&nh->rwsem);
And still what if a new entry with a non-unique priority is added to
the chain at this point?
If entry with a non-unique priority is added after the check, then
obviously it won't be detected.
Why isn't this a problem?>> I don't understand the question. These
quoted
down/up_read() are the locks that prevent the race, if that's the question.
Not really, they only prevent the race from occurring while
notifier_has_unique_priority() is running.
If anyone depends on this check for correctness, they need to lock the
rwsem, do the check, do the thing depending on the check while holding
the rwsem and then release the rwsem. Otherwise it is racy.
It's fine that it's a bit "racy" since in the context of this series. We
always do the check after adding new entry, so it's not a problem.
There are two options:
1. Use blocking_notifier_has_unique_priority() like it's done in this
patchset. Remove it after all drivers are converted to the new API and
add blocking_notifier_chain_register_unique().
2. Add blocking_notifier_chain_register_unique(), but don't let it fail
the registration of non-unique entries until all drivers are converted
to the new API.
On Fri, Dec 10, 2021 at 7:54 PM Dmitry Osipenko [off-list ref] wrote:
quoted
10.12.2021 21:32, Rafael J. Wysocki пишет:
quoted
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko [off-list ref] wrote:
quoted
Emit warning if unregister_restart_handler() fails since it never should
fail. This will ease further API development by catching mistakes early.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
kernel/reboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The only reason why it can fail is if the object pointed to by nb is
not in the chain.
I had exactly this case where object wasn't in the chain due to a bug
and this warning was very helpful.
During the development. In production it would be rather annoying.
quoted
quoted
Why WARN() about this? And what about systems with
panic_on_warn set?
That warning condition will never happen normally, only when something
is seriously wrong.
Those systems with panic_on_warn will get what was they asked for.
They may not be asking for panicking on bugs in the reboot notifier
code, though. That's what your change is making them panic on.
Alright, I'll drop the warnings and turn the warning about uniqueness
into error or warning message.
On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko [off-list ref] wrote:
quoted
10.12.2021 21:27, Rafael J. Wysocki пишет:
quoted
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko [off-list ref] wrote:
quoted
29.11.2021 03:26, Michał Mirosław пишет:
quoted
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
quoted
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
We can't just change the registration order because invocation order of
the call chain depends on the registration order
It doesn't if unique priorities are required and isn't that what you want?
quoted
and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
Well, the WARN() doesn't help much then.
Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
In which case adding the WARN() is not appropriate IMV.
Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter. I'm not sure what
the purpose of this patch is TBH.
The purpose is to let developer know that driver needs to be corrected.
quoted
The potential problem is on the user's side and the warning is intended
to aid the user.
Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about. IOW,
WARN() helps developers, not users.
quoted
We can make it a strong requirement, but only after converting and
testing all kernel drivers.
Right.
quoted
I'll consider to add patches for that.
But can you avoid adding more patches to this series?
I won't add more patches since such patches can be added only after
completion of transition to the new API of the whole kernel.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
In which case adding the WARN() is not appropriate IMV.
Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter. I'm not sure what
the purpose of this patch is TBH.
The purpose is to let developer know that driver needs to be corrected.
quoted
quoted
The potential problem is on the user's side and the warning is intended
to aid the user.
Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about. IOW,
WARN() helps developers, not users.
quoted
We can make it a strong requirement, but only after converting and
testing all kernel drivers.
Right.
quoted
I'll consider to add patches for that.
But can you avoid adding more patches to this series?
I won't add more patches since such patches can be added only after
completion of transition to the new API of the whole kernel.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
In which case adding the WARN() is not appropriate IMV.
Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter. I'm not sure what
the purpose of this patch is TBH.
The purpose is to let developer know that driver needs to be corrected.
quoted
quoted
The potential problem is on the user's side and the warning is intended
to aid the user.
Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about. IOW,
WARN() helps developers, not users.
quoted
We can make it a strong requirement, but only after converting and
testing all kernel drivers.
Right.
quoted
I'll consider to add patches for that.
But can you avoid adding more patches to this series?
I won't add more patches since such patches can be added only after
completion of transition to the new API of the whole kernel.
Not really, they only prevent the race from occurring while
notifier_has_unique_priority() is running.
If anyone depends on this check for correctness, they need to lock the
rwsem, do the check, do the thing depending on the check while holding
the rwsem and then release the rwsem. Otherwise it is racy.
It's fine that it's a bit "racy" since in the context of this series. We
always do the check after adding new entry, so it's not a problem.
There are two options:
1. Use blocking_notifier_has_unique_priority() like it's done in this
patchset. Remove it after all drivers are converted to the new API and
add blocking_notifier_chain_register_unique().
2. Add blocking_notifier_chain_register_unique(), but don't let it fail
the registration of non-unique entries until all drivers are converted
to the new API.
There is third, perhaps the best option:
3. Add blocking_notifier_chain_register_unique() and fall back to
blocking_notifier_chain_register() if unique fails, do it until all
drivers are converted to the new API.
On Fri, Dec 10, 2021 at 8:14 PM Rafael J. Wysocki [off-list ref] wrote:
On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko [off-list ref] wrote:
quoted
10.12.2021 21:27, Rafael J. Wysocki пишет:
quoted
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko [off-list ref] wrote:
quoted
29.11.2021 03:26, Michał Mirosław пишет:
quoted
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
quoted
28.11.2021 03:28, Michał Mirosław пишет:
quoted
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
quoted
Add sanity check which ensures that there are no two restart handlers
registered with the same priority. Normally it's a direct sign of a
problem if two handlers use the same priority.
The patch doesn't ensure the property that there are no duplicated-priority
entries on the chain.
It's not the exact point of this patch.
quoted
I'd rather see a atomic_notifier_chain_register_unique() that returns
-EBUSY or something istead of adding an entry with duplicate priority.
That way it would need only one list traversal unless you want to
register the duplicate anyway (then you would call the older
atomic_notifier_chain_register() after reporting the error).
The point of this patch is to warn developers about the problem that
needs to be fixed. We already have such troubling drivers in mainline.
It's not critical to register different handlers with a duplicated
priorities, but such cases really need to be corrected. We shouldn't
break users' machines during transition to the new API, meanwhile
developers should take action of fixing theirs drivers.
quoted
(Or you could return > 0 when a duplicate is registered in
atomic_notifier_chain_register() if the callers are prepared
for that. I don't really like this way, though.)
I had a similar thought at some point before and decided that I'm not in
favor of this approach. It's nicer to have a dedicated function that
verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
We can't just change the registration order because invocation order of
the call chain depends on the registration order
It doesn't if unique priorities are required and isn't that what you want?
quoted
and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.
Well, the WARN() doesn't help much then.
Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
In which case adding the WARN() is not appropriate IMV.
Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter. I'm not sure what
the purpose of this patch is TBH.
quoted
The potential problem is on the user's side and the warning is intended
to aid the user.
Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about. IOW,
WARN() helps developers, not users.
Do panic_on_warn and reboot_on_panic play well with having a WARN()
in the reboot notifier handling?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds