All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Douglas Anderson <dianders@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Jason Wessel <jason.wessel@windriver.com>,
	kgdb-bugreport@lists.sourceforge.net,
	Christophe Leroy <christophe.leroy@c-s.fr>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn't round up
Date: Mon, 7 Oct 2019 14:54:59 +0100	[thread overview]
Message-ID: <20191007135459.lj3qc2tqzcv3xcia@holly.lan> (raw)
In-Reply-To: <20190925125811.v3.3.Id33c06cbd1516b49820faccd80da01c7c4bf15c7@changeid>

On Wed, Sep 25, 2019 at 01:02:19PM -0700, Douglas Anderson wrote:
> 
> I noticed that when I did "btc <cpu>" and the CPU I passed in hadn't
> rounded up that I'd crash.  I was going to copy the same fix from
> commit 162bc7f5afd7 ("kdb: Don't back trace on a cpu that didn't round
> up") into the "not all the CPUs" case, but decided it'd be better to
> clean things up a little bit.
> 
> This consolidates the two code paths.  It is _slightly_ wasteful in in
> that the checks for "cpu" being too small or being offline isn't
> really needed when we're iterating over all online CPUs, but that
> really shouldn't hurt.  Better to have the same code path.
> 
> While at it, eliminate at least one slightly ugly (and totally
> needless) recursive use of kdb_parse().
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Patch ("kdb: Fix "btc <cpu>" crash if the CPU...") new for v3.
> 
> Changes in v2: None
> 
>  kernel/debug/kdb/kdb_bt.c | 61 ++++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
> index 120fc686c919..d9af139f9a31 100644
> --- a/kernel/debug/kdb/kdb_bt.c
> +++ b/kernel/debug/kdb/kdb_bt.c
> @@ -101,6 +101,27 @@ kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
>  	return 0;
>  }
>  
> +static void
> +kdb_bt_cpu(unsigned long cpu)
> +{
> +	struct task_struct *kdb_tsk;
> +
> +	if (cpu >= num_possible_cpus() || !cpu_online(cpu)) {
> +		kdb_printf("WARNING: no process for cpu %ld\n", cpu);
> +		return;
> +	}
> +
> +	/* If a CPU failed to round up we could be here */
> +	kdb_tsk = KDB_TSK(cpu);
> +	if (!kdb_tsk) {
> +		kdb_printf("WARNING: no task for cpu %ld\n", cpu);
> +		return;
> +	}
> +
> +	kdb_set_current_task(kdb_tsk);
> +	kdb_bt1(kdb_tsk, ~0UL, false);
> +}
> +
>  int
>  kdb_bt(int argc, const char **argv)
>  {
> @@ -161,7 +182,6 @@ kdb_bt(int argc, const char **argv)
>  	} else if (strcmp(argv[0], "btc") == 0) {
>  		unsigned long cpu = ~0;
>  		struct task_struct *save_current_task = kdb_current_task;
> -		char buf[80];
>  		if (argc > 1)
>  			return KDB_ARGCOUNT;
>  		if (argc == 1) {
> @@ -169,35 +189,22 @@ kdb_bt(int argc, const char **argv)
>  			if (diag)
>  				return diag;
>  		}
> -		/* Recursive use of kdb_parse, do not use argv after
> -		 * this point */
> -		argv = NULL;
>  		if (cpu != ~0) {
> -			if (cpu >= num_possible_cpus() || !cpu_online(cpu)) {
> -				kdb_printf("no process for cpu %ld\n", cpu);
> -				return 0;
> -			}
> -			sprintf(buf, "btt 0x%px\n", KDB_TSK(cpu));
> -			kdb_parse(buf);
> -			return 0;
> -		}
> -		kdb_printf("btc: cpu status: ");
> -		kdb_parse("cpu\n");
> -		for_each_online_cpu(cpu) {
> -			void *kdb_tsk = KDB_TSK(cpu);
> -
> -			/* If a CPU failed to round up we could be here */
> -			if (!kdb_tsk) {
> -				kdb_printf("WARNING: no task for cpu %ld\n",
> -					   cpu);
> -				continue;
> +			kdb_bt_cpu(cpu);
> +		} else {
> +			/*
> +			 * Recursive use of kdb_parse, do not use argv after
> +			 * this point.
> +			 */
> +			argv = NULL;
> +			kdb_printf("btc: cpu status: ");
> +			kdb_parse("cpu\n");
> +			for_each_online_cpu(cpu) {
> +				kdb_bt_cpu(cpu);
> +				touch_nmi_watchdog();
>  			}
> -
> -			sprintf(buf, "btt 0x%px\n", kdb_tsk);
> -			kdb_parse(buf);
> -			touch_nmi_watchdog();
> +			kdb_set_current_task(save_current_task);
>  		}
> -		kdb_set_current_task(save_current_task);

Why does this move out into only one of the conditional branches?
Don't both of the above paths modify the current task?


Daniel.


>  		return 0;
>  	} else {
>  		if (argc) {
> -- 
> 2.23.0.351.gc4317032e6-goog
> 

  parent reply	other threads:[~2019-10-07 13:55 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25 20:02 [PATCH v3 0/4] kdb: Fixes for btc Douglas Anderson
2019-09-25 20:02 ` [PATCH v3 1/4] kgdb: Remove unused DCPU_SSTEP definition Douglas Anderson
2019-09-25 20:08   ` Jason Wessel
2019-09-25 20:02 ` [PATCH v3 2/4] kdb: Remove unused "argcount" param from kdb_bt1(); make btaprompt bool Douglas Anderson
2019-09-25 20:02 ` [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn't round up Douglas Anderson
2019-10-03 21:09   ` Will Deacon
2019-10-07 13:54   ` Daniel Thompson [this message]
2019-10-07 23:34     ` Doug Anderson
2019-10-10 15:07       ` Daniel Thompson
2019-10-10 16:38         ` Doug Anderson
2019-11-09 19:20           ` Doug Anderson
2019-09-25 20:02 ` [PATCH v3 4/4] kdb: Fix stack crawling on 'running' CPUs that aren't the master Douglas Anderson
2019-10-03 21:11 ` [PATCH v3 0/4] kdb: Fixes for btc Will Deacon
2019-10-10 16:51 ` Daniel Thompson

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=20191007135459.lj3qc2tqzcv3xcia@holly.lan \
    --to=daniel.thompson@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=christophe.leroy@c-s.fr \
    --cc=dianders@chromium.org \
    --cc=jason.wessel@windriver.com \
    --cc=kgdb-bugreport@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn'\''t round up' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.