From: Derek Dolney <z23@posteo.net>
To: Vincent Donnefort <vdonnefort@google.com>,
peterz@infradead.org, tglx@linutronix.de
Cc: linux-kernel@vger.kernel.org, vschneid@redhat.com,
kernel-team@android.com
Subject: Re: [PATCH v2] cpu/hotplug: Do not bail-out in DYING/STARTING sections
Date: Thu, 26 May 2022 12:27:26 +0000 [thread overview]
Message-ID: <77d0ed9b-8521-c4a9-69c4-ec82cba81e61@posteo.net> (raw)
In-Reply-To: <20220523160536.2889162-1-vdonnefort@google.com>
Tested by: Derek Dolney <z23@posteo.net>
Suspend and resume with tboot are working again with this patch. Tested
on 5.12 broken commit and latest git 5.18.
On 5/23/22 12:05 PM, Vincent Donnefort wrote:
> The DYING/STARTING callbacks are not expected to fail. However, as reported
> by Derek, drivers such as tboot are still free to return errors within
> those sections. In that case, there's nothing the hotplug machinery can do,
> so let's just proceed and log the failures.
>
> Fixes: 453e41085183 (cpu/hotplug: Add cpuhp_invoke_callback_range())
> Reported-by: Derek Dolney <z23@posteo.net>
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
>
> ---
>
> v1 -> v2:
> - Commit message rewording.
> - More details in the warnings.
> - Some variable renaming
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index bbad5e375d3b..c3617683459e 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -663,21 +663,51 @@ static bool cpuhp_next_state(bool bringup,
> return true;
> }
>
> -static int cpuhp_invoke_callback_range(bool bringup,
> - unsigned int cpu,
> - struct cpuhp_cpu_state *st,
> - enum cpuhp_state target)
> +static int _cpuhp_invoke_callback_range(bool bringup,
> + unsigned int cpu,
> + struct cpuhp_cpu_state *st,
> + enum cpuhp_state target,
> + bool nofail)
> {
> enum cpuhp_state state;
> - int err = 0;
> + int ret = 0;
>
> while (cpuhp_next_state(bringup, &state, st, target)) {
> + int err;
> +
> err = cpuhp_invoke_callback(cpu, state, bringup, NULL, NULL);
> - if (err)
> + if (!err)
> + continue;
> +
> + if (nofail) {
> + pr_warn("CPU %u %s state %s (%d) failed (%d)\n",
> + cpu, bringup ? "UP" : "DOWN",
> + cpuhp_get_step(st->state)->name,
> + st->state, err);
> + ret = -1;
> + } else {
> + ret = err;
> break;
> + }
> }
>
> - return err;
> + return ret;
> +}
> +
> +static inline int cpuhp_invoke_callback_range(bool bringup,
> + unsigned int cpu,
> + struct cpuhp_cpu_state *st,
> + enum cpuhp_state target)
> +{
> + return _cpuhp_invoke_callback_range(bringup, cpu, st, target, false);
> +}
> +
> +static inline void cpuhp_invoke_callback_range_nofail(bool bringup,
> + unsigned int cpu,
> + struct cpuhp_cpu_state *st,
> + enum cpuhp_state target)
> +{
> + WARN_ON_ONCE(_cpuhp_invoke_callback_range(bringup, cpu, st, target, true));
> }
>
> static inline bool can_rollback_cpu(struct cpuhp_cpu_state *st)
> @@ -999,7 +1029,6 @@ static int take_cpu_down(void *_param)
> struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
> enum cpuhp_state target = max((int)st->target, CPUHP_AP_OFFLINE);
> int err, cpu = smp_processor_id();
> - int ret;
>
> /* Ensure this CPU doesn't handle any more interrupts. */
> err = __cpu_disable();
> @@ -1012,13 +1041,11 @@ static int take_cpu_down(void *_param)
> */
> WARN_ON(st->state != (CPUHP_TEARDOWN_CPU - 1));
>
> - /* Invoke the former CPU_DYING callbacks */
> - ret = cpuhp_invoke_callback_range(false, cpu, st, target);
> -
> /*
> + * Invoke the former CPU_DYING callbacks
> * DYING must not fail!
> */
> - WARN_ON_ONCE(ret);
> + cpuhp_invoke_callback_range_nofail(false, cpu, st, target);
>
> /* Give up timekeeping duties */
> tick_handover_do_timer();
> @@ -1296,16 +1323,14 @@ void notify_cpu_starting(unsigned int cpu)
> {
> struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
> enum cpuhp_state target = min((int)st->target, CPUHP_AP_ONLINE);
> - int ret;
>
> rcu_cpu_starting(cpu); /* Enables RCU usage on this CPU. */
> cpumask_set_cpu(cpu, &cpus_booted_once_mask);
> - ret = cpuhp_invoke_callback_range(true, cpu, st, target);
>
> /*
> * STARTING must not fail!
> */
> - WARN_ON_ONCE(ret);
> + cpuhp_invoke_callback_range_nofail(true, cpu, st, target);
> }
>
> /*
>
next prev parent reply other threads:[~2022-05-26 12:29 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-23 16:05 [PATCH v2] cpu/hotplug: Do not bail-out in DYING/STARTING sections Vincent Donnefort
2022-05-25 16:52 ` Peter Zijlstra
2022-05-26 8:24 ` Vincent Donnefort
2022-05-26 10:15 ` Peter Zijlstra
2022-05-29 18:01 ` Derek Dolney
2022-05-26 11:48 ` Derek Dolney
2022-05-26 12:27 ` Derek Dolney [this message]
2022-06-13 12:36 ` Thomas Gleixner
2022-06-13 13:37 ` Vincent Donnefort
2022-07-04 10:01 ` Thorsten Leemhuis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=77d0ed9b-8521-c4a9-69c4-ec82cba81e61@posteo.net \
--to=z23@posteo.net \
--cc=kernel-team@android.com \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=vdonnefort@google.com \
--cc=vschneid@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).