Re: [PATCHv12 4/4] watchdog/softlockup: report the most frequent interrupts

[Doug Anderson]

Hi,

On Mon, Mar 25, 2024 at 2:48 AM Bitao Hu  wrote:
>
> Hi, Thomas
>
> On 2024/3/24 04:43, Thomas Gleixner wrote:
> > On Wed, Mar 06 2024 at 20:52, Bitao Hu wrote:
> >> +if (__this_cpu_read(snapshot_taken)) {
> >> +for_each_active_irq(i) {
> >> +count = kstat_get_irq_since_snapshot(i);
> >> +tabulate_irq_count(irq_counts_sorted, i, count, 
> >> NUM_HARDIRQ_REPORT);
> >> +}
> >> +
> >> +/*
> >> + * We do not want the "watchdog: " prefix on every line,
> >> + * hence we use "printk" instead of "pr_crit".
> >> + */
> >
> > You are not providing any justification why the prefix is not
> > wanted. Just saying 'We do not want' does not cut it and who is 'We'. I
> > certainly not.
> >
> > I really disagree because the prefixes are very useful for searching log
> > files. So not having it makes it harder to filter out for no reason.
> >
>
>
> Regarding the use of printk() instead of pr_crit(), I have had a
> discussion with Liu Song and Douglas in PATCHv1:
> https://lore.kernel.org/all/CAD=FV=WEEQeKX=ec3gr-8cks2k0mawn3v0-0yosuret0qcb...@mail.gmail.com/
>
> Please allow me to elaborate on my reasoning. The purpose of the
> report_cpu_status() function I implemented is similar to that of
> print_modules(), show_regs(), and dump_stack(). These functions are
> designed to assist in analyzing the causes of a soft lockup, rather
> than to report that a soft lockup has occurred. Therefore, I think
> that adding the "watchdog: " prefix to every line is redundant and
> not concise. Besides, the existing pr_emerg() in the watchdog.c file
> is already sufficient for searching useful information in the logs.
> The information I added, along with the call tree and other data, is
> located near the line with the "watchdog: " prefix.
>
> Are the two reasons I've provided reasonable?

FWIW I don't feel super strongly about this, but I'm leaning towards
agreeing with Bitao. The sample output from the commit message looks
like this:

[  638.870231] watchdog: BUG: soft lockup - CPU#9 stuck for 26s! [swapper/9:0]
[  638.870825] CPU#9 Utilization every 4s during lockup:
[  638.871194]  #1:   0% system,  0% softirq,   100% hardirq,
   0% idle
[  638.871652]  #2:   0% system,  0% softirq,   100% hardirq,
   0% idle
[  638.872107]  #3:   0% system,  0% softirq,   100% hardirq,
   0% idle
[  638.872563]  #4:   0% system,  0% softirq,   100% hardirq,
   0% idle
[  638.873018]  #5:   0% system,  0% softirq,   100% hardirq,
   0% idle
[  638.873494] CPU#9 Detect HardIRQ Time exceeds 50%. Most frequent HardIRQs:
[  638.873994]  #1: 330945  irq#7
[  638.874236]  #2: 31  irq#82
[  638.874493]  #3: 10  irq#10
[  638.874744]  #4: 2   irq#89
[  638.874992]  #5: 1   irq#102

...and in my mind the "watchdog: BUG: soft lockup - CPU#9 stuck for
26s! [swapper/9:0]" line is enough to grep through the dmesg. Having
all the following lines start with "watchdog:" feels like overkill to
me, but if you feel strongly that they should then it wouldn't bother
me too much for them all to have the "watchdog:" prefix.

Could you clarify how strongly you feel about this and whether Bitao
should spin a v13?

I believe that this is the only point of contention on the patch
series right now and otherwise it could be ready to land. I know in
the past Petr has wanted ample time to comment though perhaps the fact
that it's been ~1 month is enough. Petr: do you have anything that
needs saying before this patch series lands?

Thanks!

-Doug

Re: [PATCHv11 4/4] watchdog/softlockup: report the most frequent interrupts

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 11:22 PM Bitao Hu  wrote:
>
> When the watchdog determines that the current soft lockup is due
> to an interrupt storm based on CPU utilization, reporting the
> most frequent interrupts could be good enough for further
> troubleshooting.
>
> Below is an example of interrupt storm. The call tree does not
> provide useful information, but we can analyze which interrupt
> caused the soft lockup by comparing the counts of interrupts.
>
> [  638.870231] watchdog: BUG: soft lockup - CPU#9 stuck for 26s! [swapper/9:0]
> [  638.870825] CPU#9 Utilization every 4s during lockup:
> [  638.871194]  #1:   0% system,  0% softirq,   100% hardirq, 0% 
> idle
> [  638.871652]  #2:   0% system,  0% softirq,   100% hardirq, 0% 
> idle
> [  638.872107]  #3:   0% system,  0% softirq,   100% hardirq, 0% 
> idle
> [  638.872563]  #4:   0% system,  0% softirq,   100% hardirq, 0% 
> idle
> [  638.873018]  #5:   0% system,  0% softirq,   100% hardirq, 0% 
> idle
> [  638.873494] CPU#9 Detect HardIRQ Time exceeds 50%. Most frequent HardIRQs:
> [  638.873994]  #1: 330945  irq#7
> [  638.874236]  #2: 31  irq#82
> [  638.874493]  #3: 10  irq#10
> [  638.874744]  #4: 2   irq#89
> [  638.874992]  #5: 1   irq#102
> ...
> [  638.875313] Call trace:
> [  638.875315]  __do_softirq+0xa8/0x364
>
> Signed-off-by: Bitao Hu 
> Reviewed-by: Liu Song 
> ---
>  kernel/watchdog.c | 115 --
>  1 file changed, 111 insertions(+), 4 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCHv11 3/4] genirq: Avoid summation loops for /proc/interrupts

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 11:22 PM Bitao Hu  wrote:
>
> show_interrupts() unconditionally accumulates the per CPU interrupt
> statistics to determine whether an interrupt was ever raised.
>
> This can be avoided for all interrupts which are not strictly per CPU
> and not of type NMI because those interrupts provide already an
> accumulated counter. The required logic is already implemented in
> kstat_irqs().
>
> Split the inner access logic out of kstat_irqs() and use it for
> kstat_irqs() and show_interrupts() to avoid the accumulation loop
> when possible.
>
> Originally-by: Thomas Gleixner 
> Signed-off-by: Bitao Hu 
> Reviewed-by: Liu Song 
> ---
>  kernel/irq/internals.h |  2 ++
>  kernel/irq/irqdesc.c   | 16 +++-
>  kernel/irq/proc.c  |  6 ++
>  3 files changed, 15 insertions(+), 9 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCHv11 2/4] genirq: Provide a snapshot mechanism for interrupt statistics

[Doug Anderson]

Hi,

On Tue, Feb 27, 2024 at 11:22 PM Bitao Hu  wrote:
>
> The soft lockup detector lacks a mechanism to identify interrupt storms
> as root cause of a lockup. To enable this the detector needs a
> mechanism to snapshot the interrupt count statistics on a CPU when the
> detector observes a potential lockup scenario and compare that against
> the interrupt count when it warns about the lockup later on. The number
> of interrupts in that period give a hint whether the lockup might be
> caused by an interrupt storm.
>
> Instead of having extra storage in the lockup detector and accessing
> the internals of the interrupt descriptor directly, convert the per CPU
> irq_desc::kstat_irq member to a data structure which contains the
> counter plus a snapshot member and provide interfaces to take a
> snapshot of all interrupts on the current CPU and to retrieve the delta
> of a specific interrupt later on.
>
> Originally-by: Thomas Gleixner 
> Signed-off-by: Bitao Hu 
> Reviewed-by: Liu Song 
> ---
>  arch/mips/dec/setup.c|  2 +-
>  arch/parisc/kernel/smp.c |  2 +-
>  arch/powerpc/kvm/book3s_hv_rm_xics.c |  2 +-
>  include/linux/irqdesc.h  | 14 ++--
>  include/linux/kernel_stat.h  |  3 +++
>  kernel/irq/internals.h   |  2 +-
>  kernel/irq/irqdesc.c | 34 ++--
>  kernel/irq/proc.c|  5 ++--
>  scripts/gdb/linux/interrupts.py  |  6 ++---
>  9 files changed, 51 insertions(+), 19 deletions(-)

I won't insist on it, but I continue to worry about memory
implications with large numbers of CPUs. With a 4-byte int, 8192 max
CPUs, and 100 IRQs the extra "ref" value takes up over 3MB of memory
(8192 * 4 bytes * 100).

Technically, you could add a new symbol like "config
NEED_IRQ_SNAPSHOTS". This wouldn't be a symbol selectable by the end
user but would automatically be selected by "config
SOFTLOCKUP_DETECTOR_INTR_STORM". If the config wasn't defined then the
struct wouldn't contain "ref" and the snapshot routines would just be
static inline stubs.

Maybe Thomas has an opinion about whether this is something to worry
about. Worst case it wouldn't be hard to do in a follow-up patch.

Everything else looks good to me. Given that I'm not insisting on
adding the extra CONFIG, I'm OK w/:

Reviewed-by: Douglas Anderson 

Re: [PATCH v3 1/2] nmi_backtrace: Allow excluding an arbitrary CPU

[Doug Anderson]

Hi,

On Fri, Aug 4, 2023 at 8:02 AM Michal Hocko  wrote:
>
> > > It would have been slightly safer to modify arch_trigger_cpumask_backtrace
> > > by switching arguments so that some leftovers are captured easier.
> >
> > I'm not sure I understand. Oh, you're saying make the prototype of
> > arch_trigger_cpumask_backtrace() incompatible so that if someone is
> > directly calling it then it'll be a compile-time error?
>
> exactly. bool to int promotion would be too easy to miss while the
> pointer to int would complain loudly.
>
> > I guess the
> > hope is that nobody is calling that directly and they're calling
> > through the trigger_...() functions.
>
> Hope is one thing, being preventive another.
>
> > For now I'm going to leave this alone.
>
> If you are going to send another version then please consider this. Not
> a hard requirement but better.

If I do send another version, do you have any suggestions for how to
change this to make it incompatible? I guess swapping the order of the
parameters would be best? I considered doing that for v4 but I felt
like long term the current order of the parameters was better. I also
considered a rename, but that different problems. ;-) If I rename both
the #define and the function then if someone has an out-of-tree patch
adding arch_trigger_cpumask_backtrace() for another architecture, like
say arm64, then there would be no compile-time failure indicating that
the out-of-tree patch needs updating. I could rename the functions but
_not_ the #define, I guess?

Re: [PATCH v3 1/2] nmi_backtrace: Allow excluding an arbitrary CPU

[Doug Anderson]

Hi,

On Fri, Aug 4, 2023 at 12:50 AM Michal Hocko  wrote:
>
> On Thu 03-08-23 16:07:57, Douglas Anderson wrote:
> > The APIs that allow backtracing across CPUs have always had a way to
> > exclude the current CPU. This convenience means callers didn't need to
> > find a place to allocate a CPU mask just to handle the common case.
> >
> > Let's extend the API to take a CPU ID to exclude instead of just a
> > boolean. This isn't any more complex for the API to handle and allows
> > the hardlockup detector to exclude a different CPU (the one it already
> > did a trace for) without needing to find space for a CPU mask.
> >
> > Arguably, this new API also encourages safer behavior. Specifically if
> > the caller wants to avoid tracing the current CPU (maybe because they
> > already traced the current CPU) this makes it more obvious to the
> > caller that they need to make sure that the current CPU ID can't
> > change.
>
> Yes, this looks like the best way forward.
>
> It would have been slightly safer to modify arch_trigger_cpumask_backtrace
> by switching arguments so that some leftovers are captured easier.

I'm not sure I understand. Oh, you're saying make the prototype of
arch_trigger_cpumask_backtrace() incompatible so that if someone is
directly calling it then it'll be a compile-time error? I guess the
hope is that nobody is calling that directly and they're calling
through the trigger_...() functions.

For now I'm going to leave this alone.


> You also have this leftover
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index 00982b133dc1..9f1743ee2b28 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -190,10 +190,6 @@ static inline bool trigger_all_cpu_backtrace(void)
>  {
> return false;
>  }
> -static inline bool trigger_allbutself_cpu_backtrace(void)
> -{
> -   return false;
> -}

Ah yes. I missed that case. Let me send a quick v4.


>  static inline bool trigger_cpumask_backtrace(struct cpumask *mask)
>  {
> return false;
>
> > Signed-off-by: Douglas Anderson 
>
> Anyway
> Acked-by: Michal Hocko 

Thanks!

Re: [PATCH v2 6/6] watchdog/hardlockup: Define HARDLOCKUP_DETECTOR_ARCH

[Doug Anderson]

Hi,

On Sat, Jul 1, 2023 at 7:40 AM Guenter Roeck  wrote:
>
> On Fri, Jun 16, 2023 at 05:06:18PM +0200, Petr Mladek wrote:
> > The HAVE_ prefix means that the code could be enabled. Add another
> > variable for HAVE_HARDLOCKUP_DETECTOR_ARCH without this prefix.
> > It will be set when it should be built. It will make it compatible
> > with the other hardlockup detectors.
> >
> > The change allows to clean up dependencies of PPC_WATCHDOG
> > and HAVE_HARDLOCKUP_DETECTOR_PERF definitions for powerpc.
> >
> > As a result HAVE_HARDLOCKUP_DETECTOR_PERF has the same dependencies
> > on arm, x86, powerpc architectures.
> >
> > Signed-off-by: Petr Mladek 
> > Reviewed-by: Douglas Anderson 
> > ---
> ...
> > --- a/include/linux/nmi.h
> > +++ b/include/linux/nmi.h
> > @@ -9,7 +9,7 @@
> >  #include 
> >
> >  /* Arch specific watchdogs might need to share extra watchdog-related 
> > APIs. */
> > -#if defined(CONFIG_HAVE_HARDLOCKUP_DETECTOR_ARCH) || 
> > defined(CONFIG_HARDLOCKUP_DETECTOR_SPARC64)
> > +#if defined(CONFIG_HARDLOCKUP_DETECTOR_ARCH) || 
> > defined(CONFIG_HARDLOCKUP_DETECTOR_SPARC64)
>
> This results in:
>
> arch/powerpc/platforms/pseries/mobility.c: In function 
> 'pseries_migrate_partition':
> arch/powerpc/platforms/pseries/mobility.c:753:17: error: implicit declaration 
> of function 'watchdog_hardlockup_set_timeout_pct'; did you mean 
> 'watchdog_hardlockup_stop'? [-Werror=implicit-function-declaration]
>   753 | watchdog_hardlockup_set_timeout_pct(factor);
>
> with ppc64_defconfig -CONFIG_HARDLOCKUP_DETECTOR, because the dummy
> for watchdog_hardlockup_set_timeout_pct() is still defined in
> arch/powerpc/include/asm/nmi.h which is no longer included.

Can you test with:

https://lore.kernel.org/r/20230629124500.1.I55e2f4e7903d686c4484cb23c033c6a9e1a9d4c4@changeid

Thanks!

Re: [PATCH v2 6/6] watchdog/hardlockup: Define HARDLOCKUP_DETECTOR_ARCH

[Doug Anderson]

Hi,

On Wed, Jun 21, 2023 at 6:08 AM Michael Ellerman  wrote:
>
> Petr Mladek  writes:
> > The HAVE_ prefix means that the code could be enabled. Add another
> > variable for HAVE_HARDLOCKUP_DETECTOR_ARCH without this prefix.
> > It will be set when it should be built. It will make it compatible
> > with the other hardlockup detectors.
> >
> > The change allows to clean up dependencies of PPC_WATCHDOG
> > and HAVE_HARDLOCKUP_DETECTOR_PERF definitions for powerpc.
> >
> > As a result HAVE_HARDLOCKUP_DETECTOR_PERF has the same dependencies
> > on arm, x86, powerpc architectures.
> >
> > Signed-off-by: Petr Mladek 
> > Reviewed-by: Douglas Anderson 
> > ---
> >  arch/powerpc/Kconfig | 5 ++---
> >  include/linux/nmi.h  | 2 +-
> >  lib/Kconfig.debug| 9 +
> >  3 files changed, 12 insertions(+), 4 deletions(-)
>
> Something in this patch is breaking the powerpc g5_defconfig, I don't
> immediately see what though.
>
> ../arch/powerpc/kernel/stacktrace.c: In function ‘handle_backtrace_ipi’:
> ../arch/powerpc/kernel/stacktrace.c:171:9: error: implicit declaration of 
> function ‘nmi_cpu_backtrace’ [-Werror=implicit-function-declaration]
>   171 | nmi_cpu_backtrace(regs);
>   | ^
> ../arch/powerpc/kernel/stacktrace.c: In function 
> ‘arch_trigger_cpumask_backtrace’:
> ../arch/powerpc/kernel/stacktrace.c:226:9: error: implicit declaration of 
> function ‘nmi_trigger_cpumask_backtrace’; did you mean 
> ‘arch_trigger_cpumask_backtrace’? [-Werror=implicit-function-declaration]
>   226 | nmi_trigger_cpumask_backtrace(mask, exclude_self, 
> raise_backtrace_ipi);
>   | ^
>   | arch_trigger_cpumask_backtrace
> cc1: all warnings being treated as errors

Yeah, I can reproduce that.

The problem is that before ${SUBJECT} patch "include/linux/nmi.h"
would include . Now it won't.

There are a ton of different ways to fix this, but I think the one
that makes sense is to be consistent with other architectures and move
the "arch_trigger_cpumask_backtrace" definitions to asm/irq.h.

https://lore.kernel.org/r/20230621164809.1.Ice67126857506712559078e7de26d32d26e64631@changeid

-Doug

Re: [PATCH v2 4/6] watchdog/hardlockup: Make HAVE_NMI_WATCHDOG sparc64-specific

[Doug Anderson]

Hi,

On Fri, Jun 16, 2023 at 8:07 AM Petr Mladek  wrote:
>
> There are several hardlockup detector implementations and several Kconfig
> values which allow selection and build of the preferred one.
>
> CONFIG_HARDLOCKUP_DETECTOR was introduced by the commit 23637d477c1f53acb
> ("lockup_detector: Introduce CONFIG_HARDLOCKUP_DETECTOR") in v2.6.36.
> It was a preparation step for introducing the new generic perf hardlockup
> detector.
>
> The existing arch-specific variants did not support the to-be-created
> generic build configurations, sysctl interface, etc. This distinction
> was made explicit by the commit 4a7863cc2eb5f98 ("x86, nmi_watchdog:
> Remove ARCH_HAS_NMI_WATCHDOG and rely on CONFIG_HARDLOCKUP_DETECTOR")
> in v2.6.38.
>
> CONFIG_HAVE_NMI_WATCHDOG was introduced by the commit d314d74c695f967e105
> ("nmi watchdog: do not use cpp symbol in Kconfig") in v3.4-rc1. It replaced
> the above mentioned ARCH_HAS_NMI_WATCHDOG. At that time, it was still used
> by three architectures, namely blackfin, mn10300, and sparc.
>
> The support for blackfin and mn10300 architectures has been completely
> dropped some time ago. And sparc is the only architecture with the historic
> NMI watchdog at the moment.
>
> And the old sparc implementation is really special. It is always built on
> sparc64. It used to be always enabled until the commit 7a5c8b57cec93196b
> ("sparc: implement watchdog_nmi_enable and watchdog_nmi_disable") added
> in v4.10-rc1.
>
> There are only few locations where the sparc64 NMI watchdog interacts
> with the generic hardlockup detectors code:
>
>   + implements arch_touch_nmi_watchdog() which is called from the generic
> touch_nmi_watchdog()
>
>   + implements watchdog_hardlockup_enable()/disable() to support
> /proc/sys/kernel/nmi_watchdog
>
>   + is always preferred over other generic watchdogs, see
> CONFIG_HARDLOCKUP_DETECTOR
>
>   + includes asm/nmi.h into linux/nmi.h because some sparc-specific
> functions are needed in sparc-specific code which includes
> only linux/nmi.h.
>
> The situation became more complicated after the commit 05a4a95279311c3
> ("kernel/watchdog: split up config options") and commit 2104180a53698df5
> ("powerpc/64s: implement arch-specific hardlockup watchdog") in v4.13-rc1.
> They introduced HAVE_HARDLOCKUP_DETECTOR_ARCH. It was used for powerpc
> specific hardlockup detector. It was compatible with the perf one
> regarding the general boot, sysctl, and programming interfaces.
>
> HAVE_HARDLOCKUP_DETECTOR_ARCH was defined as a superset of
> HAVE_NMI_WATCHDOG. It made some sense because all arch-specific
> detectors had some common requirements, namely:
>
>   + implemented arch_touch_nmi_watchdog()
>   + included asm/nmi.h into linux/nmi.h
>   + defined the default value for /proc/sys/kernel/nmi_watchdog
>
> But it actually has made things pretty complicated when the generic
> buddy hardlockup detector was added. Before the generic perf detector
> was newer supported together with an arch-specific one. But the buddy
> detector could work on any SMP system. It means that an architecture
> could support both the arch-specific and buddy detector.
>
> As a result, there are few tricky dependencies. For example,
> CONFIG_HARDLOCKUP_DETECTOR depends on:
>
>   ((HAVE_HARDLOCKUP_DETECTOR_PERF || HAVE_HARDLOCKUP_DETECTOR_BUDDY) && 
> !HAVE_NMI_WATCHDOG) || HAVE_HARDLOCKUP_DETECTOR_ARCH
>
> The problem is that the very special sparc implementation is defined as:
>
>   HAVE_NMI_WATCHDOG && !HAVE_HARDLOCKUP_DETECTOR_ARCH
>
> Another problem is that the meaning of HAVE_NMI_WATCHDOG is far from clear
> without reading understanding the history.
>
> Make the logic less tricky and more self-explanatory by making
> HAVE_NMI_WATCHDOG specific for the sparc64 implementation. And rename it to
> HAVE_HARDLOCKUP_DETECTOR_SPARC64.
>
> Note that HARDLOCKUP_DETECTOR_PREFER_BUDDY, HARDLOCKUP_DETECTOR_PERF,
> and HARDLOCKUP_DETECTOR_BUDDY may conflict only with
> HAVE_HARDLOCKUP_DETECTOR_ARCH. They depend on HARDLOCKUP_DETECTOR
> and it is not longer enabled when HAVE_NMI_WATCHDOG is set.
>
> Signed-off-by: Petr Mladek 
>
> watchdog/sparc64: Rename HAVE_NMI_WATCHDOG to HAVE_HARDLOCKUP_WATCHDOG_SPARC64
>
> The configuration variable HAVE_NMI_WATCHDOG has a generic name but
> it is selected only for SPARC64.
>
> It should _not_ be used in general because it is not integrated with
> the other hardlockup detectors. Namely, it does not support the hardlockup
> specific command line parameters and systcl interface. Instead, it is
> enabled/disabled together with the softlockup detector by the global
> "watchdog" sysctl.
>
> Rename it to HAVE_HARDLOCKUP_WATCHDOG_SPARC64 to make the special
> behavior more clear.
>
> Also the variable is set only on sparc64. Move the definition
> from arch/Kconfig to arch/sparc/Kconfig.debug.
>
> Signed-off-by: Petr Mladek 

I think you goofed up when squashing the patches. You've now got a
second patch subject after your first Signed-off-by and 

Re: [PATCH v2 2/6] watchdog/hardlockup: Make the config checks more straightforward

[Doug Anderson]

Hi,

On Fri, Jun 16, 2023 at 8:07 AM Petr Mladek  wrote:
>
> There are four possible variants of hardlockup detectors:
>
>   + buddy: available when SMP is set.
>
>   + perf: available when HAVE_HARDLOCKUP_DETECTOR_PERF is set.
>
>   + arch-specific: available when HAVE_HARDLOCKUP_DETECTOR_ARCH is set.
>
>   + sparc64 special variant: available when HAVE_NMI_WATCHDOG is set
> and HAVE_HARDLOCKUP_DETECTOR_ARCH is not set.
>
> The check for the sparc64 variant is more complicated because
> HAVE_NMI_WATCHDOG is used to #ifdef code used by both arch-specific
> and sparc64 specific variant. Therefore it is automatically
> selected with HAVE_HARDLOCKUP_DETECTOR_ARCH.
>
> This complexity is partly hidden in HAVE_HARDLOCKUP_DETECTOR_NON_ARCH.
> It reduces the size of some checks but it makes them harder to follow.
>
> Finally, the other temporary variable HARDLOCKUP_DETECTOR_NON_ARCH
> is used to re-compute HARDLOCKUP_DETECTOR_PERF/BUDDY when the global
> HARDLOCKUP_DETECTOR switch is enabled/disabled.
>
> Make the logic more straightforward by the following changes:
>
>   + Better explain the role of HAVE_HARDLOCKUP_DETECTOR_ARCH and
> HAVE_NMI_WATCHDOG in comments.
>
>   + Add HAVE_HARDLOCKUP_DETECTOR_BUDDY so that there is separate
> HAVE_* for all four hardlockup detector variants.
>
> Use it in the other conditions instead of SMP. It makes it
> clear that it is about the buddy detector.
>
>   + Open code HAVE_HARDLOCKUP_DETECTOR_NON_ARCH in HARDLOCKUP_DETECTOR
> and HARDLOCKUP_DETECTOR_PREFER_BUDDY. It helps to understand
> the conditions between the four hardlockup detector variants.
>
>   + Define the exact conditions when HARDLOCKUP_DETECTOR_PERF/BUDDY
> can be enabled. It explains the dependency on the other
> hardlockup detector variants.
>
> Also it allows to remove HARDLOCKUP_DETECTOR_NON_ARCH by using "imply".
> It triggers re-evaluating HARDLOCKUP_DETECTOR_PERF/BUDDY when
> the global HARDLOCKUP_DETECTOR switch is changed.
>
>   + Add dependency on HARDLOCKUP_DETECTOR so that the affected variables
> disappear when the hardlockup detectors are disabled.
>
> Another nice side effect is that HARDLOCKUP_DETECTOR_PREFER_BUDDY
> value is not preserved when the global switch is disabled.
> The user has to make the decision again when it gets re-enabled.
>
> Signed-off-by: Petr Mladek 
> ---
>  arch/Kconfig  | 23 +-
>  lib/Kconfig.debug | 62 +++
>  2 files changed, 53 insertions(+), 32 deletions(-)

While I'd still paint the bikeshed a different color and organize the
dependencies a little differently, as discussed in your v1 post, this
is still OK w/ me.

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 1/6] watchdog/hardlockup: Sort hardlockup detector related config values a logical way

[Doug Anderson]

Hi,

On Fri, Jun 16, 2023 at 8:06 AM Petr Mladek  wrote:
>
> There are four possible variants of hardlockup detectors:
>
>   + buddy: available when SMP is set.
>
>   + perf: available when HAVE_HARDLOCKUP_DETECTOR_PERF is set.
>
>   + arch-specific: available when HAVE_HARDLOCKUP_DETECTOR_ARCH is set.
>
>   + sparc64 special variant: available when HAVE_NMI_WATCHDOG is set
> and HAVE_HARDLOCKUP_DETECTOR_ARCH is not set.
>
> Only one hardlockup detector can be compiled in. The selection is done
> using quite complex dependencies between several CONFIG variables.
> The following patches will try to make it more straightforward.
>
> As a first step, reorder the definitions of the various CONFIG variables.
> The logical order is:
>
>1. HAVE_* variables define available variants. They are typically
>   defined in the arch/ config files.
>
>2. HARDLOCKUP_DETECTOR y/n variable defines whether the hardlockup
>   detector is enabled at all.
>
>3. HARDLOCKUP_DETECTOR_PREFER_BUDDY y/n variable defines whether
>   the buddy detector should be preferred over the perf one.
>   Note that the arch specific variants are always preferred when
>   available.
>
>4. HARDLOCKUP_DETECTOR_PERF/BUDDY variables define whether the given
>   detector is enabled in the end.
>
>5. HAVE_HARDLOCKUP_DETECTOR_NON_ARCH and HARDLOCKUP_DETECTOR_NON_ARCH
>   are temporary variables that are going to be removed in
>   a followup patch.
>
> This is a preparation step for further cleanup. It will change the logic
> without shuffling the definitions.
>
> This change temporary breaks the C-like ordering where the variables are
> declared or defined before they are used. It is not really needed for
> Kconfig. Also the following patches will rework the logic so that
> the ordering will be C-like in the end.
>
> The patch just shuffles the definitions. It should not change the existing
> behavior.
>
> Signed-off-by: Petr Mladek 
> ---
>  lib/Kconfig.debug | 112 +++---
>  1 file changed, 56 insertions(+), 56 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 2/7] watchdog/hardlockup: Make the config checks more straightforward

[Doug Anderson]

Hi,

On Wed, Jun 14, 2023 at 3:29 AM Petr Mladek  wrote:
>
> It seems that we have entered into a bike shedding mode.
> The following questions come to my mind:
>
>1. Does this patchset improve the current state?
>
>2. Maybe, it is not black Is it possible to summarize
>   what exactly got better and what got worse?
>
> Maybe, there is no need to do bike-shedding about every step
> if the final result is reasonable and the steps are not
> completely wrong.
>
> I just followed my intuition and tried to do some changes step
> by step. I got lost many times so maybe the steps are not
> ideal. Anyway, the steps helped me to understand the logic
> and stay reasonably confident that they did not change
> the behavior.
>
> I could rework the patchset. But I first need to know what
> exactly is bad in the result. And eventually if there is more
> logical way how to end there.

Sure. I still feel like the end result of the CONFIG options after
your whole patchset is easier to understand / cleaner by adjusting the
dependencies as I have suggested. That being said, I agree that it is
the type of thing that can be more a matter of personal preference. I
do agree that, even if you don't take my suggestion of adjusting the
dependencies, the end result of your patchset still makes things
better than they were.

...so if you really feel strongly that things are more understandable
with the dependencies specified as you have, I won't stand in the way.
I still think you need a v2, though, just to address other nits.

-Doug

Re: [PATCH v5 15/18] watchdog/perf: Add a weak function for an arch to detect if perf can use NMIs

[Doug Anderson]

Mark,

On Mon, Jun 12, 2023 at 3:33 AM Mark Rutland  wrote:
>
> On Fri, May 19, 2023 at 10:18:39AM -0700, Douglas Anderson wrote:
> > On arm64, NMI support needs to be detected at runtime. Add a weak
> > function to the perf hardlockup detector so that an architecture can
> > implement it to detect whether NMIs are available.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> > While I won't object to this patch landing, I consider it part of the
> > arm64 perf hardlockup effort. I would be OK with the earlier patches
> > in the series landing and then not landing ${SUBJECT} patch nor
> > anything else later.
>
> FWIW, everything prior to this looks fine to me, so I reckon it'd be worth
> splitting the series here and getting the buddy lockup detector in first, to
> avoid a log-jam on all the subsequent NMI bits.

I think the whole series has already landed in Andrew's tree,
including the arm64 "perf" lockup detector bits. I saw all the
notifications from Andrew go through over the weekend that they were
moved from an "unstable" branch to a "stable" one and I see them at:

https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/log/?h=mm-nonmm-stable

When I first saw Anderw land the arm64 perf lockup detector bits in
his unstable branch several weeks ago, I sent a private message to the
arm64 maintainers (yourself included) to make sure you were aware of
it and that it hadn't been caught in mail filters. I got the
impression that everything was OK. Is that not the case?


-Doug

Re: [PATCH 2/7] watchdog/hardlockup: Make the config checks more straightforward

[Doug Anderson]

Hi,

On Thu, Jun 8, 2023 at 4:02 AM Petr Mladek  wrote:
>
> > >  config HARDLOCKUP_DETECTOR
> > > bool "Detect Hard Lockups"
> > > depends on DEBUG_KERNEL && !S390
> > > -   depends on HAVE_HARDLOCKUP_DETECTOR_NON_ARCH || 
> > > HAVE_HARDLOCKUP_DETECTOR_ARCH
> > > +   depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || 
> > > HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || 
> > > HAVE_HARDLOCKUP_DETECTOR_ARCH
> >
> > Adding the dependency to buddy (see ablove) would simplify the above
> > to just this:
> >
> > depends on HAVE_HARDLOCKUP_DETECTOR_PERF ||
> > HAVE_HARDLOCKUP_DETECTOR_BUDDY || HAVE_HARDLOCKUP_DETECTOR_ARCH
>
> This is exactly what I do not want. It would just move the check
> somewhere else. But it would make the logic harder to understand.

Hmmm. To me, it felt easier to understand by moving this into the
"HAVE_HARDLOCKUP_DETECTOR_BUDDY". To me it was pretty easy to say "if
an architecture defined its own arch-specific watchdog then buddy
can't be enabled" and that felt like it fit cleanly within the
"HAVE_HARDLOCKUP_DETECTOR_BUDDY" definition. It got rid of _a lot_ of
other special cases / checks elsewhere and felt quite a bit cleaner to
me. I only had to think about the conflict between the "buddy" and
"nmi" watchdogs once when I understood
"HAVE_HARDLOCKUP_DETECTOR_BUDDY".


> > As per above, it's simply a responsibility of architectures not to
> > define that they have both "perf" if they have the NMI watchdog, so
> > it's just buddy to worry about.
>
> Where is this documented, please?
> Is it safe to assume this?

It's not well documented and I agree that it could be improved. Right
now, HAVE_NMI_WATCHDOG is documented to say that the architecture
"defines its own arch_touch_nmi_watchdog()". Looking before my
patches, you can see that "kernel/watchdog_hld.c" (the "perf" detector
code) unconditionally defines arch_touch_nmi_watchdog(). That would
give you a linker error.


> I would personally prefer to ensure this by the config check.
> It is even better than documentation because nobody reads
> documentation ;-)

Sure. IMO this should be documented as close as possible to the root
of the problem. Make "HAVE_NMI_WATCHDOG" depend on
"!HAVE_HARDLOCKUP_DETECTOR_PERF". That expresses that an architecture
is not allowed to declare that it has both.

Re: [PATCH 4/7] watchdog/hardlockup: Enable HAVE_NMI_WATCHDOG only on sparc64

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek  wrote:
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 13c6e596cf9e..57f15babe188 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -404,10 +404,9 @@ config HAVE_NMI_WATCHDOG
> depends on HAVE_NMI
> bool
> help
> - The arch provides its own hardlockup detector implementation instead
> + Sparc64 provides its own hardlockup detector implementation instead
>   of the generic perf one.

It's a little weird to document generic things with the specifics of
the user. The exception, IMO, is when something is deprecated.
Personally, it would sound less weird to me to say something like:

The arch provides its own hardlockup detector implementation instead
of the generic perf one. This is a deprecated thing to do and kept
around until sparc64 provides a full hardlockup implementation or
moves to generic code.


> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d201f5d3876b..4b4aa0f941f9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1050,15 +1050,13 @@ config HAVE_HARDLOCKUP_DETECTOR_BUDDY
>  #  sparc64: has a custom implementation which is not using the common
>  #  hardlockup command line options and sysctl interface.
>  #
> -# Note that HAVE_NMI_WATCHDOG is used to distinguish the sparc64 specific
> -# implementaion. It is automatically enabled also for other arch-specific
> -# variants which set HAVE_HARDLOCKUP_DETECTOR_ARCH. It makes the check
> -# of avaialable and supported variants quite tricky.
> +# Note that HAVE_NMI_WATCHDOG is set when the sparc64 specific implementation
> +# is used.
>  #
>  config HARDLOCKUP_DETECTOR
> bool "Detect Hard Lockups"
> -   depends on DEBUG_KERNEL && !S390
> -   depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || 
> HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || 
> HAVE_HARDLOCKUP_DETECTOR_ARCH
> +   depends on DEBUG_KERNEL && !S390 && !HAVE_NMI_WATCHDOG
> +   depends on HAVE_HARDLOCKUP_DETECTOR_PERF || 
> HAVE_HARDLOCKUP_DETECTOR_BUDDY || HAVE_HARDLOCKUP_DETECTOR_ARCH

If you add the "!HAVE_NMI_WATCHDOG" as a dependency to
HAVE_HARDLOCKUP_DETECTOR_BUDDY, as discussed in a previous patch, you
can skip adding it here.


> imply HARDLOCKUP_DETECTOR_PERF
> imply HARDLOCKUP_DETECTOR_BUDDY
> select LOCKUP_DETECTOR
> @@ -1079,7 +1077,7 @@ config HARDLOCKUP_DETECTOR_PREFER_BUDDY
> bool "Prefer the buddy CPU hardlockup detector"
> depends on HARDLOCKUP_DETECTOR
> depends on HAVE_HARDLOCKUP_DETECTOR_PERF && 
> HAVE_HARDLOCKUP_DETECTOR_BUDDY
> -   depends on !HAVE_NMI_WATCHDOG
> +   depends on !HAVE_HARLOCKUP_DETECTOR_ARCH

Don't need this. Architectures never are allowed to define
HAVE_HARDLOCKUP_DETECTOR_PERF and HAVE_HARLOCKUP_DETECTOR_ARCH


> default n
> help
>   Say Y here to prefer the buddy hardlockup detector over the perf 
> one.
> @@ -1096,7 +1094,7 @@ config HARDLOCKUP_DETECTOR_PERF
> bool
> depends on HARDLOCKUP_DETECTOR
> depends on HAVE_HARDLOCKUP_DETECTOR_PERF && 
> !HARDLOCKUP_DETECTOR_PREFER_BUDDY
> -   depends on !HAVE_NMI_WATCHDOG
> +   depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH

Similarly, don't need this.


> select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER
>
>  config HARDLOCKUP_DETECTOR_BUDDY
> @@ -1104,7 +1102,7 @@ config HARDLOCKUP_DETECTOR_BUDDY
> depends on HARDLOCKUP_DETECTOR
> depends on HAVE_HARDLOCKUP_DETECTOR_BUDDY
> depends on !HAVE_HARDLOCKUP_DETECTOR_PERF || 
> HARDLOCKUP_DETECTOR_PREFER_BUDDY
> -   depends on !HAVE_NMI_WATCHDOG
> +   depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH

Similarly, don't need this.


In general I don't object to splitting out HAVE_NMI_WATCHDOG from
HAVE_HARDLOCKUP_DETECTOR_ARCH.

Re: [PATCH 6/7] watchdog/sparc64: Define HARDLOCKUP_DETECTOR_SPARC64

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:26 AM Petr Mladek  wrote:
>
> The HAVE_ prefix means that the code could be enabled. Add another
> variable for HAVE_HARDLOCKUP_DETECTOR_SPARC64 without this prefix.
> It will be set when it should be built. It will make it compatible
> with the other hardlockup detectors.
>
> Before, it is far from obvious that the SPARC64 variant is actually used:
>
> $> make ARCH=sparc64 defconfig
> $> grep HARDLOCKUP_DETECTOR .config
> CONFIG_HAVE_HARDLOCKUP_DETECTOR_BUDDY=y
> CONFIG_HAVE_HARDLOCKUP_DETECTOR_SPARC64=y
>
> After, it is more clear:
>
> $> make ARCH=sparc64 defconfig
> $> grep HARDLOCKUP_DETECTOR .config
> CONFIG_HAVE_HARDLOCKUP_DETECTOR_BUDDY=y
> CONFIG_HAVE_HARDLOCKUP_DETECTOR_SPARC64=y
> CONFIG_HARDLOCKUP_DETECTOR_SPARC64=y
>
> Signed-off-by: Petr Mladek 
> ---
>  arch/sparc/Kconfig.debug | 10 +-
>  include/linux/nmi.h  |  4 ++--
>  kernel/watchdog.c|  2 +-
>  lib/Kconfig.debug|  2 +-
>  4 files changed, 13 insertions(+), 5 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 3/7] watchdog/hardlockup: Declare arch_touch_nmi_watchdog() only in linux/nmi.h

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek  wrote:
>
> arch_touch_nmi_watchdog() needs a different implementation for various
> hardlockup detector implementations. And it does nothing when
> any hardlockup detector is not build at all.

s/build/built/


> arch_touch_nmi_watchdog() has to be declared in linux/nmi.h. It is done
> directly in this header file for the perf and buddy detectors. And it
> is done in the included asm/linux.h for arch specific detectors.
>
> The reason probably is that the arch specific variants build the code
> using another conditions. For example, powerpc64/sparc64 builds the code
> when CONFIG_PPC_WATCHDOG is enabled.
>
> Another reason might be that these architectures define more functions
> in asm/nmi.h anyway.
>
> However the generic code actually knows the information. The config
> variables HAVE_NMI_WATCHDOG and HAVE_HARDLOCKUP_DETECTOR_ARCH are used
> to decide whether to build the buddy detector.
>
> In particular, CONFIG_HARDLOCKUP_DETECTOR is set only when a generic
> or arch-specific hardlockup detector is built. The only exception
> is sparc64 which ignores the global HARDLOCKUP_DETECTOR switch.
>
> The information about sparc64 is a bit complicated. The hardlockup
> detector is built there when CONFIG_HAVE_NMI_WATCHDOG is set and
> CONFIG_HAVE_HARDLOCKUP_DETECTOR_ARCH is not set.
>
> People might wonder whether this change really makes things easier.
> The motivation is:
>
>   + The current logic in linux/nmi.h is far from obvious.
> For example, arch_touch_nmi_watchdog() is defined as {} when
> neither CONFIG_HARDLOCKUP_DETECTOR_COUNTS_HRTIMER nor
> CONFIG_HAVE_NMI_WATCHDOG is defined.
>
>   + The change synchronizes the checks in lib/Kconfig.debug and
> in the generic code.
>
>   + It is a step that will help cleaning HAVE_NMI_WATCHDOG related
> checks.
>
> The change should not change the existing behavior.
>
> Signed-off-by: Petr Mladek 
> ---
>  arch/powerpc/include/asm/nmi.h |  2 --
>  arch/sparc/include/asm/nmi.h   |  1 -
>  include/linux/nmi.h| 13 ++---
>  3 files changed, 10 insertions(+), 6 deletions(-)

This looks right and is a nice cleanup.

Reviewed-by: Douglas Anderson 

Re: [PATCH 0/7] watchdog/hardlockup: Cleanup configuration of hardlockup detectors

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek  wrote:
>
> Hi,
>
> this patchset is supposed to replace the last patch in the patchset cleaning
> up after introducing the buddy detector, see
> https://lore.kernel.org/r/20230526184139.10.I821fe7609e57608913fe05abd8f35b343e7a9aae@changeid

I will let Andrew chime in with his preference, but so far I haven't
seen him dropping and/or modifying any patches that he's picked up in
this series. I see that he's already picked up the patch that you're
"replacing". I wonder if it would be easier for him if you just built
atop that?

-Doug

Re: [PATCH 7/7] watchdog/hardlockup: Define HARDLOCKUP_DETECTOR_ARCH

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:26 AM Petr Mladek  wrote:
>
> @@ -1102,6 +1103,14 @@ config HARDLOCKUP_DETECTOR_BUDDY
> depends on !HAVE_HARDLOCKUP_DETECTOR_ARCH
> select HARDLOCKUP_DETECTOR_COUNTS_HRTIMER
>
> +config HARDLOCKUP_DETECTOR_ARCH
> +   bool
> +   depends on HARDLOCKUP_DETECTOR
> +   depends on HAVE_HARDLOCKUP_DETECTOR_ARCH
> +   help
> + The arch-specific implementation of the hardlockup detector is
> + available.

nit: "is available" makes it sound a bit too much like a "have"
version. Maybe "The arch-specific implementation of the hardlockup
detector will be used" or something like that?

Otherise:

Reviewed-by: Douglas Anderson 

Re: [PATCH 5/7] watchdog/sparc64: Rename HAVE_NMI_WATCHDOG to HAVE_HARDLOCKUP_WATCHDOG_SPARC64

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek  wrote:
>
> The configuration variable HAVE_NMI_WATCHDOG has a generic name but
> it is selected only for SPARC64.
>
> It should _not_ be used in general because it is not integrated with
> the other hardlockup detectors. Namely, it does not support the hardlockup
> specific command line parameters and systcl interface. Instead, it is
> enabled/disabled together with the softlockup detector by the global
> "watchdog" sysctl.
>
> Rename it to HAVE_HARDLOCKUP_WATCHDOG_SPARC64 to make the special
> behavior more clear.
>
> Also the variable is set only on sparc64. Move the definition
> from arch/Kconfig to arch/sparc/Kconfig.debug.
>
> Signed-off-by: Petr Mladek 
> ---
>  arch/Kconfig | 12 
>  arch/sparc/Kconfig   |  2 +-
>  arch/sparc/Kconfig.debug | 12 
>  include/linux/nmi.h  |  4 ++--
>  kernel/watchdog.c|  2 +-
>  lib/Kconfig.debug|  5 +
>  6 files changed, 17 insertions(+), 20 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 2/7] watchdog/hardlockup: Make the config checks more straightforward

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek  wrote:
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 422f0ffa269e..13c6e596cf9e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -404,17 +404,27 @@ config HAVE_NMI_WATCHDOG
> depends on HAVE_NMI
> bool
> help
> - The arch provides a low level NMI watchdog. It provides
> - asm/nmi.h, and defines its own watchdog_hardlockup_probe() and
> - arch_touch_nmi_watchdog().
> + The arch provides its own hardlockup detector implementation instead
> + of the generic perf one.

nit: did you mean to have different wording here compared to
HAVE_HARDLOCKUP_DETECTOR_ARCH? Here you say "the generic perf one" and
there you say "the generic ones", though it seems like you mean them
to be the same.

> +
> + Sparc64 defines this variable without HAVE_HARDLOCKUP_DETECTOR_ARCH.
> + It does _not_ use the command line parameters and sysctl interface
> + used by generic hardlockup detectors. Instead it is enabled/disabled
> + by the top-level watchdog interface that is common for both 
> softlockup
> + and hardlockup detectors.
>
>  config HAVE_HARDLOCKUP_DETECTOR_ARCH
> bool
> select HAVE_NMI_WATCHDOG
> help
> - The arch chooses to provide its own hardlockup detector, which is
> - a superset of the HAVE_NMI_WATCHDOG. It also conforms to config
> - interfaces and parameters provided by hardlockup detector subsystem.
> + The arch provides its own hardlockup detector implementation instead
> + of the generic ones.
> +
> + It uses the same command line parameters, and sysctl interface,
> + as the generic hardlockup detectors.
> +
> + HAVE_NMI_WATCHDOG is selected to build the code shared with
> + the sparc64 specific implementation.
>
>  config HAVE_PERF_REGS
> bool
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 3e91fa33c7a0..d201f5d3876b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1035,16 +1035,33 @@ config BOOTPARAM_SOFTLOCKUP_PANIC
>
>   Say N if unsure.
>
> +config HAVE_HARDLOCKUP_DETECTOR_BUDDY
> +   bool
> +   depends on SMP
> +   default y

I think you simplify your life if you also add:

  depends on !HAVE_NMI_WATCHDOG

The existing config system has always assumed that no architecture
defines both HAVE_HARDLOCKUP_DETECTOR_PERF and HAVE_NMI_WATCHDOG
(symbols would have clashed and you would get a link error as two
watchdogs try to implement the same weak symbol). If you add the extra
dependency to "buddy" as per above, then a few things below fall out.
I'll try to point them out below.


> +
>  #
> -# arch/ can define HAVE_HARDLOCKUP_DETECTOR_ARCH to provide their own hard
> -# lockup detector rather than the perf based detector.
> +# Global switch whether to build a hardlockup detector at all. It is 
> available
> +# only when the architecture supports at least one implementation. There are
> +# two exceptions. The hardlockup detector is newer enabled on:

s/newer/never/


> +#
> +#  s390: it reported many false positives there
> +#
> +#  sparc64: has a custom implementation which is not using the common
> +#  hardlockup command line options and sysctl interface.
> +#
> +# Note that HAVE_NMI_WATCHDOG is used to distinguish the sparc64 specific
> +# implementaion. It is automatically enabled also for other arch-specific
> +# variants which set HAVE_HARDLOCKUP_DETECTOR_ARCH. It makes the check
> +# of avaialable and supported variants quite tricky.
>  #
>  config HARDLOCKUP_DETECTOR
> bool "Detect Hard Lockups"
> depends on DEBUG_KERNEL && !S390
> -   depends on HAVE_HARDLOCKUP_DETECTOR_NON_ARCH || 
> HAVE_HARDLOCKUP_DETECTOR_ARCH
> +   depends on ((HAVE_HARDLOCKUP_DETECTOR_PERF || 
> HAVE_HARDLOCKUP_DETECTOR_BUDDY) && !HAVE_NMI_WATCHDOG) || 
> HAVE_HARDLOCKUP_DETECTOR_ARCH

Adding the dependency to buddy (see ablove) would simplify the above
to just this:

depends on HAVE_HARDLOCKUP_DETECTOR_PERF ||
HAVE_HARDLOCKUP_DETECTOR_BUDDY || HAVE_HARDLOCKUP_DETECTOR_ARCH

As per above, it's simply a responsibility of architectures not to
define that they have both "perf" if they have the NMI watchdog, so
it's just buddy to worry about.


> +   imply HARDLOCKUP_DETECTOR_PERF
> +   imply HARDLOCKUP_DETECTOR_BUDDY
> select LOCKUP_DETECTOR
> -   select HARDLOCKUP_DETECTOR_NON_ARCH if 
> HAVE_HARDLOCKUP_DETECTOR_NON_ARCH
>
> help
>   Say Y here to enable the kernel to act as a watchdog to detect
> @@ -1055,9 +1072,15 @@ config HARDLOCKUP_DETECTOR
>   chance to run.  The current stack trace is displayed upon detection
>   and the system will stay locked up.
>
> +#
> +# Note that arch-specific variants are always preferred.
> +#
>  config HARDLOCKUP_DETECTOR_PREFER_BUDDY
> bool "Prefer the buddy CPU hardlockup detector"
> 

Re: [PATCH 1/7] watchdog/hardlockup: Sort hardlockup detector related config values a logical way

[Doug Anderson]

Hi,

On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek  wrote:
>
> There are four possible variants of hardlockup detectors:
>
>   + buddy: available when SMP is set.
>
>   + perf: available when HAVE_HARDLOCKUP_DETECTOR_PERF is set.
>
>   + arch-specific: available when HAVE_HARDLOCKUP_DETECTOR_ARCH is set.
>
>   + sparc64 special variant: available when HAVE_NMI_WATCHDOG is set
> and HAVE_HARDLOCKUP_DETECTOR_ARCH is not set.
>
> Only one hardlockup detector can be compiled in. The selection is done
> using quite complex dependencies between several CONFIG variables.
> The following patches will try to make it more straightforward.
>
> As a first step, reorder the definitions of the various CONFIG variables.
> The logical order is:
>
>1. HAVE_* variables define available variants. They are typically
>   defined in the arch/ config files.
>
>2. HARDLOCKUP_DETECTOR y/n variable defines whether the hardlockup
>   detector is enabled at all.
>
>3. HARDLOCKUP_DETECTOR_PREFER_BUDDY y/n variable defines whether
>   the buddy detector should be preferred over the perf one.
>   Note that the arch specific variants are always preferred when
>   available.
>
>4. HARDLOCKUP_DETECTOR_PERF/BUDDY variables define whether the given
>   detector is enabled in the end.
>
>5. HAVE_HARDLOCKUP_DETECTOR_NON_ARCH and HARDLOCKUP_DETECTOR_NON_ARCH
>   are temporary variables that are going to be removed in
>   a followup patch.
>
> The patch just shuffles the definitions. It should not change the existing
> behavior.
>
> Signed-off-by: Petr Mladek 
> ---
>  lib/Kconfig.debug | 112 +++---
>  1 file changed, 56 insertions(+), 56 deletions(-)

I don't really have any strong opinions, so I'm fine with this. In
general I think the ordering I picked tried to match the existing
"style" which generally tried to list configs and then select them
below. To me the existing style makes more sense when thinking about
writing C code without having to put a pile of prototypes at the top
of your file: you define things higher in the file and reference them
below. For instance, the old style (before any of my patches) had:

config SOFTLOCKUP_DETECTOR:
  ... blah blah blah ...

config HARDLOCKUP_DETECTOR_PERF:
  select SOFTLOCKUP_DETECTOR

config HARDLOCKUP_DETECTOR:
  ... blah blah blah ...
  select LOCKUP_DETECTOR
  select HARDLOCKUP_DETECTOR_PERF if HAVE_HARDLOCKUP_DETECTOR_PERF

Your new style seems to be the opposite of that.

-Doug

Re: [PATCH v8 00/10] arm64: Add framework to turn an IPI as NMI

[Doug Anderson]

Hi,

On Wed, May 10, 2023 at 9:42 AM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, May 10, 2023 at 9:30 AM Mark Rutland  wrote:
> >
> > On Wed, May 10, 2023 at 08:28:17AM -0700, Doug Anderson wrote:
> > > Hi,
> >
> > Hi Doug,
> >
> > > On Wed, Apr 19, 2023 at 3:57 PM Douglas Anderson  
> > > wrote:
> > > > This is an attempt to resurrect Sumit's old patch series [1] that
> > > > allowed us to use the arm64 pseudo-NMI to get backtraces of CPUs and
> > > > also to round up CPUs in kdb/kgdb. The last post from Sumit that I
> > > > could find was v7, so I called this series v8. I haven't copied all of
> > > > his old changelongs here, but you can find them from the link.
> > > >
> > > > Since v7, I have:
> > > > * Addressed the small amount of feedback that was there for v7.
> > > > * Rebased.
> > > > * Added a new patch that prevents us from spamming the logs with idle
> > > >   tasks.
> > > > * Added an extra patch to gracefully fall back to regular IPIs if
> > > >   pseudo-NMIs aren't there.
> > > >
> > > > Since there appear to be a few different patches series related to
> > > > being able to use NMIs to get stack traces of crashed systems, let me
> > > > try to organize them to the best of my understanding:
> > > >
> > > > a) This series. On its own, a) will (among other things) enable stack
> > > >traces of all running processes with the soft lockup detector if
> > > >you've enabled the sysctl "kernel.softlockup_all_cpu_backtrace". On
> > > >its own, a) doesn't give a hard lockup detector.
> > > >
> > > > b) A different recently-posted series [2] that adds a hard lockup
> > > >detector based on perf. On its own, b) gives a stack crawl of the
> > > >locked up CPU but no stack crawls of other CPUs (even if they're
> > > >locked too). Together with a) + b) we get everything (full lockup
> > > >detect, full ability to get stack crawls).
> > > >
> > > > c) The old Android "buddy" hard lockup detector [3] that I'm
> > > >considering trying to upstream. If b) lands then I believe c) would
> > > >be redundant (at least for arm64). c) on its own is really only
> > > >useful on arm64 for platforms that can print CPU_DBGPCSR somehow
> > > >(see [4]). a) + c) is roughly as good as a) + b).
> >
> > > It's been 3 weeks and I haven't heard a peep on this series. That
> > > means nobody has any objections and it's all good to land, right?
> > > Right? :-P
> >
> > FWIW, there are still longstanding soundness issues in the arm64 pseudo-NMI
> > support (and fixing that requires an overhaul of our DAIF / IRQ flag
> > management, which I've been chipping away at for a number of releases), so I
> > hadn't looked at this in detail yet because the foundations are still 
> > somewhat
> > dodgy.
> >
> > I appreciate that this has been around for a while, and it's on my queue to
> > look at.
>
> Ah, thanks for the heads up! We've been thinking about turning this on
> in production in ChromeOS because it will help us track down a whole
> class of field-generated crash reports that are otherwise opaque to
> us. It sounds as if maybe that's not a good idea quite yet? Do you
> have any idea of how much farther along this needs to go?

I'm still very interested in this topic. Do you have anything concrete
that is broken? Your reply gives me the vibe that there have been a
bunch of bugs found recently and, while there are no known issues,
you're worried that there might be something lingering still. Is that
correct, or do you have something concrete that's broken? Is this
anything others could help with? I could make an attempt, or we might
be able to rope some of the others interested in this topic and more
GIC-knowledgeable to help?

I still have a goal for turning this on for production but obviously
don't want to make the device crashier because of it.

Also: if this really has known bugs, should we put a big WARN_ON splat
if anyone tries to turn on pseudo NMIs? Without that, it feels like
it's a bit of a footgun.


> ...of
> course, we've also run into issues with Mediatek devices because they
> don't save/restore GICR registers properly [1]. In theory, we might be
> able to work around that in the kernel.

To followup with this, we've formulated a plan for dealing with
Mediatek Chromebooks. I doubt anyone is truly interested, but if
anyone is the details are in the public 

Re: [PATCH v5 12/18] watchdog/hardlockup: Rename some "NMI watchdog" constants/function

[Doug Anderson]

Hi,

On Wed, May 24, 2023 at 6:38 AM Petr Mladek  wrote:
>
> On Fri 2023-05-19 10:18:36, Douglas Anderson wrote:
> > Do a search and replace of:
> > - NMI_WATCHDOG_ENABLED => WATCHDOG_HARDLOCKUP_ENABLED
> > - SOFT_WATCHDOG_ENABLED => WATCHDOG_SOFTOCKUP_ENABLED
> > - watchdog_nmi_ => watchdog_hardlockup_
> > - nmi_watchdog_available => watchdog_hardlockup_available
> > - nmi_watchdog_user_enabled => watchdog_hardlockup_user_enabled
> > - soft_watchdog_user_enabled => watchdog_softlockup_user_enabled
> > - NMI_WATCHDOG_DEFAULT => WATCHDOG_HARDLOCKUP_DEFAULT
> >
> > Then update a few comments near where names were changed.
> >
> > This is specifically to make it less confusing when we want to
> > introduce the buddy hardlockup detector, which isn't using NMIs. As
> > part of this, we sanitized a few names for consistency.
> >
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -30,17 +30,17 @@
> >  static DEFINE_MUTEX(watchdog_mutex);
> >
> >  #if defined(CONFIG_HARDLOCKUP_DETECTOR) || 
> > defined(CONFIG_HAVE_NMI_WATCHDOG)
> > -# define NMI_WATCHDOG_DEFAULT1
> > +# define WATCHDOG_HARDLOCKUP_DEFAULT 1
> >  #else
> > -# define NMI_WATCHDOG_DEFAULT0
> > +# define WATCHDOG_HARDLOCKUP_DEFAULT 0
> >  #endif
> >
> >  unsigned long __read_mostly watchdog_enabled;
> >  int __read_mostly watchdog_user_enabled = 1;
> > -int __read_mostly nmi_watchdog_user_enabled = NMI_WATCHDOG_DEFAULT;
> > -int __read_mostly soft_watchdog_user_enabled = 1;
> > +int __read_mostly watchdog_hardlockup_user_enabled = 
> > WATCHDOG_HARDLOCKUP_DEFAULT;
> > +int __read_mostly watchdog_softlockup_user_enabled = 1;
>
> I still see nmi_watchdog_user_enabled and soft_watchdog_user_enabled
> in include/linux/nmi.h. They are declared there and also mentioned
> in a comment.
>
> It seems that they do not actually need to be declared there.
> I guess that it was need for the /proc stuff. But it was
> moved into kernel/watchdog.c by the commit commit dd0693fdf054f2ed37
> ("watchdog: move watchdog sysctl interface to watchdog.c").
>
> >  int __read_mostly watchdog_thresh = 10;
> > -static int __read_mostly nmi_watchdog_available;
> > +static int __read_mostly watchdog_hardlockup_available;
> >
> >  struct cpumask watchdog_cpumask __read_mostly;
> >  unsigned long *watchdog_cpumask_bits = cpumask_bits(_cpumask);
>
> Otherwise, I like the changes.
>
> With the following:
>
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index 83076bf70ce8..d14fe345eae9 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -17,8 +17,6 @@ void lockup_detector_soft_poweroff(void);
>  void lockup_detector_cleanup(void);
>
>  extern int watchdog_user_enabled;
> -extern int nmi_watchdog_user_enabled;
> -extern int soft_watchdog_user_enabled;
>  extern int watchdog_thresh;
>  extern unsigned long watchdog_enabled;
>
> @@ -68,8 +66,8 @@ static inline void reset_hung_task_detector(void) { }
>   * 'watchdog_enabled' variable. Each lockup detector has its dedicated bit -
>   * bit 0 for the hard lockup detector and bit 1 for the soft lockup detector.
>   *
> - * 'watchdog_user_enabled', 'nmi_watchdog_user_enabled' and
> - * 'soft_watchdog_user_enabled' are variables that are only used as an
> + * 'watchdog_user_enabled', 'watchdog_hardlockup_user_enabled' and
> + * 'watchdog_softlockup_user_enabled' are variables that are only used as an
>   * 'interface' between the parameters in /proc/sys/kernel and the internal
>   * state bits in 'watchdog_enabled'. The 'watchdog_thresh' variable is
>   * handled differently because its value is not boolean, and the lockup
>
> Reviewed-by: Petr Mladek 
>
> Even better might be to remove the unused declaration in a separate
> patch. I think that more declarations are not needed after moving
> the /proc stuff into kernel/watchdog.c.
>
> But it might also be fixed later.

Breadcrumbs: I squashed your suggestion together with Tom's patch and
posted the result:

https://lore.kernel.org/r/20230525162822.1.I0fb41d138d158c9230573eaa37dc56afa2fb14ee@changeid

-Doug

Re: [PATCH v5 14/18] watchdog/hardlockup: detect hard lockups using secondary (buddy) CPUs

[Doug Anderson]

Hi,

On Thu, May 25, 2023 at 9:27 AM Petr Mladek  wrote:
>
> On Fri 2023-05-19 10:18:38, Douglas Anderson wrote:
> > Implement a hardlockup detector that doesn't doesn't need any extra
> > arch-specific support code to detect lockups. Instead of using
> > something arch-specific we will use the buddy system, where each CPU
> > watches out for another one. Specifically, each CPU will use its
> > softlockup hrtimer to check that the next CPU is processing hrtimer
> > interrupts by verifying that a counter is increasing.
> >
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -85,7 +85,7 @@ __setup("nmi_watchdog=", hardlockup_panic_setup);
> >
> >  #endif /* CONFIG_HARDLOCKUP_DETECTOR */
> >
> > -#if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
> > +#if defined(CONFIG_HARDLOCKUP_DETECTOR_COUNTS_HRTIMER)
> >
> >  static DEFINE_PER_CPU(atomic_t, hrtimer_interrupts);
> >  static DEFINE_PER_CPU(int, hrtimer_interrupts_saved);
> > @@ -106,6 +106,14 @@ notrace void arch_touch_nmi_watchdog(void)
> >  }
> >  EXPORT_SYMBOL(arch_touch_nmi_watchdog);
> >
> > +void watchdog_hardlockup_touch_cpu(unsigned int cpu)
> > +{
> > + per_cpu(watchdog_hardlockup_touched, cpu) = true;
> > +
> > + /* Match with smp_rmb() in watchdog_hardlockup_check() */
> > + smp_wmb();
>
> It is great that you described where the related barrier is.
>
> Another important information is what exactly is synchronized.
> And I am actually not sure what we are synchronizing here.
>
> My understanding is that a write barrier should synchronize
> related writes, for example:
>
> X = ...;
> /* Make sure that X is modified before Y */
> smp_wmb();
> Y = ...;
>
> And the related read barrier should synchronize the related reads,
> for example:
>
> if (test(Y)) {
> /*
>  * Make sure that we use the updated X when
>  * we saw the updated Y.
>  */
>  smp_rmb();
>  do_something(X);
>  }
>
> IMHO, we do not need any barrier here because we have only
> one variable "watchdog_hardlockup_touched" here.
> watchdog_hardlockup_check() will either see the updated value
> or not. But it does not synchronize it against any other
> variables or values.

Fair. These barriers were present in the original buddy lockup
detector that we've been carrying in ChromeOS but that doesn't
necessarily mean that they were there for a good reason.

Reasoning about weakly ordered memory always makes my brain hurt and I
never feel confident at the end that I got the right answer and, of
course, this is coupled by the fact that if I have a logic error in my
reasoning that it might cause a rare / subtle bug. :( When possible I
try to write code that uses full blown locks to make it easier to
reason about (even if less efficient), but that's not necessarily
possible here. While we obviously don't just want to sprinkle barriers
all over the code, IMO it's not a terrible sin to put a barrier in a
case where it makes it easier to reason about the order of things.

In any case, I guess in this case I would worry about some sort of
ordering race when enabling / disabling the buddy lockup detector. At
the end of the buddy's watchdog_hardlockup_enable() /
watchdog_hardlockup_disable() we adjust the "watchdog_cpus" which
changes buddy assignments. Without a barrier, I _think_ it would be
possible for a new CPU to notice the change in buddies without
noticing the touch. Does that match your understanding? Now when
reasoning about CPUs going online/offline we need to consider this
extra case and we have to decide if there's any chance it could lead
to a false lockup detection. With the memory barriers here, it's a
little easier to think about.

Did the above convince you about keeping the barriers? If so, do you
have any suggested comment that would make it clearer?


> > +}
> > +
> >  static bool is_hardlockup(unsigned int cpu)
> >  {
> >   int hrint = atomic_read(_cpu(hrtimer_interrupts, cpu));
> > @@ -443,11 +454,15 @@ static enum hrtimer_restart watchdog_timer_fn(struct 
> > hrtimer *hrtimer)
> >   struct pt_regs *regs = get_irq_regs();
> >   int duration;
> >   int softlockup_all_cpu_backtrace = 
> > sysctl_softlockup_all_cpu_backtrace;
> > + unsigned long hrtimer_interrupts;
> >
> >   if (!watchdog_enabled)
> >   return HRTIMER_NORESTART;
> >
> > - watchdog_hardlockup_kick();
> > + hrtimer_interrupts = watchdog_hardlockup_kick();
> > +
> > + /* test for hardlockups */
>
> I would omit the comment. It is not valid when perf detector is used.
> And checking the buddy is clear from the function name.
>
> > + watchdog_buddy_check_hardlockup(hrtimer_interrupts);
>
> I would personally move this into watchdog_hardlockup_kick().
> watchdog_timer_fn() is already complex enough. And checking
> the buddy when kicking a CPU makes sense.

Sure, I'll add that to my list of things to follow-up 

Re: [PATCH v5 13/18] watchdog/hardlockup: Have the perf hardlockup use __weak functions more cleanly

[Doug Anderson]

Hi,

On Wed, May 24, 2023 at 6:59 AM Petr Mladek  wrote:
>
> On Fri 2023-05-19 10:18:37, Douglas Anderson wrote:
> > The fact that there watchdog_hardlockup_enable(),
> > watchdog_hardlockup_disable(), and watchdog_hardlockup_probe() are
> > declared __weak means that the configured hardlockup detector can
> > define non-weak versions of those functions if it needs to. Instead of
> > doing this, the perf hardlockup detector hooked itself into the
> > default __weak implementation, which was a bit awkward. Clean this up.
> >
> > >From comments, it looks as if the original design was done because the
> > __weak function were expected to implemented by the architecture and
> > not by the configured hardlockup detector. This got awkward when we
> > tried to add the buddy lockup detector which was not arch-specific but
> > wanted to hook into those same functions.
> >
> > This is not expected to have any functional impact.
> >
> > @@ -187,27 +187,33 @@ static inline void watchdog_hardlockup_kick(void) { }
> >  #endif /* !CONFIG_HARDLOCKUP_DETECTOR_PERF */
> >
> >  /*
> > - * These functions can be overridden if an architecture implements its
> > - * own hardlockup detector.
> > + * These functions can be overridden based on the configured hardlockdup 
> > detector.
> >   *
> >   * watchdog_hardlockup_enable/disable can be implemented to start and stop 
> > when
> > - * softlockup watchdog start and stop. The arch must select the
> > + * softlockup watchdog start and stop. The detector must select the
> >   * SOFTLOCKUP_DETECTOR Kconfig.
> >   */
> > -void __weak watchdog_hardlockup_enable(unsigned int cpu)
> > -{
> > - hardlockup_detector_perf_enable();
> > -}
> > +void __weak watchdog_hardlockup_enable(unsigned int cpu) { }
> >
> > -void __weak watchdog_hardlockup_disable(unsigned int cpu)
> > -{
> > - hardlockup_detector_perf_disable();
> > -}
> > +void __weak watchdog_hardlockup_disable(unsigned int cpu) { }
> >
> >  /* Return 0, if a hardlockup watchdog is available. Error code otherwise */
> >  int __weak __init watchdog_hardlockup_probe(void)
> >  {
> > - return hardlockup_detector_perf_init();
> > + /*
> > +  * If CONFIG_HAVE_NMI_WATCHDOG is defined then an architecture
> > +  * is assumed to have the hard watchdog available and we return 0.
> > +  */
> > + if (IS_ENABLED(CONFIG_HAVE_NMI_WATCHDOG))
> > + return 0;
> > +
> > + /*
> > +  * Hardlockup detectors other than those using 
> > CONFIG_HAVE_NMI_WATCHDOG
> > +  * are required to implement a non-weak version of this probe function
> > +  * to tell whether they are available. If they don't override then
> > +  * we'll return -ENODEV.
> > +  */
> > + return -ENODEV;
> >  }
>
> When thinking more about it. It is weird that we need to handle
> CONFIG_HAVE_NMI_WATCHDOG in this default week function.
>
> It should be handled in watchdog_hardlockup_probe() implemented
> in kernel/watchdog_perf.c.
>
> IMHO, the default __weak function could always return -ENODEV;
>
> Would it make sense, please?

I don't quite understand. I'd agree that the special case for
CONFIG_HAVE_NMI_WATCHDOG is ugly, but it was also ugly before. IMO
it's actually a little less ugly / easier to understand after my
patch. ...but let me walk through how I think this special case works
and maybe you can tell me where I'm confused.

The first thing to understand is that CONFIG_HARDLOCKUP_DETECTOR_PERF
and CONFIG_HAVE_NMI_WATCHDOG are mutually exclusive from each other.
This was true before any of my patches and is still true after them.
Specifically, if CONFIG_HAVE_NMI_WATCHDOG is defined then an
architecture implements arch_touch_nmi_watchdog() (as documented in
the Kconfig docs for HAVE_NMI_WATCHDOG). Looking at the tree before my
series you can see that the perf hardlockup detector also implemented
arch_touch_nmi_watchdog(). This would have caused a conflict. The
mutual exclusion was presumably enforced by an architecture not
defining both HAVE_NMI_WATCHDOG and HAVE_HARDLOCKUP_DETECTOR_PERF.

The second thing to understand is that an architecture that defines
CONFIG_HAVE_NMI_WATCHDOG is _not_ required to implement
watchdog_hardlockup_probe() (used to be called watchdog_nmi_probe()).
Maybe this should change, but at the very least it appears that
SPARC64 defines HAVE_NMI_WATCHDOG but doesn't define
watchdog_hardlockup_probe() AKA watchdog_nmi_probe(). Anyone who
defines CONFIG_HAVE_NMI_WATCHDOG and doesn't implement
watchdog_hardlockup_probe() is claiming that their watchdog needs no
probing and is always available.

So with that context:

1. We can't handle any special cases for CONFIG_HAVE_NMI_WATCHDOG in
"kernel/watchdog_perf.c". The special cases that we need to handle are
all for the cases where CONFIG_HARDLOCKUP_DETECTOR_PERF isn't defined
and that means "kernel/watchdog_perf.c" isn't included.

2. We can't have the default __weak function return -ENODEV because
CONFIG_HAVE_NMI_WATCHDOG doesn't require an 

Re: [PATCH v5 10/18] watchdog/hardlockup: Add a "cpu" param to watchdog_hardlockup_check()

[Doug Anderson]

Hi,

On Tue, May 23, 2023 at 9:02 AM Petr Mladek  wrote:
>
> On Fri 2023-05-19 10:18:34, Douglas Anderson wrote:
> > In preparation for the buddy hardlockup detector where the CPU
> > checking for lockup might not be the currently running CPU, add a
> > "cpu" parameter to watchdog_hardlockup_check().
> >
> > As part of this change, make hrtimer_interrupts an atomic_t since now
> > the CPU incrementing the value and the CPU reading the value might be
> > different. Technially this could also be done with just READ_ONCE and
> > WRITE_ONCE, but atomic_t feels a little cleaner in this case.
> >
> > While hrtimer_interrupts is made atomic_t, we change
> > hrtimer_interrupts_saved from "unsigned long" to "int". The "int" is
> > needed to match the data type backing atomic_t for hrtimer_interrupts.
> > Even if this changes us from 64-bits to 32-bits (which I don't think
> > is true for most compilers), it doesn't really matter. All we ever do
> > is increment it every few seconds and compare it to an old value so
> > 32-bits is fine (even 16-bits would be). The "signed" vs "unsigned"
> > also doesn't matter for simple equality comparisons.
> >
> > hrtimer_interrupts_saved is _not_ switched to atomic_t nor even
> > accessed with READ_ONCE / WRITE_ONCE. The hrtimer_interrupts_saved is
> > always consistently accessed with the same CPU. NOTE: with the
> > upcoming "buddy" detector there is one special case. When a CPU goes
> > offline/online then we can change which CPU is the one to consistently
> > access a given instance of hrtimer_interrupts_saved. We still can't
> > end up with a partially updated hrtimer_interrupts_saved, however,
> > because we end up petting all affected CPUs to make sure the new and
> > old CPU can't end up somehow read/write hrtimer_interrupts_saved at
> > the same time.
> >
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -87,29 +87,34 @@ __setup("nmi_watchdog=", hardlockup_panic_setup);
> >
> >  #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
> >
> > -static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts);
> > -static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
> > +static DEFINE_PER_CPU(atomic_t, hrtimer_interrupts);
> > +static DEFINE_PER_CPU(int, hrtimer_interrupts_saved);
> >  static DEFINE_PER_CPU(bool, watchdog_hardlockup_warned);
> >  static unsigned long watchdog_hardlockup_all_cpu_dumped;
> >
> > -static bool is_hardlockup(void)
> > +static bool is_hardlockup(unsigned int cpu)
> >  {
> > - unsigned long hrint = __this_cpu_read(hrtimer_interrupts);
> > + int hrint = atomic_read(_cpu(hrtimer_interrupts, cpu));
> >
> > - if (__this_cpu_read(hrtimer_interrupts_saved) == hrint)
> > + if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
> >   return true;
> >
> > - __this_cpu_write(hrtimer_interrupts_saved, hrint);
> > + /*
> > +  * NOTE: we don't need any fancy atomic_t or READ_ONCE/WRITE_ONCE
> > +  * for hrtimer_interrupts_saved. hrtimer_interrupts_saved is
> > +  * written/read by a single CPU.
> > +  */
> > + per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
> >
> >   return false;
> >  }
> >
> >  static void watchdog_hardlockup_kick(void)
> >  {
> > - __this_cpu_inc(hrtimer_interrupts);
> > + atomic_inc(raw_cpu_ptr(_interrupts));
>
> Is there any particular reason why raw_*() is needed, please?
>
> My expectation is that the raw_ API should be used only when
> there is a good reason for it. Where a good reason might be
> when the checks might fail but the consistency is guaranteed
> another way.
>
> IMHO, we should use:
>
> atomic_inc(this_cpu_ptr(_interrupts));
>
> To be honest, I am a bit lost in the per_cpu API definitions.
>
> But this_cpu_ptr() seems to be implemented the same way as
> per_cpu_ptr() when CONFIG_DEBUG_PREEMPT is enabled.
> And we use per_cpu_ptr() in is_hardlockup().
>
> Also this_cpu_ptr() is used more commonly:
>
> $> git grep this_cpu_ptr | wc -l
> 1385
> $> git grep raw_cpu_ptr | wc -l
> 114

Hmmm, I think maybe I confused myself. The old code purposely used the
double-underscore prefixed version of this_cpu_inc(). I couldn't find
a double-underscore version of this_cpu_ptr() and I somehow convinced
myself that the raw() version was the right equivalent version.

You're right that this_cpu_ptr() is a better choice here and I don't
see any reason why we'd need the raw version.

> >  }
> >
> > -void watchdog_hardlockup_check(struct pt_regs *regs)
> > +void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
> >  {
> >   /*
> >* Check for a hardlockup by making sure the CPU's timer
> > @@ -117,35 +122,42 @@ void watchdog_hardlockup_check(struct pt_regs *regs)
> >* fired multiple times before we overflow'd. If it hasn't
> >* then this is a good indication the cpu is stuck
> >*/
> > - if (is_hardlockup()) {
> > + if (is_hardlockup(cpu)) {
> >   unsigned int this_cpu = smp_processor_id();
> > 

Re: [PATCH v4 13/17] watchdog/hardlockup: detect hard lockups using secondary (buddy) CPUs

[Doug Anderson]

Hi,

On Mon, May 8, 2023 at 8:52 AM Doug Anderson  wrote:
>
> Hmmm, but I don't think you really need "all-to-all" checking to get
> the stacktraces you want, do you? Each CPU can be "watching" exactly
> one other CPU, but then when we actually lock up we could check all of
> them and dump stacks on all the ones that are locked up. I think this
> would be a fairly easy improvement for the buddy system. I'll leave it
> out for now just to keep things simple for the initial landing, but it
> wouldn't be hard to add. Then I think the two SMP systems  (buddy vs.
> all-to-all) would be equivalent in terms of functionality?

FWIW, I take back my "this would be fairly easy" comment. :-P ...or,
at least I'll acknowledge that the easy way has some tradeoffs. It
wouldn't be trivially easy to just snoop on the data of the other
buddies because the watching processors aren't necessarily
synchronized with each other.

That being said, if someone really wanted to report on other locked
CPUs before doing a panic() and was willing to delay the panic, it
probably wouldn't be too hard to put in a mode where the CPU that
detects the first lockup could do some extra work to look for lockups.
Maybe it could send a normal IPI to other CPUs and see if they respond
or maybe it could take over monitoring all CPUs and wait one extra
period.

In any case, I'm not planning on implementing this now, but at least
wanted to document thoughts. ;-)

> With my simplistic solution
> of just allowing the buddy detector to be enabled in parallel with a
> perf-based detector then we wouldn't have this level of coordination,
> but I'll assume that's OK for the initial landing.

I dug into this more as well and I also wanted to note that, at least
for now, I'm not going to include support to turn on both the buddy
and perf lockup detectors in the common core. In order to do this and
not have them stomp on each other then I think we need extra
coordination or two copies of the interrupt count / saved interrupt
count and, at least at this point in time, it doesn't seem worth it
for a halfway solution. From everything I've heard there is a push on
many x86 machines to get off the perf lockup detector anyway to free
up the resources. Someone could look at adding this complexity later.

-Doug

Re: [PATCH v4 10/17] watchdog/hardlockup: Move perf hardlockup watchdog petting to watchdog.c

[Doug Anderson]

Hi,

On Thu, May 11, 2023 at 8:46 AM Petr Mladek  wrote:
>
> > @@ -111,6 +125,11 @@ static void watchdog_hardlockup_interrupt_count(void)
> >
> >  void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
> >  {
> > + if (__this_cpu_read(watchdog_hardlockup_touch)) {
> > + __this_cpu_write(watchdog_hardlockup_touch, false);
> > + return;
> > + }
>
> If we clear watchdog_hardlockup_touch() here then
> watchdog_hardlockup_check() won't be called yet another
> watchdog_hrtimer_sample_threshold perior.
>
> It means that any touch will cause ignoring one full period.
> The is_hardlockup() check will be done after full two periods.
>
> It is not ideal, see below.
>
> > +
> >   /*
> >* Check for a hardlockup by making sure the CPU's timer
> >* interrupt is incrementing. The timer interrupt should have
> > diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
> > index 9be90b2a2ea7..547917ebd5d3 100644
> > --- a/kernel/watchdog_perf.c
> > +++ b/kernel/watchdog_perf.c
> > @@ -112,11 +98,6 @@ static void watchdog_overflow_callback(struct 
> > perf_event *event,
> >   /* Ensure the watchdog never gets throttled */
> >   event->hw.interrupts = 0;
> >
> > - if (__this_cpu_read(watchdog_nmi_touch) == true) {
> > - __this_cpu_write(watchdog_nmi_touch, false);
> > - return;
> > - }
>
> The original code looks wrong. arch_touch_nmi_watchdog() caused
> skipping only one period of the perf event.
>
> I would expect that it caused restarting the period,
> something like:
>
> if (__this_cpu_read(watchdog_nmi_touch) == true) {
> /*
>  * Restart the period after which the interrupt
>  * counter is checked.
>  */
> __this_cpu_write(nmi_rearmed, 0);
> __this_cpu_write(last_timestamp, now);
> __this_cpu_write(watchdog_nmi_touch, false);
> return;
> }
>
> By other words, we should restart the period in the very next perf
> event after the watchdog was touched.
>
> That said, the new code looks better than the original.
> IMHO, the original code was prone to false positives.

I had a little bit of a hard time following, but I _think_ the "tl;dr"
of all the above is that my change is fine. If I misunderstood, please
yell.


> Best Regards,
> Petr
>
> PS: It might be worth fixing this problem in a separate patch at the
> beginning of this patchset. It might be a candidate for stable
> backports.

Done. It's now its own patch and early in the series.

Re: [PATCH v4 09/17] watchdog/hardlockup: Add a "cpu" param to watchdog_hardlockup_check()

[Doug Anderson]

Hi,

On Thu, May 11, 2023 at 7:14 AM Petr Mladek  wrote:
>
> On Thu 2023-05-04 15:13:41, Douglas Anderson wrote:
> > In preparation for the buddy hardlockup detector where the CPU
> > checking for lockup might not be the currently running CPU, add a
> > "cpu" parameter to watchdog_hardlockup_check().
> >
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -92,14 +92,14 @@ static DEFINE_PER_CPU(unsigned long, 
> > hrtimer_interrupts_saved);
> >  static DEFINE_PER_CPU(bool, watchdog_hardlockup_processed);
> >  static unsigned long watchdog_hardlockup_dumped_stacks;
> >
> > -static bool watchdog_hardlockup_is_lockedup(void)
> > +static bool watchdog_hardlockup_is_lockedup(unsigned int cpu)
> >  {
> > - unsigned long hrint = __this_cpu_read(hrtimer_interrupts);
> > + unsigned long hrint = per_cpu(hrtimer_interrupts, cpu);
>
> My radar tells me that this should be
> READ_ONCE(per_cpu(hrtimer_interrupts, cpu)) when the value might
> be modified on another CPU. Otherwise, the compiler is allowed
> to split the read into more instructions.
>
> It will be needed for the buddy detector. And it will require
> also incrementing the value in watchdog_hardlockup_interrupt_count()
> an atomic way.
>
> Note that __this_cpu_inc_return() does not guarantee atomicity
> according to my understanding. In theory, the following should
> work because counter will never be incremented in parallel:
>
> static unsigned long watchdog_hardlockup_interrupt_count(void)
> {
> unsigned long count;
>
> count = __this_cpu_read(hrtimer_interrupts);
> count++;
> WRITE_ONCE(*raw_cpu_ptr(hrtimer_interrupts), count);
> }
>
> but it is nasty. A more elegant solution might be using atomic_t
> for hrtimer_interrupts counter.

I switched it over to atomic_t.


> > - if (__this_cpu_read(hrtimer_interrupts_saved) == hrint)
> > + if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
> >   return true;
> >
> > - __this_cpu_write(hrtimer_interrupts_saved, hrint);
> > + per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
>
> IMHO, hrtimer_interrupts_saved might be handled this way.
> The value is read/written only by this function.
>
> The buddy watchdog should see consistent values even when
> the buddy CPU goes offline. This check should never race
> because this CPU should get touched when another buddy
> gets assigned.
>
> Well, it would deserve a comment.

I spent a bunch of time thinking about this too and I agree that for
hrtimer_interrupts_saved we don't need atomic_t nor even
READ_ONCE/WRITE_ONCE. I've add a comment and a note in the commit
message in v5.

Re: [PATCH v8 00/10] arm64: Add framework to turn an IPI as NMI

[Doug Anderson]

Hi,

On Wed, May 10, 2023 at 9:30 AM Mark Rutland  wrote:
>
> On Wed, May 10, 2023 at 08:28:17AM -0700, Doug Anderson wrote:
> > Hi,
>
> Hi Doug,
>
> > On Wed, Apr 19, 2023 at 3:57 PM Douglas Anderson  
> > wrote:
> > > This is an attempt to resurrect Sumit's old patch series [1] that
> > > allowed us to use the arm64 pseudo-NMI to get backtraces of CPUs and
> > > also to round up CPUs in kdb/kgdb. The last post from Sumit that I
> > > could find was v7, so I called this series v8. I haven't copied all of
> > > his old changelongs here, but you can find them from the link.
> > >
> > > Since v7, I have:
> > > * Addressed the small amount of feedback that was there for v7.
> > > * Rebased.
> > > * Added a new patch that prevents us from spamming the logs with idle
> > >   tasks.
> > > * Added an extra patch to gracefully fall back to regular IPIs if
> > >   pseudo-NMIs aren't there.
> > >
> > > Since there appear to be a few different patches series related to
> > > being able to use NMIs to get stack traces of crashed systems, let me
> > > try to organize them to the best of my understanding:
> > >
> > > a) This series. On its own, a) will (among other things) enable stack
> > >traces of all running processes with the soft lockup detector if
> > >you've enabled the sysctl "kernel.softlockup_all_cpu_backtrace". On
> > >its own, a) doesn't give a hard lockup detector.
> > >
> > > b) A different recently-posted series [2] that adds a hard lockup
> > >detector based on perf. On its own, b) gives a stack crawl of the
> > >locked up CPU but no stack crawls of other CPUs (even if they're
> > >locked too). Together with a) + b) we get everything (full lockup
> > >detect, full ability to get stack crawls).
> > >
> > > c) The old Android "buddy" hard lockup detector [3] that I'm
> > >considering trying to upstream. If b) lands then I believe c) would
> > >be redundant (at least for arm64). c) on its own is really only
> > >useful on arm64 for platforms that can print CPU_DBGPCSR somehow
> > >(see [4]). a) + c) is roughly as good as a) + b).
>
> > It's been 3 weeks and I haven't heard a peep on this series. That
> > means nobody has any objections and it's all good to land, right?
> > Right? :-P
>
> FWIW, there are still longstanding soundness issues in the arm64 pseudo-NMI
> support (and fixing that requires an overhaul of our DAIF / IRQ flag
> management, which I've been chipping away at for a number of releases), so I
> hadn't looked at this in detail yet because the foundations are still somewhat
> dodgy.
>
> I appreciate that this has been around for a while, and it's on my queue to
> look at.

Ah, thanks for the heads up! We've been thinking about turning this on
in production in ChromeOS because it will help us track down a whole
class of field-generated crash reports that are otherwise opaque to
us. It sounds as if maybe that's not a good idea quite yet? Do you
have any idea of how much farther along this needs to go? ...of
course, we've also run into issues with Mediatek devices because they
don't save/restore GICR registers properly [1]. In theory, we might be
able to work around that in the kernel.

In any case, even if there are bugs that would prevent turning this on
for production, it still seems like we could still land this series.
It simply wouldn't do anything until someone turned on pseudo NMIs,
which wouldn't happen till the kinks are worked out.

...actually, I guess I should say that if all the patches of the
current series do land then it actually _would_ still do something,
even without pseudo-NMI. Assuming the last patch looks OK, it would at
least start falling back to using regular IPIs to do backtraces. That
wouldn't get backtraces on hard locked up CPUs but it would be better
than what we have today where we don't get any backtraces. This would
get arm64 on par with arm32...

[1] https://issuetracker.google.com/281831288

Re: [PATCH v8 00/10] arm64: Add framework to turn an IPI as NMI

[Doug Anderson]

Hi,

On Wed, Apr 19, 2023 at 3:57 PM Douglas Anderson  wrote:
>
> This is an attempt to resurrect Sumit's old patch series [1] that
> allowed us to use the arm64 pseudo-NMI to get backtraces of CPUs and
> also to round up CPUs in kdb/kgdb. The last post from Sumit that I
> could find was v7, so I called this series v8. I haven't copied all of
> his old changelongs here, but you can find them from the link.
>
> Since v7, I have:
> * Addressed the small amount of feedback that was there for v7.
> * Rebased.
> * Added a new patch that prevents us from spamming the logs with idle
>   tasks.
> * Added an extra patch to gracefully fall back to regular IPIs if
>   pseudo-NMIs aren't there.
>
> Since there appear to be a few different patches series related to
> being able to use NMIs to get stack traces of crashed systems, let me
> try to organize them to the best of my understanding:
>
> a) This series. On its own, a) will (among other things) enable stack
>traces of all running processes with the soft lockup detector if
>you've enabled the sysctl "kernel.softlockup_all_cpu_backtrace". On
>its own, a) doesn't give a hard lockup detector.
>
> b) A different recently-posted series [2] that adds a hard lockup
>detector based on perf. On its own, b) gives a stack crawl of the
>locked up CPU but no stack crawls of other CPUs (even if they're
>locked too). Together with a) + b) we get everything (full lockup
>detect, full ability to get stack crawls).
>
> c) The old Android "buddy" hard lockup detector [3] that I'm
>considering trying to upstream. If b) lands then I believe c) would
>be redundant (at least for arm64). c) on its own is really only
>useful on arm64 for platforms that can print CPU_DBGPCSR somehow
>(see [4]). a) + c) is roughly as good as a) + b).
>
> [1] 
> https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.g...@linaro.org/
> [2] 
> https://lore.kernel.org/linux-arm-kernel/20220903093415.15850-1-lecopzer.c...@mediatek.com/
> [3] https://issuetracker.google.com/172213097
> [4] https://issuetracker.google.com/172213129
>
> Changes in v8:
> - dynamic_ipi_setup() and dynamic_ipi_teardown() no longer take cpu param
> - dynamic_ipi_setup() and dynamic_ipi_teardown() no longer take cpu param
> - Add loongarch support, too
> - Removed "#ifdef CONFIG_SMP" since arm64 is always SMP
> - "Tag the arm64 idle functions as __cpuidle" new for v8
> - "Provide a stub kgdb_nmicallback() if !CONFIG_KGDB" new for v8
> - "Fallback to a regular IPI if NMI isn't enabled" new for v8
>
> Douglas Anderson (3):
>   arm64: idle: Tag the arm64 idle functions as __cpuidle
>   kgdb: Provide a stub kgdb_nmicallback() if !CONFIG_KGDB
>   arm64: ipi_nmi: Fallback to a regular IPI if NMI isn't enabled
>
> Sumit Garg (7):
>   arm64: Add framework to turn IPI as NMI
>   irqchip/gic-v3: Enable support for SGIs to act as NMIs
>   arm64: smp: Assign and setup an IPI as NMI
>   nmi: backtrace: Allow runtime arch specific override
>   arm64: ipi_nmi: Add support for NMI backtrace
>   kgdb: Expose default CPUs roundup fallback mechanism
>   arm64: kgdb: Roundup cpus using IPI as NMI
>
>  arch/arm/include/asm/irq.h   |   2 +-
>  arch/arm/kernel/smp.c|   3 +-
>  arch/arm64/include/asm/irq.h |   4 ++
>  arch/arm64/include/asm/nmi.h |  17 +
>  arch/arm64/kernel/Makefile   |   2 +-
>  arch/arm64/kernel/idle.c |   4 +-
>  arch/arm64/kernel/ipi_nmi.c  | 103 +++
>  arch/arm64/kernel/kgdb.c |  18 ++
>  arch/arm64/kernel/smp.c  |   8 +++
>  arch/loongarch/include/asm/irq.h |   2 +-
>  arch/loongarch/kernel/process.c  |   3 +-
>  arch/mips/include/asm/irq.h  |   2 +-
>  arch/mips/kernel/process.c   |   3 +-
>  arch/powerpc/include/asm/nmi.h   |   2 +-
>  arch/powerpc/kernel/stacktrace.c |   3 +-
>  arch/sparc/include/asm/irq_64.h  |   2 +-
>  arch/sparc/kernel/process_64.c   |   4 +-
>  arch/x86/include/asm/irq.h   |   2 +-
>  arch/x86/kernel/apic/hw_nmi.c|   3 +-
>  drivers/irqchip/irq-gic-v3.c |  29 ++---
>  include/linux/kgdb.h |  13 
>  include/linux/nmi.h  |  12 ++--
>  kernel/debug/debug_core.c|   8 ++-
>  23 files changed, 217 insertions(+), 32 deletions(-)

It's been 3 weeks and I haven't heard a peep on this series. That
means nobody has any objections and it's all good to land, right?
Right? :-P

Seriously, though, I really think we should figure out how to get this
landed. There's obviously interest from several different parties and
I'm chomping at the bit waiting to spin this series according to your
wishes. I also don't think there's anything super scary/ugly here. IMO
the ideal situation is that folks are OK with the idea presented in
the last patch in the series ("arm64: ipi_nmi: Fallback to a regular
IPI if NMI isn't enabled") and then I can re-spin this series to be
_much_ simpler. That being said, I'm also OK with 

Re: [PATCH v4 05/17] watchdog/hardlockup: Rename touch_nmi_watchdog() to touch_hardlockup_watchdog()

[Doug Anderson]

Hi,

On Sun, May 7, 2023 at 6:35 PM Nicholas Piggin  wrote:
>
> On Sat May 6, 2023 at 2:37 AM AEST, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 4, 2023 at 7:51 PM Nicholas Piggin  wrote:
> > >
> > > On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > > > In preparation for the buddy hardlockup detector, rename
> > > > touch_nmi_watchdog() to touch_hardlockup_watchdog() to make it clear
> > > > that it will touch whatever hardlockup detector is configured. We'll
> > > > add a #define for the old name (touch_nmi_watchdog) so that we don't
> > > > have to touch every piece of code referring to the old name.
> > >
> > > Is this really helpful? Now it's got two names Could just leave it.
> > > If you insist then it'd be better just to rename everything in one
> > > go at the end of a merge window IMO. Conflicts would be trivial.
> >
> > I'm not picky here. I changed the name since Petr requested names to
> > be changed for any code I was touching [1] and so I threw this out as
> > a proposal. I agree that having two names can be confusing, but in
> > this case it didn't feel too terrible to me.
> >
> > I'd love to hear Petr's opinion on this name change. I'm happy with:
> >
> > a) This patch as it is.
> >
> > b) Dropping this patch (or perhaps just changing it to add comments).
> >
> > c) Changing this patch to rename all 70 uses of the old name. Assuming
> > this will go through Andrew Morton's tree, I'd be interested in
> > whether he's OK w/ this.
> >
> > d) Dropping this patch from this series but putting it on the
> > backburner to try to do later (so that the rename can happen at a time
> > when it's least disruptive).
> >
> >
> > > > Ideally this change would also rename the arch_touch_nmi_watchdog(),
> > > > but that is harder since arch_touch_nmi_watchdog() is exported with
> > > > EXPORT_SYMBOL() and thus is ABI. Add a comment next to the call to
> > > > hopefully alleviate some of the confusion here.
> > >
> > > We don't keep ABI fixed upstream.
> >
> > I'm happy to be corrected, but my understanding was that kernel devs
> > made an effort not to mess with things exported via "EXPORT_SYMBOL",
> > but things exported via "EXPORT_SYMBOL_GPL" were fair game.
>
> I don't think that's the case. If anything people might be a bit more
> inclined to accommodate GPL exports for out of tree modules that use
> them.
>
> > I guess maybe my patch calling it "ABI" is a stronger statement than
> > that, though. Doing a little more research, nobody wants to say that
> > things exported with "EXPORT_SYMBOL" are ABI, they just want to say
> > that we make an effort to have them be more stable.
>
> We wouldn't break any symbol for no reason, but in this case there is a
> good reason. If the name change is important for clarity then we change
> it. And this is about the easiest change for an out of tree module to
> deal with, so it should be no big deal for them.

OK, fair enough. My current plan is to wait a few more days to see if
anyone else chimes in with opinions. If I don't hear anything, in my
next version I will rename _neither_ touch_nmi_watchdog() nor
arch_touch_nmi_watchdog(). I'll still add comments indicating that
these functions touch the "hardlockup" watchdog but I won't attempt
the rename just to keep the series simpler.

-Doug

Re: [PATCH v4 13/17] watchdog/hardlockup: detect hard lockups using secondary (buddy) CPUs

[Doug Anderson]

Hi,

On Sun, May 7, 2023 at 6:05 PM Nicholas Piggin  wrote:
>
> > No, I wasn't aware of it. Interesting, it seems to basically enable
> > both types of hardlockup detectors together. If that really catches
> > more lockups, it seems like we could do the same thing for the buddy
> > system.
>
> It doesn't catch more lockups. On powerpc we don't have a reliable
> periodic NMI hence the SMP checker. But it is preferable that a CPU
> detects its own lockup because NMI IPIs can result in crashes if
> they are taken in certain critical sections.

Ah, interesting. Is the fact that NMI IPIs can crash the system
something specific to the way they're implemented in powerpc, or is
this something common in Linux? I've been assuming that IPI NMIs would
be roughly as reliable (or perhaps more reliable) than the NMI
timer/perf counter.


> > > It's all to
> > > all rather than buddy which makes it more complicated but arguably
> > > bit better functionality.
> >
> > Can you come up with an example crash where the "all to all" would
> > work better than the simple buddy system provided by this patch?
>
> CPU2 CPU3
> spin_lock_irqsave(A) spin_lock_irqsave(B)
> spin_lock_irqsave(B) spin_lock_irqsave(A)
>
> CPU1 will detect the lockup on CPU2, but CPU3's lockup won't be
> detected so we don't get the trace that can diagnose the bug.

Ah, that makes sense as a benefit if
"sysctl_hardlockup_all_cpu_backtrace" is false, which you'd probably
want if you had massively SMP systems. ...of course, if you had a lot
of cores then I'd imagine the "all-to-all" might become a lot of
overhead...

Hmmm, but I don't think you really need "all-to-all" checking to get
the stacktraces you want, do you? Each CPU can be "watching" exactly
one other CPU, but then when we actually lock up we could check all of
them and dump stacks on all the ones that are locked up. I think this
would be a fairly easy improvement for the buddy system. I'll leave it
out for now just to keep things simple for the initial landing, but it
wouldn't be hard to add. Then I think the two SMP systems  (buddy vs.
all-to-all) would be equivalent in terms of functionality?


Thinking about this more, I guess you must be assuming coordination
between the "SMP" and "non-SMP" checker. Specifically I guess you'd
want some guarantee that the "SMP" checker always fires before the
non-SMP checker because that would have a higher chance of getting a
stack trace without tickling the IPI NMI crash. ...but then once the
non-SMP checker printed its own stack trace you'd like the SMP checker
running on the same CPU to check to see if any other CPUs were locked
up so you could get their stacks as well. With my simplistic solution
of just allowing the buddy detector to be enabled in parallel with a
perf-based detector then we wouldn't have this level of coordination,
but I'll assume that's OK for the initial landing.


> Another thing I actually found it useful for is you can easily
> see if a core (i.e., all threads in the core) or a chip has
> died. Maybe more useful when doing presilicon and bring up work
> or firmware hacking, but still useful.

I'm probably biased, but for bringup work and stuff like this I
usually have kdb/kgdb enabled and then I could get stack traces for
whichever CPUs I want. :-P

-Doug

Re: [PATCH v4 11/17] watchdog/hardlockup: Rename some "NMI watchdog" constants/function

[Doug Anderson]

Hi,

On Thu, May 4, 2023 at 8:07 PM Nicholas Piggin  wrote:
>
> On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > Do a search and replace of:
> > - NMI_WATCHDOG_ENABLED => HARD_WATCHDOG_ENABLED
> > - watchdog_nmi_ => watchdog_hardlockup_
>
> These are just making prefixes inconsistent again.
>
> If you really want to do a prefix, I would call it hardlockup which
> probably best matches existing code and sysctl / boot stuff, and
> concentrate on non-static symbols.

As with other similar patches, I'm happy to drop this and am doing it
at Petr's request.

[1] https://lore.kernel.org/r/ZFErmshcrcikrSU1@alley

Re: [PATCH v4 08/17] watchdog/hardlockup: Style changes to watchdog_hardlockup_check() / ..._is_lockedup()

[Doug Anderson]

Hi,

On Thu, May 4, 2023 at 8:02 PM Nicholas Piggin  wrote:
>
> On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > These are tiny style changes:
> > - Add a blank line before a "return".
> > - Renames two globals to use the "watchdog_hld" prefix.
>
> Particularly static ones don't really need the namespace prefixes.

Renames are mostly at Petr's request. If I've misunderstood what he
wants here that I'm happy to remove them.


> Not sure if processed is better than warn.

I can undo this one if you want. It felt like we were doing more than
just warning, but if people think "warn" is a better way to describe
it then that's fine with me.


> allcpu_dumped is better
> than dumped_stacks though because the all-CPUs-dump is a particular
> thing.

OK, I can undo this and leave it as "allcpu_dumped".


[1] https://lore.kernel.org/r/ZFErmshcrcikrSU1@alley

Re: [PATCH v4 07/17] watchdog/hardlockup: Move perf hardlockup checking/panic to common watchdog.c

[Doug Anderson]

Hi,

On Thu, May 4, 2023 at 7:58 PM Nicholas Piggin  wrote:
>
> On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > The perf hardlockup detector works by looking at interrupt counts and
> > seeing if they change from run to run. The interrupt counts are
> > managed by the common watchdog code via its watchdog_timer_fn().
> >
> > Currently the API between the perf detector and the common code is a
> > function: is_hardlockup(). When the hard lockup detector sees that
> > function return true then it handles printing out debug info and
> > inducing a panic if necessary.
> >
> > Let's change the API a little bit in preparation for the buddy
> > hardlockup detector. The buddy hardlockup detector wants to print
>
> I think the name change is a gratuitous. Especially since it's now
> static.
>
> watchdog_hardlockup_ is a pretty long prefix too, hardlockup_
> should be enough?
>
> Seems okay otherwise though.

I went back and forth on names far too much when constructing this
patch series. Mostly I was trying to balance what looked good to me
and what Petr suggested [1]. I'm not super picky about the names and
I'm happy to change them all to a "hardlockup_" prefix. I'd love to
hear Petr's opinion.

[1] https://lore.kernel.org/r/ZFErmshcrcikrSU1@alley

Re: [PATCH v4 05/17] watchdog/hardlockup: Rename touch_nmi_watchdog() to touch_hardlockup_watchdog()

[Doug Anderson]

Hi,

On Thu, May 4, 2023 at 7:51 PM Nicholas Piggin  wrote:
>
> On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > In preparation for the buddy hardlockup detector, rename
> > touch_nmi_watchdog() to touch_hardlockup_watchdog() to make it clear
> > that it will touch whatever hardlockup detector is configured. We'll
> > add a #define for the old name (touch_nmi_watchdog) so that we don't
> > have to touch every piece of code referring to the old name.
>
> Is this really helpful? Now it's got two names Could just leave it.
> If you insist then it'd be better just to rename everything in one
> go at the end of a merge window IMO. Conflicts would be trivial.

I'm not picky here. I changed the name since Petr requested names to
be changed for any code I was touching [1] and so I threw this out as
a proposal. I agree that having two names can be confusing, but in
this case it didn't feel too terrible to me.

I'd love to hear Petr's opinion on this name change. I'm happy with:

a) This patch as it is.

b) Dropping this patch (or perhaps just changing it to add comments).

c) Changing this patch to rename all 70 uses of the old name. Assuming
this will go through Andrew Morton's tree, I'd be interested in
whether he's OK w/ this.

d) Dropping this patch from this series but putting it on the
backburner to try to do later (so that the rename can happen at a time
when it's least disruptive).


> > Ideally this change would also rename the arch_touch_nmi_watchdog(),
> > but that is harder since arch_touch_nmi_watchdog() is exported with
> > EXPORT_SYMBOL() and thus is ABI. Add a comment next to the call to
> > hopefully alleviate some of the confusion here.
>
> We don't keep ABI fixed upstream.

I'm happy to be corrected, but my understanding was that kernel devs
made an effort not to mess with things exported via "EXPORT_SYMBOL",
but things exported via "EXPORT_SYMBOL_GPL" were fair game.

I guess maybe my patch calling it "ABI" is a stronger statement than
that, though. Doing a little more research, nobody wants to say that
things exported with "EXPORT_SYMBOL" are ABI, they just want to say
that we make an effort to have them be more stable.

So certainly I should adjust my patch series not to call it ABI, but
I'm still on the fence about whether I should rename this or not. I'd
love to hear other opinions. This rename actually would be a lot
easier than the touch_nmi_watchdog() one since the code referencing
the name "arch_touch_nmi_watchdog" isn't spread so broadly through the
kernel.

[1] https://lore.kernel.org/r/ZFErmshcrcikrSU1@alley

-Doug

Re: [PATCH v4 13/17] watchdog/hardlockup: detect hard lockups using secondary (buddy) CPUs

[Doug Anderson]

Hi,

On Thu, May 4, 2023 at 7:36 PM Nicholas Piggin  wrote:
>
> On Fri May 5, 2023 at 8:13 AM AEST, Douglas Anderson wrote:
> > From: Colin Cross 
> >
> > Implement a hardlockup detector that doesn't doesn't need any extra
> > arch-specific support code to detect lockups. Instead of using
> > something arch-specific we will use the buddy system, where each CPU
> > watches out for another one. Specifically, each CPU will use its
> > softlockup hrtimer to check that the next CPU is processing hrtimer
> > interrupts by verifying that a counter is increasing.
>
> Powerpc's watchdog has an SMP checker, did you see it?

No, I wasn't aware of it. Interesting, it seems to basically enable
both types of hardlockup detectors together. If that really catches
more lockups, it seems like we could do the same thing for the buddy
system. If people want, I don't think it would be very hard to make
the buddy system _not_ exclusive of the perf system. Instead of having
the buddy system implement the "weak" functions I could just call the
buddy functions in the right places directly and leave the "weak"
functions for a more traditional hardlockup detector to implement.
Opinions?

Maybe after all this lands, the powerpc watchdog could move to use the
common code? As evidenced by this patch series, there's not really a
reason for the SMP detection to be platform specific.


> It's all to
> all rather than buddy which makes it more complicated but arguably
> bit better functionality.

Can you come up with an example crash where the "all to all" would
work better than the simple buddy system provided by this patch? It
seems like they would be equivalent, but I could be missing something.
Specifically they both need at least one non-locked-up CPU to detect a
problem. If one or more CPUs is locked up then we'll always detect it.
I suppose maybe you could provide a better error message at lockup
time saying that several CPUs were locked up and that could be
helpful. For now, I'd keep the current buddy system the way it is and
if you want to provide a patch improving things to be "all-to-all" in
the future that would be interesting to review.

Re: [PATCH 6/7] modules: Add CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC

[Doug Anderson]

Hi,

On Mon, Jan 24, 2022 at 1:22 AM Christophe Leroy
 wrote:
>
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -2022,8 +2022,11 @@ static int kdb_lsmod(int argc, const char **argv)
> if (mod->state == MODULE_STATE_UNFORMED)
> continue;
>
> -   kdb_printf("%-20s%8u  0x%px ", mod->name,
> -  mod->core_layout.size, (void *)mod);
> +   kdb_printf("%-20s%8u", mod->name, mod->core_layout.size);
> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> +   kdb_printf("/%8u  0x%px ", mod->data_layout.size);

Just counting percentages and arguments, it seems like something's
wrong in the above print statement.

-Doug

Re: [PATCH 04/20] Documentation: kgdb: eliminate duplicated word

[Doug Anderson]

Hi,

On Tue, Jul 7, 2020 at 11:05 AM Randy Dunlap  wrote:
>
> Drop the doubled word "driver".
>
> Signed-off-by: Randy Dunlap 
> Cc: Jonathan Corbet 
> Cc: linux-...@vger.kernel.org
> Cc: Jason Wessel 
> Cc: Daniel Thompson 
> Cc: Douglas Anderson 
> Cc: kgdb-bugrep...@lists.sourceforge.net
> ---
>  Documentation/dev-tools/kgdb.rst |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [REPOST PATCH v6 0/4] kgdb: Fix kgdb_roundup_cpus()

[Doug Anderson]

Hi,

On Fri, Dec 7, 2018 at 9:42 AM Catalin Marinas  wrote:
>
> On Tue, Dec 04, 2018 at 07:38:24PM -0800, Douglas Anderson wrote:
> > Douglas Anderson (4):
> >   kgdb: Remove irq flags from roundup
> >   kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()
> >   kgdb: Don't round up a CPU that failed rounding up before
> >   kdb: Don't back trace on a cpu that didn't round up
>
> FWIW, trying these on arm64 (ThunderX2) with CONFIG_KGDB_TESTS_ON_BOOT=y
> on top of 4.20-rc5 doesn't boot. It looks like they leave interrupts
> disabled when they shouldn't and it trips over the BUG at
> mm/vmalloc.c:1380 (called via do_fork -> copy_process).
>
> Now, I don't think these patches make things worse on arm64 since prior
> to them the kgdb boot tests on arm64 were stuck in a loop (RUN
> singlestep).

Thanks for the report!  ...actually, I'd never tried CONFIG_KGDB_TESTS
before.  ...so I tried them now:

A) chromeos-4.19 tree on qcom-sdm845 without this series: booted up OK
B) chromeos-4.19 tree on qcom-sdm845 with this series: booted up OK
C) v4.20-rc5-90-g30002dd008ed on rockchip-rk3399 (kevin) with this
series: booted up OK

Example output from B) above:

localhost ~ # dmesg | grep kgdbts
[2.139814] KGDB: Registered I/O driver kgdbts
[2.144582] kgdbts:RUN plant and detach test
[2.165333] kgdbts:RUN sw breakpoint test
[2.172990] kgdbts:RUN bad memory access test
[2.178640] kgdbts:RUN singlestep test 1000 iterations
[2.187765] kgdbts:RUN singlestep [0/1000]
[2.559596] kgdbts:RUN singlestep [100/1000]
[2.931419] kgdbts:RUN singlestep [200/1000]
[3.303474] kgdbts:RUN singlestep [300/1000]
[3.675121] kgdbts:RUN singlestep [400/1000]
[4.046867] kgdbts:RUN singlestep [500/1000]
[4.418920] kgdbts:RUN singlestep [600/1000]
[4.790824] kgdbts:RUN singlestep [700/1000]
[5.162479] kgdbts:RUN singlestep [800/1000]
[5.534103] kgdbts:RUN singlestep [900/1000]
[5.902299] kgdbts:RUN do_fork for 100 breakpoints
[8.463900] KGDB: Unregistered I/O driver kgdbts, debugger disabled

...so I guess I'm a little confused.  Either I have a different config
than you do or something is special about your machine?


NOTE: In general I've never considered "single step" as reliable in
kgdb.  I mostly use kgdb as "after the fact" crash debugging to
analyze local variables / memory / other tasks.  If it worked that'd
actually be kinda great, but at least when I started using kgdb years
ago I learned that it didn't work and stopped trying...


-Doug

Re: [PATCH v6 2/4] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()

[Doug Anderson]

Hi,

On Mon, Dec 3, 2018 at 7:57 AM Daniel Thompson
 wrote:
>
> On Tue, Nov 27, 2018 at 09:38:37AM -0800, Douglas Anderson wrote:
> > When I had lockdep turned on and dropped into kgdb I got a nice splat
> > on my system.  Specifically it hit:
> >   DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> >
> > Specifically it looked like this:
> >   sysrq: SysRq : DEBUG
> >   [ cut here ]
> >   DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> >   WARNING: CPU: 0 PID: 0 at .../kernel/locking/lockdep.c:2875 
> > lockdep_hardirqs_on+0xf0/0x160
> >   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.19.0 #27
> >   pstate: 604003c9 (nZCv DAIF +PAN -UAO)
> >   pc : lockdep_hardirqs_on+0xf0/0x160
> >   ...
> >   Call trace:
> >lockdep_hardirqs_on+0xf0/0x160
> >trace_hardirqs_on+0x188/0x1ac
> >kgdb_roundup_cpus+0x14/0x3c
> >kgdb_cpu_enter+0x53c/0x5cc
> >kgdb_handle_exception+0x180/0x1d4
> >kgdb_compiled_brk_fn+0x30/0x3c
> >brk_handler+0x134/0x178
> >do_debug_exception+0xfc/0x178
> >el1_dbg+0x18/0x78
> >kgdb_breakpoint+0x34/0x58
> >sysrq_handle_dbg+0x54/0x5c
> >__handle_sysrq+0x114/0x21c
> >handle_sysrq+0x30/0x3c
> >qcom_geni_serial_isr+0x2dc/0x30c
> >   ...
> >   ...
> >   irq event stamp: ...45
> >   hardirqs last  enabled at (...44): [...] __do_softirq+0xd8/0x4e4
> >   hardirqs last disabled at (...45): [...] el1_irq+0x74/0x130
> >   softirqs last  enabled at (...42): [...] _local_bh_enable+0x2c/0x34
> >   softirqs last disabled at (...43): [...] irq_exit+0xa8/0x100
> >   ---[ end trace adf21f830c46e638 ]---
> >
> > Looking closely at it, it seems like a really bad idea to be calling
> > local_irq_enable() in kgdb_roundup_cpus().  If nothing else that seems
> > like it could violate spinlock semantics and cause a deadlock.
> >
> > Instead, let's use a private csd alongside
> > smp_call_function_single_async() to round up the other CPUs.  Using
> > smp_call_function_single_async() doesn't require interrupts to be
> > enabled so we can remove the offending bit of code.
> >
> > In order to avoid duplicating this across all the architectures that
> > use the default kgdb_roundup_cpus(), we'll add a "weak" implementation
> > to debug_core.c.
> >
> > Looking at all the people who previously had copies of this code,
> > there were a few variants.  I've attempted to keep the variants
> > working like they used to.  Specifically:
> > * For arch/arc we passed NULL to kgdb_nmicallback() instead of
> >   get_irq_regs().
> > * For arch/mips there was a bit of extra code around
> >   kgdb_nmicallback()
> >
> > NOTE: In this patch we will still get into trouble if we try to round
> > up a CPU that failed to round up before.  We'll try to round it up
> > again and potentially hang when we try to grab the csd lock.  That's
> > not new behavior but we'll still try to do better in a future patch.
> >
> > Suggested-by: Daniel Thompson 
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > Changes in v6:
> > - Moved smp_call_function_single_async() error check to patch 3.
> >
> > Changes in v5:
> > - Add a comment about get_irq_regs().
> > - get_cpu() => raw_smp_processor_id() in kgdb_roundup_cpus().
> > - for_each_cpu() => for_each_online_cpu()
> > - Error check smp_call_function_single_async()
> >
> > Changes in v4: None
> > Changes in v3:
> > - No separate init call.
> > - Don't round up the CPU that is doing the rounding up.
> > - Add "#ifdef CONFIG_SMP" to match the rest of the file.
> > - Updated desc saying we don't solve the "failed to roundup" case.
> > - Document the ignored parameter.
> >
> > Changes in v2:
> > - Removing irq flags separated from fixing lockdep splat.
> > - Don't use smp_call_function (Daniel).
> >
> >  arch/arc/kernel/kgdb.c | 10 ++
> >  arch/arm/kernel/kgdb.c | 12 ---
> >  arch/arm64/kernel/kgdb.c   | 12 ---
> >  arch/hexagon/kernel/kgdb.c | 27 -
> >  arch/mips/kernel/kgdb.c|  9 +
> >  arch/powerpc/kernel/kgdb.c |  4 ++--
> >  arch/sh/kernel/kgdb.c  | 12 ---
>
> Please could you re-send with the arch maintainers for these platforms
> included in the Cc: of the patch description rather than just in the
> e-mail header.
>
> I'd like to make sure arch maintainers have a chance to opt out if they
> want to (it's a weak symbol so an arch could chose not to use it).
>
> Otherwise I shall start testing shortly!

OK, I did a repost of v6 with the Cc's and also the Acks I've received
so far.  There are no code changes in the repost.  Please let me know
if you have additional comments and thanks for your help.

-Doug

Re: [PATCH v5 2/4] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()

[Doug Anderson]

Hi,

On Mon, Nov 26, 2018 at 9:39 PM kbuild test robot  wrote:
>
> Hi Douglas,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on kgdb/kgdb-next]
> [also build test ERROR on v4.20-rc4 next-20181126]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Douglas-Anderson/kgdb-Fix-kgdb_roundup_cpus/20181127-115425
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jwessel/kgdb.git 
> kgdb-next
> config: sparc64-allyesconfig (attached as .config)
> compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.2.0 make.cross ARCH=sparc64
>
> Note: the 
> linux-review/Douglas-Anderson/kgdb-Fix-kgdb_roundup_cpus/20181127-115425 HEAD 
> b8d0502e65f2d7a2187baa69870146a6fbf18c9f builds fine.
>   It only hurts bisectibility.
>
> All errors (new ones prefixed by >>):
>
>kernel/debug/debug_core.c: In function 'kgdb_roundup_cpus':
> >> kernel/debug/debug_core.c:261:18: error: 'struct debuggerinfo_struct' has 
> >> no member named 'rounding_up'
>kgdb_info[cpu].rounding_up = false;
>  ^

Oops, I squashed this into the wrong patch.  It should have been in
patch #3.  Sorry about that.  I'll send out a quick v6.

Re: [PATCH v4 2/4] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()

[Doug Anderson]

Hi,

On Wed, Nov 14, 2018 at 2:07 PM Will Deacon  wrote:
>
> > +void __weak kgdb_call_nmi_hook(void *ignored)
> > +{
> > + kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
> > +}
>
> I suppose you could pass the cpu as an argument, but it doesn't really
> matter.

I probably won't change it for now if it doesn't matter...


> Also, I think there are cases where the CSD callback can run without
> having received an IPI, so we could potentially up passing NULL for the regs
> here which probably goes boom.

Hrm, good point.  This is not a new issue so I'd tend to add it to the
TODO list rather than block the series.  I'll also add a comment in
the code since I'm touching it anyway.

Interestingly enough quite a bit of things continue to work just fine
even if this is NULL.  I simulated setting this to NULL for all CPUs
and I could drop into the debugger, type "btc" to backtrace all CPUs,
attach to kgdb, etc.  ...but when I typed "cpu 1" it went boom.  So it
seems like parts of kdb use this but definitely not everything.

Also interesting is that on MIPS this is always NULL.  I have no idea
why but my patch series preserves this oddity.  Presumably if someone
was on a SMP MIPS machine and did "cpu 1" from kdb they'd go boom too.

In general kdb has a lot of crufty stuff like this in it.  We need to
work to get rid of the cruft but one step at a time I think.

I've started a kgdb-wishlist:

https://bugs.chromium.org/p/chromium/issues/list?q=label%3Akgdb-wishlist

...and this is crbug.com/908723


> > +void __weak kgdb_roundup_cpus(void)
> > +{
> > + call_single_data_t *csd;
> > + int this_cpu = get_cpu();
>
> Do you actually need to disable preemption here? afaict, irqs are already
> disabled by the kgdb core.

Ah, right.  I can just use raw_smp_processor_id().  Done.  I didn't
try to see if I could use smp_processor_id() since
kgdb_call_nmi_hook() already used raw_smp_processor_id(), but I can
dig if you wish.


> > + int cpu;
> > +
> > + for_each_cpu(cpu, cpu_online_mask) {
>
> for_each_online_cpu(cpu) ?

Done.


> I'm assuming this is serialised wrt CPU hotplug somehow?

I doubt it.  I can add it to my wishlist (crbug.com/908722) , but I
don't think it's something I'm going to try to solve right now and
it's definitely not new.  I think we need to make some sort of attempt
in kgdb_cpu_enter() to stop hotplugging, though we'd have to take into
account that we may be entering kgdb in an IRQ context so it might be
hard to grab a mutex.  We need to account for it there since that
function has code like:

> while (kgdb_do_roundup && --time_left &&
>(atomic_read(_in_kgdb) + atomic_read(_in_kgdb)) !=
>online_cpus)
> udelay(1000);

...and that would also be broken if cpus were plugging / unplugging.

In general, at least, the worst case would be that we'd either have an
extra 1 second delay entering the debugger (because we were waiting
for a CPU to respond that's been hotplugged) or we'd enter kgdb
without stopping one of the CPUs.  Neither of those is ideal but I
don't think we'd end up in too bad shape.

Oh, but actually, I guess I should probably check the error return of
smp_call_function_single_async() and if it returns an error I should
unset rounding_up...  That would make things behave slightly better
and is probably right anyway.


Overall: thank you very much for the review and the feedback.  Sorry
I'm not really fixing everything here.  My hope is to move things to a
slightly better state but I don't have time to fix everything.
Hopefully I can find some more time soon to fix more, or perhaps
someone else will.


-Doug

Re: [PATCH v2 2/2] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()

[Doug Anderson]

Hi,

On Sat, Nov 3, 2018 at 3:45 AM Daniel Thompson
 wrote:
>
> On Wed, Oct 31, 2018 at 02:41:14PM -0700, Doug Anderson wrote:
> > > As mentioned in another part of the thread we can also add robustness
> > > by skipping a cpu where csd->flags != 0 (and adding an appropriately
> > > large comment regarding why). Doing the check directly is abusing
> > > internal knowledge that smp.c normally keeps to itself so an accessor
> > > of some kind would be needed.
> >
> > Sure.  I could add smp_async_func_finished() that just looked like:
> >
> > int smp_async_func_finished(call_single_data_t *csd)
> > {
> >   return !(csd->flags & CSD_FLAG_LOCK);
> > }
> >
> > My understanding of all the mutual exclusion / memory barrier concepts
> > employed by smp.c is pretty weak, though.  I'm hoping that it's safe
> > to just access the structure and check the bit directly.
> >
> > ...but do you think adding a generic accessor like this is better than
> > just keeping track of this in kgdb directly?  I could avoid the
> > accessor by adding a "rounding_up" member to "struct
> > debuggerinfo_struct" and doing something like this in roundup:
> >
> >   /* If it didn't round up last time, don't try again */
> >   if (kgdb_info[cpu].rounding_up)
> > continue
> >
> >   kgdb_info[cpu].rounding_up = true
> >   smp_call_function_single_async(cpu, csd);
> >
> > ...and then in kgdb_nmicallback() I could just add:
> >
> >   kgdb_info[cpu].rounding_up = false
> >
> > In that case we're not adding a generic accessor to smp.c that most
> > people should never use.
>
> Whilst it is very tempting to make a sarcastic reply here ("Of course! What
> kgdb really needs is yet another set of condition variables") I can't
> because I actually agree with the proposal. I don't really want kgdb to
> be too "special" especially when it doesn't need to be.
>
> Only thing to note is that rounding_up will not be manipulated within a
> common spin lock so you might have to invest a bit of thought to make
> sure any races between the master and slave as the slave CPU clears the
> flag are benign.

OK, so I've hopefully got all this thought through and posted v3.
I've put most of my thoughts in the patch descriptions themselves so
let's continue the discussion there.

-Doug

Re: [PATCH v2 2/2] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()

[Doug Anderson]

Hi,

On Wed, Oct 31, 2018 at 11:40 AM Daniel Thompson
 wrote:
>
> On Tue, Oct 30, 2018 at 03:18:43PM -0700, Douglas Anderson wrote:
> > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> > index f3cadda45f07..9a3f952de6ed 100644
> > --- a/kernel/debug/debug_core.c
> > +++ b/kernel/debug/debug_core.c
> > @@ -55,6 +55,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -220,6 +221,39 @@ int __weak kgdb_skipexception(int exception, struct 
> > pt_regs *regs)
> >   return 0;
> >  }
> >
> > +/*
> > + * Default (weak) implementation for kgdb_roundup_cpus
> > + */
> > +
> > +static DEFINE_PER_CPU(call_single_data_t, kgdb_roundup_csd);
> > +
> > +void __weak kgdb_call_nmi_hook(void *ignored)
> > +{
> > + kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
> > +}
> > +
> > +void __weak kgdb_roundup_cpus(void)
> > +{
> > + call_single_data_t *csd;
> > + int cpu;
> > +
> > + for_each_cpu(cpu, cpu_online_mask) {
> > + csd = _cpu(kgdb_roundup_csd, cpu);
> > + smp_call_function_single_async(cpu, csd);
> > + }
>
> smp_call_function() automatically skips the calling CPU but this code does
> not. It isn't a hard bug since kgdb_nmicallback() does a re-entrancy
> check but I'd still prefer to skip the calling CPU.

I'll incorporate this into the next version.


> As mentioned in another part of the thread we can also add robustness
> by skipping a cpu where csd->flags != 0 (and adding an appropriately
> large comment regarding why). Doing the check directly is abusing
> internal knowledge that smp.c normally keeps to itself so an accessor
> of some kind would be needed.

Sure.  I could add smp_async_func_finished() that just looked like:

int smp_async_func_finished(call_single_data_t *csd)
{
  return !(csd->flags & CSD_FLAG_LOCK);
}

My understanding of all the mutual exclusion / memory barrier concepts
employed by smp.c is pretty weak, though.  I'm hoping that it's safe
to just access the structure and check the bit directly.

...but do you think adding a generic accessor like this is better than
just keeping track of this in kgdb directly?  I could avoid the
accessor by adding a "rounding_up" member to "struct
debuggerinfo_struct" and doing something like this in roundup:

  /* If it didn't round up last time, don't try again */
  if (kgdb_info[cpu].rounding_up)
continue

  kgdb_info[cpu].rounding_up = true
  smp_call_function_single_async(cpu, csd);

...and then in kgdb_nmicallback() I could just add:

  kgdb_info[cpu].rounding_up = false

In that case we're not adding a generic accessor to smp.c that most
people should never use.


I'll wait to hear back from you if you think the accessor is OK.  It
seems like it might be nice not to have to add something to smp.c just
for this one use case.


> > +}
> > +
> > +static void kgdb_generic_roundup_init(void)
> > +{
> > + call_single_data_t *csd;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + csd = _cpu(kgdb_roundup_csd, cpu);
> > + csd->func = kgdb_call_nmi_hook;
> > + }
> > +}
>
> I can't help noticing this code is very similar to kgdb_roundup_cpus. Do
> we really gain much from ahead-of-time initializing csd->func?

Oh!  Right...  At first I thought about just trying to put the "csd"
on the stack in kgdb_roundup_cpus() but then I realized that it needed
to persist past the end of kgdb_roundup_cpus().  ...and once I gave up
on the idea of putting it on the stack I decided I needed the init.

...but you're right that I don't really.  The only thing I'm initting
is the function pointer and it totally wouldn't hurt to just init that
over and over again every time kgdb_roundup_cpus() is called.

-Doug

Re: [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb

[Doug Anderson]

Hi,

On Tue, Oct 30, 2018 at 4:56 AM Daniel Thompson
 wrote:
>
> On Mon, Oct 29, 2018 at 11:07:00AM -0700, Douglas Anderson wrote:
> > Looking back, this is pretty much two series squashed that could be
> > treated indepdently.  The first is a serial series and the second is a
> > kgdb series.
>
> Indeed.
>
> I couldn't work out the link between the first 5 patches and the last 2
> until I read this...
>
> Is anything in the 01->05 patch set even related to kgdb? From the stack
> traces it looks to me like the lock dep warning would trigger for any
> sysrq. I think separating into two threads for v2 would be sensible.

Yes, sorry about the mess.  Splitting this into two series makes the
most sense.  Then I can focus more on trying to get the CCs right and
people can just get the patches that matter to them.

OK, I've sent v2 of both series out now.  Please yell if you can't
find them for whatever reason.

-Doug

-Doug

Re: [PATCH 7/7] kgdb: Remove irq flags and local_irq_enable/disable from roundup

[Doug Anderson]

Hi,

On Tue, Oct 30, 2018 at 4:46 AM Daniel Thompson
 wrote:
>
> On Mon, Oct 29, 2018 at 11:07:07AM -0700, Douglas Anderson wrote:
> > The function kgdb_roundup_cpus() was passed a parameter that was
> > documented as:
> >
> > > the flags that will be used when restoring the interrupts. There is
> > > local_irq_save() call before kgdb_roundup_cpus().
> >
> > Nobody used those flags.  Anyone who wanted to temporarily turn on
> > interrupts just did local_irq_enable() and local_irq_disable() without
> > looking at them.  So we can definitely remove the flags.
>
> On the whole I'd rather that this change...
>
>
> > Speaking of calling local_irq_enable(), it seems like a bad idea and
> > it caused a nice splat on my system with lockdep turned on.
> > Specifically it hit:
> >   DEBUG_LOCKS_WARN_ON(current->hardirq_context)
>
> ... and fixes for this this were in separate patches. They don't appear
> especially related.

Agreed that this is cleaner.  Done for v2.

-Doug

Re: [PATCH 6/7] smp: Don't yell about IRQs disabled in kgdb_roundup_cpus()

[Doug Anderson]

Daniel,

On Tue, Oct 30, 2018 at 2:42 AM Daniel Thompson
 wrote:
>
> On Mon, Oct 29, 2018 at 11:07:06AM -0700, Douglas Anderson wrote:
> > In kgdb_roundup_cpus() we've got code that looks like:
> >   local_irq_enable();
> >   smp_call_function(kgdb_call_nmi_hook, NULL, 0);
> >   local_irq_disable();
> >
> > In certain cases when we drop into kgdb (like with sysrq-g on a serial
> > console) we'll get a big yell that looks like:
> >
> >   sysrq: SysRq : DEBUG
> >   [ cut here ]
> >   DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> >   WARNING: CPU: 0 PID: 0 at .../kernel/locking/lockdep.c:2875 
> > lockdep_hardirqs_on+0xf0/0x160
> >   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.19.0 #27
> >   pstate: 604003c9 (nZCv DAIF +PAN -UAO)
> >   pc : lockdep_hardirqs_on+0xf0/0x160
> >   ...
> >   Call trace:
> >lockdep_hardirqs_on+0xf0/0x160
> >trace_hardirqs_on+0x188/0x1ac
> >kgdb_roundup_cpus+0x14/0x3c
> >kgdb_cpu_enter+0x53c/0x5cc
> >kgdb_handle_exception+0x180/0x1d4
> >kgdb_compiled_brk_fn+0x30/0x3c
> >brk_handler+0x134/0x178
> >do_debug_exception+0xfc/0x178
> >el1_dbg+0x18/0x78
> >kgdb_breakpoint+0x34/0x58
> >sysrq_handle_dbg+0x54/0x5c
> >__handle_sysrq+0x114/0x21c
> >handle_sysrq+0x30/0x3c
> >qcom_geni_serial_isr+0x2dc/0x30c
> >   ...
> >   ...
> >   irq event stamp: ...45
> >   hardirqs last  enabled at (...44): [...] __do_softirq+0xd8/0x4e4
> >   hardirqs last disabled at (...45): [...] el1_irq+0x74/0x130
> >   softirqs last  enabled at (...42): [...] _local_bh_enable+0x2c/0x34
> >   softirqs last disabled at (...43): [...] irq_exit+0xa8/0x100
> >   ---[ end trace adf21f830c46e638 ]---
> >
> > Let's add kgdb to the list of reasons not to warn in
> > smp_call_function_many().  That will allow us (in a future patch) to
> > stop calling local_irq_enable() which will get rid of the original
> > splat.
> >
> > NOTE: with this change comes the obvious question: will we start
> > deadlocking more often now when we drop into the debugger.  I can't
> > say that for sure one way or the other, but the fact that we do the
> > same logic for "oops_in_progress" makes me feel like it shouldn't
> > happen too often.  Also note that the old logic of turning on
> > interrupts temporarily wasn't exactly safe since (I presume) that
> > could have violated spin_lock_irqsave() semantics and ended up with a
> > deadlock of its own.
>
> This is part of the code to bring all the cores to a halt and since
> the other cores are still running kgdb isn't yet able to use the fact
> all the CPUs are halted to bend the rules. It is better for this code
> to play by the rules if it can.
>
> Is is possible to get the roundup functions to use a private csd
> alongside smp_call_function_single_async()? We could add a helper
> function to the debug core to avoid having to add cpu_online loops into
> every kgdb_roundup_cpus() implementaton.

Exactly the kind of helpful suggestion I was looking for.  Thank you
very much!  See v2 and hopefully it matches what you were thinking of.

-Doug

Re: Mac Mini G4 defconfig ?

[Doug Anderson]

Hi,

On Sun, Dec 17, 2017 at 2:54 AM, Mathieu Malaterre  wrote:
> On Fri, Dec 15, 2017 at 10:01 PM, Mathieu Malaterre  wrote:
>> On Fri, Dec 15, 2017 at 9:52 PM, Mathieu Malaterre  wrote:
>>> On Fri, Dec 15, 2017 at 8:50 PM, Mathieu Malaterre  wrote:
 Hi there,

 Does anyone has working defconfig for a Mac Mini G4 ?

 Here is what I tried:

 $ cat ./arch/powerpc/configs/g4_defconfig
 CONFIG_PPC_FPU=y
 CONFIG_ALTIVEC=y
 $ make ARCH=powerpc g4_defconfig
 $ make -j8 ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- V=1
 set -e; : '  CHK include/config/kernel.release'; mkdir -p
>>>
>>> That is odd. Doing a quick git bisect:
>>>
>>> $ git checkout 3298b690b21cdbe6b2ae8076d9147027f396f2b1
>>> $ make -n ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -f ./Makefile
>>> silentoldconfig V=1
>>> + make -n ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -f ./Makefile
>>> silentoldconfig V=1
>>> /bin/sh: line 0: [: -ge: unary operator expected
>>> make -f ./scripts/Makefile.build obj=scripts/basic
>>>
>>> I cannot make sense of this shell error, maybe this is unrelated but
>>> things start breaking around this commit.
>>
>> Even if I discard this shell error, the next error comes with:
>>
>> 433dc2ebe7d17dd21cba7ad5c362d37323592236 is the first bad commit
>>
>> With:
>>
>> $ make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- V=1
>> [...]
>>   powerpc-linux-gnu-gcc -Wp,-MD,kernel/.bounds.s.d  -nostdinc -isystem
>>  -I./arch/powerpc/include -I./arch/powerpc/include/generated
>> -I./include -I./arch/powerpc/include/uapi
>> -I./arch/powerpc/include/generated/uapi -I./include/uapi
>> -I./include/generated/uapi -include ./include/linux/kconfig.h
>> -D__KERNEL__ -Iarch/powerpc -Wall -Wundef -Wstrict-prototypes
>> -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar
>> -Werror-implicit-function-declaration -Wno-format-security -std=gnu89
>> -pipe -Iarch/powerpc -ffixed-r2 -mmultiple -mcpu=powerpc -Wa,-maltivec
>> -mbig-endian -fno-delete-null-pointer-checks -Wno-frame-address -O2
>> -Wno-maybe-uninitialized --param=allow-store-data-races=0
>> -DCC_HAVE_ASM_GOTO -Wframe-larger-than=1024 -fno-stack-protector
>> -Wno-unused-but-set-variable -Wno-unused-const-variable
>> -fomit-frame-pointer -fno-var-tracking-assignments
>> -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow
>> -fconserve-stack -Werror=implicit-int -Werror=strict-prototypes
>> -Werror=date-time -Werror=incompatible-pointer-types
>> -Werror=designated-init-DKBUILD_BASENAME='"bounds"'
>> -DKBUILD_MODNAME='"bounds"'  -fverbose-asm -S -o kernel/bounds.s
>> kernel/bounds.c
>> In file included from ./include/linux/page-flags.h:9:0,
>>  from kernel/bounds.c:9:
>> ./include/linux/bug.h:4:21: fatal error: asm/bug.h: No such file or directory
>>  #include 
>>  ^
>> compilation terminated.
>> Kbuild:20: recipe for target 'kernel/bounds.s' failed
>> make[1]: *** [kernel/bounds.s] Error 1
>> Makefile:1051: recipe for target 'prepare0' failed
>> make: *** [prepare0] Error 2
>>
>>
>> Comments ?
>
> Solution is:
>
> $ rm .cache.mk
>
> I guess the cache was not cleared in between my different kernel compilations.
>
> Sorry for the noise

Looks like others are hitting this too and it seems like we'll need to
come up with a solution that avoids this (or at least makes it somehow
clear that a make clean will fix it).  See the thread at
.

-Doug

[PATCH] i2c: Remove unneeded xxx_set_drvdata(..., NULL) calls

[Doug Anderson]

There is simply no reason to be manually setting the private driver
data to NULL in the remove/fail to probe cases.  This is just extra
cruft code that can be removed.

A few notes:
* Nothing relies on drvdata being set to NULL.
* The __device_release_driver() function eventually calls
  dev_set_drvdata(dev, NULL) anyway, so there's no need to do it
  twice.
* I verified that there were no cases where xxx_get_drvdata() was
  being called in these drivers and checking for / relying on the NULL
  return value.

This could be cleaned up kernel-wide but for now just take the baby
step and remove from the i2c subsystem.

Reported-by: Wolfram Sang w...@the-dreams.de
Reported-by: Stephen Warren swar...@wwwdotorg.org
Signed-off-by: Doug Anderson diand...@chromium.org
---
 drivers/i2c/busses/i2c-au1550.c | 1 -
 drivers/i2c/busses/i2c-bfin-twi.c   | 2 --
 drivers/i2c/busses/i2c-cpm.c| 2 --
 drivers/i2c/busses/i2c-davinci.c| 2 --
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 2 --
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 --
 drivers/i2c/busses/i2c-eg20t.c  | 2 --
 drivers/i2c/busses/i2c-highlander.c | 4 
 drivers/i2c/busses/i2c-i801.c   | 1 -
 drivers/i2c/busses/i2c-ibm_iic.c| 3 ---
 drivers/i2c/busses/i2c-imx.c| 1 -
 drivers/i2c/busses/i2c-intel-mid.c  | 2 --
 drivers/i2c/busses/i2c-iop3xx.c | 2 --
 drivers/i2c/busses/i2c-mpc.c| 2 --
 drivers/i2c/busses/i2c-mxs.c| 2 --
 drivers/i2c/busses/i2c-nomadik.c| 2 --
 drivers/i2c/busses/i2c-ocores.c | 1 -
 drivers/i2c/busses/i2c-octeon.c | 5 +
 drivers/i2c/busses/i2c-omap.c   | 3 ---
 drivers/i2c/busses/i2c-pca-platform.c   | 1 -
 drivers/i2c/busses/i2c-pmcmsp.c | 2 --
 drivers/i2c/busses/i2c-pnx.c| 2 --
 drivers/i2c/busses/i2c-powermac.c   | 1 -
 drivers/i2c/busses/i2c-puv3.c   | 2 --
 drivers/i2c/busses/i2c-pxa-pci.c| 2 --
 drivers/i2c/busses/i2c-pxa.c| 2 --
 drivers/i2c/busses/i2c-s6000.c  | 1 -
 drivers/i2c/busses/i2c-sh7760.c | 1 -
 drivers/i2c/busses/i2c-stu300.c | 1 -
 drivers/i2c/busses/i2c-taos-evm.c   | 2 --
 drivers/i2c/busses/i2c-versatile.c  | 2 --
 drivers/i2c/busses/i2c-xiic.c   | 2 --
 drivers/i2c/busses/i2c-xlr.c| 1 -
 drivers/i2c/busses/scx200_acb.c | 1 -
 drivers/i2c/muxes/i2c-mux-gpio.c| 1 -
 35 files changed, 1 insertion(+), 64 deletions(-)

diff --git a/drivers/i2c/busses/i2c-au1550.c b/drivers/i2c/busses/i2c-au1550.c
index b278298..b5b8923 100644
--- a/drivers/i2c/busses/i2c-au1550.c
+++ b/drivers/i2c/busses/i2c-au1550.c
@@ -376,7 +376,6 @@ static int i2c_au1550_remove(struct platform_device *pdev)
 {
struct i2c_au1550_data *priv = platform_get_drvdata(pdev);
 
-   platform_set_drvdata(pdev, NULL);
i2c_del_adapter(priv-adap);
i2c_au1550_disable(priv);
iounmap(priv-psc_base);
diff --git a/drivers/i2c/busses/i2c-bfin-twi.c 
b/drivers/i2c/busses/i2c-bfin-twi.c
index 0cf780f..05080c4 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -724,8 +724,6 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
 {
struct bfin_twi_iface *iface = platform_get_drvdata(pdev);
 
-   platform_set_drvdata(pdev, NULL);
-
i2c_del_adapter((iface-adap));
free_irq(iface-irq, iface);
peripheral_free_list((unsigned short *)pdev-dev.platform_data);
diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
index 2e79c10..3823623 100644
--- a/drivers/i2c/busses/i2c-cpm.c
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -682,7 +682,6 @@ static int cpm_i2c_probe(struct platform_device *ofdev)
 out_shut:
cpm_i2c_shutdown(cpm);
 out_free:
-   dev_set_drvdata(ofdev-dev, NULL);
kfree(cpm);
 
return result;
@@ -696,7 +695,6 @@ static int cpm_i2c_remove(struct platform_device *ofdev)
 
cpm_i2c_shutdown(cpm);
 
-   dev_set_drvdata(ofdev-dev, NULL);
kfree(cpm);
 
return 0;
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 6a0a553..7d1e590 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -755,7 +755,6 @@ err_mem_ioremap:
clk_put(dev-clk);
dev-clk = NULL;
 err_free_mem:
-   platform_set_drvdata(pdev, NULL);
put_device(pdev-dev);
kfree(dev);
 err_release_region:
@@ -771,7 +770,6 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 
i2c_davinci_cpufreq_deregister(dev);
 
-   platform_set_drvdata(pdev, NULL);
i2c_del_adapter(dev-adapter);
put_device(pdev-dev);
 
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c 
b/drivers/i2c/busses/i2c-designware-pcidrv.c
index

Re: [PATCH v2] of: Change logic to overwrite cmd_line with CONFIG_CMDLINE

[Doug Anderson]

I know this is a long-dead thread, but I was a little curious about
the motivation here.

I'm looking at trying to support CONFIG_CMDLINE_EXTEND (an ARM
Kconfig) in this function and don't know in which cases I should look
at the CONFIG_CMDLINE and in which cases I should use whatever
happened to be in data before the function was called.

Here's the definition in the KConfig:
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=arch/arm/Kconfig;h=24626b0419ee97e963e68329a8eb6769360b46ea;hb=HEAD#l1984

Which case do you have CONFIG_CMDLINE defined but not CMDLINE_FORCE?
In those cases, do you happen to have CONFIG_CMDLINE_EXTEND or
CMDLINE_FROM_BOOTLOADER defined?

Thanks much!

-Doug

---

On Mon, Sep 19, 2011 at 9:55 PM, Grant Likely grant.lik...@secretlab.ca wrote:

 On Tue, Sep 20, 2011 at 02:50:15PM +1000, Benjamin Herrenschmidt wrote:
  We used to overwrite with CONFIG_CMDLINE if we found a chosen
  node but failed to get bootargs out of it or they were empty,
  unless CONFIG_CMDLINE_FORCE is set.
 
  Instead change that to overwrite if data is non empty after
  the bootargs check. It allows arch code to have other mechanisms
  to retrieve the command line prior to parsing the device-tree.
 
  Note: CONFIG_CMDLINE_FORCE case should ideally be handled elsewhere
  as it won't work as it-is if the device-tree has no /chosen node
 
  Signed-off-by: Benjamin Herrenschmidt b...@kernel.crashing.org
  CC: devicetree-disc...@lists-ozlabs.org
  CC: Grant Likely grant.lik...@secretlab.ca

 Looks okay to me.

 Acked-by: Grant Likely grant.lik...@secretlab.ca

  ---
   drivers/of/fdt.c |    7 ++-
   1 files changed, 6 insertions(+), 1 deletions(-)
 
  v2. Use data instead of cmd_line so it works on archs like
  mips who don't pass cmd_line to that function to start with, also
  add a comment explaining the mechanism.
 
  (resent with right list address as well while at it)
 
  diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
  index 65200af..323b722 100644
  --- a/drivers/of/fdt.c
  +++ b/drivers/of/fdt.c
  @@ -681,9 +681,14 @@ int __init early_init_dt_scan_chosen(unsigned long 
  node, const char *uname,
        if (p != NULL  l  0)
                strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));
 
  +     /*
  +      * CONFIG_CMDLINE is meant to be a default in case nothing else
  +      * managed to set the command line, unless CONFIG_CMDLINE_FORCE
  +      * is set in which case we override whatever was found earlier.
  +      */
   #ifdef CONFIG_CMDLINE
   #ifndef CONFIG_CMDLINE_FORCE
  -     if (p == NULL || l == 0 || (l == 1  (*p) == 0))
  +     if (!data[0])
   #endif
                strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
   #endif /* CONFIG_CMDLINE */
  --
  1.7.4.1
 
 
 
 
 ___
 devicetree-discuss mailing list
 devicetree-disc...@lists.ozlabs.org
 https://lists.ozlabs.org/listinfo/devicetree-discuss
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev