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

[Petr Mladek]

On Wed 2023-06-07 16:34:20, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 7, 2023 at 8:25 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > 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.
> >
> 
> 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

I know. My motivation was the following:

1. Kconfig is not C. My view is that it is more like a menu. There is a
   top level item. If the top level is enabled then there is a submenu
   with a more detailed selection of various variants and options.

2. The current logic is quite complicated from my POV. And it was
   even before your patchset. For example,
   HAVE_HARDLOCKUP_DETECTOR_BUDDY is defined as:

	config HAVE_HARDLOCKUP_DETECTOR_BUDDY
		bool
		depends on SMP
		default y

   One would expect that it would be enabled on SMP system.
   But the final value depends on many other variables
   which are defined using relatively complex conditions,
   especially HARDLOCKUP_DETECTOR, HAVE_HARDLOCKUP_DETECTOR_NON_ARCH,
   and HARDLOCKUP_DETECTOR_NON_ARCH.

   Understanding the logic is even more complicated because Kconfig is
   not indexed by cscope.

Important: The logic used at the end of the patchset actually
   follows the C style. It defines how the various variables
   depend on each other from top to bottom.

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

This one is actually defined in the menu-like order:

	config SOFTLOCKUP_DETECTOR

	config BOOTPARAM_SOFTLOCKUP_PANIC
		depends on SOFTLOCKUP_DETECTOR

It is because the custom option depends on the top level one.
This is exactly what I would like to achieve with HARDLOCKUP
variables in this patchset.

Best Regards,
Petr