Re: [PATCH 3/4] watchdog: Split up config options

[Babu Moger]

Nick,

On 6/14/2017 10:04 PM, Nicholas Piggin wrote:
> On Wed, 14 Jun 2017 21:16:04 -0500
> Babu Moger <babu.moger@xxxxxxxxxx> wrote:
>
> > Hi Don,
> >
> > On 6/14/2017 9:09 AM, Don Zickus wrote:
> > > On Wed, Jun 14, 2017 at 02:11:18AM +1000, Nicholas Piggin wrote:
> > > > > Yeah, if you wouldn't mind.  Sorry for dragging this out, but I feel like we
> > > > > are getting close to have this defined properly which would allow us to
> > > > > split the code up correctly in the future.
> > > > How's this for a replacement patch 3? I think the Kconfig works out much
> > > > better now.
> > > Hi Nick,
> > >
> > > I think you made this much clearer, thank you!  I am good with this.
> > >
> > >
> > > Hi Babu,
> > >
> > > Can you give this patchset (and particularly this version of patch 3) a try
> > > on sparc to make sure we didn't break anything?  I believe this should
> > > resolve the start nmi watchdog on boot issue you noticed.  Thanks!
> > There is still one problem with the patch.
> >
> > # cat /proc/sys/kernel/watchdog
> > 1
> > # cat /proc/sys/kernel/nmi_watchdog
> > 0
> >
> > Problem is setting the initial value for  "nmi_watchdog"
> >
> > We need something(or similar) patch on top to address this.
> > ============================================
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index 5397c63..0105856 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -34,9 +34,13 @@
> >
> >    int __read_mostly nmi_watchdog_enabled;
> >
> > -#ifdef CONFIG_HARDLOCKUP_DETECTOR
> > +#if defined(CONFIG_HARDLOCKUP_DETECTOR) ||
> > defined(CONFIG_HAVE_NMI_WATCHDOG)
> >    unsigned long __read_mostly watchdog_enabled =
> > SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
> > +#else
> > +unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
> > +#endif
> >
> > +#ifdef CONFIG_HARDLOCKUP_DETECTOR
> >    /* boot commands */
> >    /*
> >     * Should we panic when a soft-lockup or hard-lockup occurs:
> > @@ -69,9 +73,6 @@ static int __init hardlockup_panic_setup(char *str)
> >           return 1;
> >    }
> >    __setup("nmi_watchdog=", hardlockup_panic_setup);
> > -
> > -#else
> > -unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
> >    #endif
> >
> >    #ifdef CONFIG_SOFTLOCKUP_DETECTOR
> Hmm, I guess I missed this because sparc parses nmi_watchdog=, but it
> also relies on the watchdog_enabled value.
>
> I guess I can fold your incremental patch in. I hope we could get
 Sure. Please go ahead.
> sparc quickly to adopt the complate HAVE_HARDLOCKUP_DETECTOR_ARCH soon
> afterwards though, so we only have 2 cases -- complete hardlockup
Sure. Sounds good. Will look at it later.
> detector, or the very bare minimum NMI_WATCHDOG.
>
> Thanks,
> Nick