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.