| Message ID | 20220411233832.391817-1-dmitry.osipenko@collabora.com |
|---|---|
| Headers | show |
| Series | Introduce power-off+restart call chain API | expand |
Hi Dmitry, On Tue, Apr 12, 2022 at 1:38 AM Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > 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. > Changelog: > > v7: - Rebased on a recent linux-next. Dropped the recently removed > NDS32 architecture. Only SH and x86 arches left un-acked. > > - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski > to the MIPS and memory/emif patches respectively. Looks like you forgot to add the actual acks? 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
On 4/12/22 10:06, Geert Uytterhoeven wrote: > Hi Dmitry, > > On Tue, Apr 12, 2022 at 1:38 AM Dmitry Osipenko > <dmitry.osipenko@collabora.com> wrote: >> 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. > >> Changelog: >> >> v7: - Rebased on a recent linux-next. Dropped the recently removed >> NDS32 architecture. Only SH and x86 arches left un-acked. >> >> - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski >> to the MIPS and memory/emif patches respectively. > > Looks like you forgot to add the actual acks? Good catch, thank you! Indeed, I sent out the version without the acks, but luckily it's only the acks that are missing, the code is fine.
On 4/12/22 02:38, Dmitry Osipenko wrote: > Replace legacy pm_power_off with kernel_can_power_off() helper that > is aware about chained power-off handlers. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/memory/emif.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/memory/emif.c b/drivers/memory/emif.c > index edf3ba7447ed..fa6845313a43 100644 > --- a/drivers/memory/emif.c > +++ b/drivers/memory/emif.c > @@ -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"); Adding ack from Krzysztof that he gave to v6. It's missing in v7 by accident. Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > Add sanity check which ensures that there are no two restart handlers > registered using the same priority. This requirement will become mandatory > once all drivers will be converted to the new API and such errors will be > fixed. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> The first two patches in the series are fine with me and there's only one minor nit regarding this one (below). > --- > kernel/reboot.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/kernel/reboot.c b/kernel/reboot.c > index ed4e6dfb7d44..acdae4e95061 100644 > --- a/kernel/reboot.c > +++ b/kernel/reboot.c > @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); > */ > int register_restart_handler(struct notifier_block *nb) > { > + int ret; > + > + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); > + if (ret != -EBUSY) > + return ret; > + > + /* > + * Handler must have unique priority. Otherwise call order is > + * determined by registration order, which is unreliable. > + * > + * This requirement will become mandatory once all drivers > + * will be converted to use new sys-off API. > + */ > + pr_err("failed to register restart handler using unique priority\n"); I would use pr_info() here, because this is not a substantial error AFAICS. > + > return atomic_notifier_chain_register(&restart_handler_list, nb); > } > EXPORT_SYMBOL(register_restart_handler); > --
On 4/13/22 21:48, Rafael J. Wysocki wrote: > On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko > <dmitry.osipenko@collabora.com> wrote: >> >> Add sanity check which ensures that there are no two restart handlers >> registered using the same priority. This requirement will become mandatory >> once all drivers will be converted to the new API and such errors will be >> fixed. >> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > The first two patches in the series are fine with me and there's only > one minor nit regarding this one (below). > >> --- >> kernel/reboot.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/kernel/reboot.c b/kernel/reboot.c >> index ed4e6dfb7d44..acdae4e95061 100644 >> --- a/kernel/reboot.c >> +++ b/kernel/reboot.c >> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); >> */ >> int register_restart_handler(struct notifier_block *nb) >> { >> + int ret; >> + >> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); >> + if (ret != -EBUSY) >> + return ret; >> + >> + /* >> + * Handler must have unique priority. Otherwise call order is >> + * determined by registration order, which is unreliable. >> + * >> + * This requirement will become mandatory once all drivers >> + * will be converted to use new sys-off API. >> + */ >> + pr_err("failed to register restart handler using unique priority\n"); > > I would use pr_info() here, because this is not a substantial error AFAICS. It's indeed not a substantial error so far, but it will become substantial later on once only unique priorities will be allowed. The pr_warn() could be a good compromise here, pr_info() is too mild, IMO.
On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > On 4/13/22 21:48, Rafael J. Wysocki wrote: > > On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko > > <dmitry.osipenko@collabora.com> wrote: > >> > >> Add sanity check which ensures that there are no two restart handlers > >> registered using the same priority. This requirement will become mandatory > >> once all drivers will be converted to the new API and such errors will be > >> fixed. > >> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > > > The first two patches in the series are fine with me and there's only > > one minor nit regarding this one (below). > > > >> --- > >> kernel/reboot.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/kernel/reboot.c b/kernel/reboot.c > >> index ed4e6dfb7d44..acdae4e95061 100644 > >> --- a/kernel/reboot.c > >> +++ b/kernel/reboot.c > >> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); > >> */ > >> int register_restart_handler(struct notifier_block *nb) > >> { > >> + int ret; > >> + > >> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); > >> + if (ret != -EBUSY) > >> + return ret; > >> + > >> + /* > >> + * Handler must have unique priority. Otherwise call order is > >> + * determined by registration order, which is unreliable. > >> + * > >> + * This requirement will become mandatory once all drivers > >> + * will be converted to use new sys-off API. > >> + */ > >> + pr_err("failed to register restart handler using unique priority\n"); > > > > I would use pr_info() here, because this is not a substantial error AFAICS. > > It's indeed not a substantial error so far, but it will become > substantial later on once only unique priorities will be allowed. The > pr_warn() could be a good compromise here, pr_info() is too mild, IMO. Well, I'm still unconvinced about requiring all of the users of this interface to use unique priorities. Arguably, there are some of them who don't really care about the ordering, so could there be an option for them to specify the lack of care by, say, passing 0 as the priority that would be regarded as a special case? IOW, if you pass 0, you'll be run along the others who've also passed 0, but if you pass anything different from 0, it must be unique. What do you think?
On 4/14/22 14:19, Rafael J. Wysocki wrote: > On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko > <dmitry.osipenko@collabora.com> wrote: >> >> On 4/13/22 21:48, Rafael J. Wysocki wrote: >>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko >>> <dmitry.osipenko@collabora.com> wrote: >>>> >>>> Add sanity check which ensures that there are no two restart handlers >>>> registered using the same priority. This requirement will become mandatory >>>> once all drivers will be converted to the new API and such errors will be >>>> fixed. >>>> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> >>> The first two patches in the series are fine with me and there's only >>> one minor nit regarding this one (below). >>> >>>> --- >>>> kernel/reboot.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/kernel/reboot.c b/kernel/reboot.c >>>> index ed4e6dfb7d44..acdae4e95061 100644 >>>> --- a/kernel/reboot.c >>>> +++ b/kernel/reboot.c >>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); >>>> */ >>>> int register_restart_handler(struct notifier_block *nb) >>>> { >>>> + int ret; >>>> + >>>> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); >>>> + if (ret != -EBUSY) >>>> + return ret; >>>> + >>>> + /* >>>> + * Handler must have unique priority. Otherwise call order is >>>> + * determined by registration order, which is unreliable. >>>> + * >>>> + * This requirement will become mandatory once all drivers >>>> + * will be converted to use new sys-off API. >>>> + */ >>>> + pr_err("failed to register restart handler using unique priority\n"); >>> >>> I would use pr_info() here, because this is not a substantial error AFAICS. >> >> It's indeed not a substantial error so far, but it will become >> substantial later on once only unique priorities will be allowed. The >> pr_warn() could be a good compromise here, pr_info() is too mild, IMO. > > Well, I'm still unconvinced about requiring all of the users of this > interface to use unique priorities. > > Arguably, there are some of them who don't really care about the > ordering, so could there be an option for them to specify the lack of > care by, say, passing 0 as the priority that would be regarded as a > special case? > > IOW, if you pass 0, you'll be run along the others who've also passed > 0, but if you pass anything different from 0, it must be unique. What > do you think? There are indeed cases where ordering is unimportant. Like a case of PMIC and watchdog restart handlers for example, both handlers will produce equal effect from a user's perspective. Perhaps indeed it's more practical to have at least one shared level. In this patchset the level 0 is specified as an alias to the default level 128. If one user registers handler using unique level 128 and the other user uses non-unique level 0, then we have ambiguity. One potential option is to make the whole default level 128 non-unique. This will allow users to not care about the uniqueness by default like they always did it previously, but it will hide potential problems for users who actually need unique level and don't know about it yet due to a lucky registration ordering that they have today. Are you okay with this option?
On Mon, Apr 18, 2022 at 3:29 AM Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > On 4/14/22 14:19, Rafael J. Wysocki wrote: > > On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko > > <dmitry.osipenko@collabora.com> wrote: > >> > >> On 4/13/22 21:48, Rafael J. Wysocki wrote: > >>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko > >>> <dmitry.osipenko@collabora.com> wrote: > >>>> > >>>> Add sanity check which ensures that there are no two restart handlers > >>>> registered using the same priority. This requirement will become mandatory > >>>> once all drivers will be converted to the new API and such errors will be > >>>> fixed. > >>>> > >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>> > >>> The first two patches in the series are fine with me and there's only > >>> one minor nit regarding this one (below). > >>> > >>>> --- > >>>> kernel/reboot.c | 15 +++++++++++++++ > >>>> 1 file changed, 15 insertions(+) > >>>> > >>>> diff --git a/kernel/reboot.c b/kernel/reboot.c > >>>> index ed4e6dfb7d44..acdae4e95061 100644 > >>>> --- a/kernel/reboot.c > >>>> +++ b/kernel/reboot.c > >>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); > >>>> */ > >>>> int register_restart_handler(struct notifier_block *nb) > >>>> { > >>>> + int ret; > >>>> + > >>>> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); > >>>> + if (ret != -EBUSY) > >>>> + return ret; > >>>> + > >>>> + /* > >>>> + * Handler must have unique priority. Otherwise call order is > >>>> + * determined by registration order, which is unreliable. > >>>> + * > >>>> + * This requirement will become mandatory once all drivers > >>>> + * will be converted to use new sys-off API. > >>>> + */ > >>>> + pr_err("failed to register restart handler using unique priority\n"); > >>> > >>> I would use pr_info() here, because this is not a substantial error AFAICS. > >> > >> It's indeed not a substantial error so far, but it will become > >> substantial later on once only unique priorities will be allowed. The > >> pr_warn() could be a good compromise here, pr_info() is too mild, IMO. > > > > Well, I'm still unconvinced about requiring all of the users of this > > interface to use unique priorities. > > > > Arguably, there are some of them who don't really care about the > > ordering, so could there be an option for them to specify the lack of > > care by, say, passing 0 as the priority that would be regarded as a > > special case? > > > > IOW, if you pass 0, you'll be run along the others who've also passed > > 0, but if you pass anything different from 0, it must be unique. What > > do you think? > > There are indeed cases where ordering is unimportant. Like a case of > PMIC and watchdog restart handlers for example, both handlers will > produce equal effect from a user's perspective. Perhaps indeed it's more > practical to have at least one shared level. > > In this patchset the level 0 is specified as an alias to the default > level 128. If one user registers handler using unique level 128 and the > other user uses non-unique level 0, then we have ambiguity. > > One potential option is to make the whole default level 128 non-unique. > This will allow users to not care about the uniqueness by default like > they always did it previously, but it will hide potential problems for > users who actually need unique level and don't know about it yet due to > a lucky registration ordering that they have today. Are you okay with > this option? Yes, I am.
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 the new API and convert arch code to use do_kernel_power_off(). For now the new API will co-exist with the older API. 2. Convert all drivers and platform code to the new API. 3. Remove obsoleted pm_power_off and pm_power_off_prepare variables. 4. Make restart-notifier API private to kernel/reboot.c once no users left. 5. Make unique-priority of the handlers' a mandatory requirement in the new API. The plan is fully implemented here: [1] https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/commits/sys-off-handler For now I'm sending the first 20 base patches out of ~180. Majority of drivers and platform patches depend on the base patches, hence the rest will come later on, once base will land. All [1] patches are compile-tested. Tegra, Rockchip and x86 ACPI patches are tested on hardware. 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 #4 which adds verification of restart handlers priorities, ensuring that they are unique. Changelog: v7: - Rebased on a recent linux-next. Dropped the recently removed NDS32 architecture. Only SH and x86 arches left un-acked. - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski to the MIPS and memory/emif patches respectively. - Made couple minor cosmetic improvements to the new API. - A month ago I joined Collabora and continuing to work on this series on the company's time, so changed my email address to collabora.com v6: - Rebased on a recent linux-next. - Made minor couple cosmetic changes. v5: - Dropped patches which cleaned up notifier/reboot headers, as was requested by Rafael Wysocki. - Dropped WARN_ON() from the code, as was requested by Rafael Wysocki. Replaced it with pr_err() appropriately. - Dropped *_notifier_has_unique_priority() functions and added *_notifier_chain_register_unique_prio() instead, as was suggested by Michał Mirosław and Rafael Wysocki. - Dropped export of blocking_notifier_call_chain_is_empty() symbol, as was suggested by Rafael Wysocki. - Michał Mirosław suggested that will be better to split up patch that adds the new API to ease reviewing, but Rafael Wysocki asked not add more patches, so I kept it as a single patch. - Added temporary "weak" stub for pm_power_off() which fixes linkage failure once symbol is removed from arch/* code. Previously I missed this problem because was only compile-testing object files. 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 (20): notifier: Add blocking_notifier_call_chain_is_empty() notifier: Add atomic/blocking_notifier_chain_register_unique_prio() reboot: Print error message if restart handler has duplicated priority 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() 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/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 | 7 + include/linux/pm.h | 1 - include/linux/reboot.h | 229 ++++++++- kernel/notifier.c | 101 +++- kernel/power/hibernate.c | 2 +- kernel/reboot.c | 622 ++++++++++++++++++++++++- 27 files changed, 980 insertions(+), 116 deletions(-)