Re: [PATCH] PM: sleep: Allow disabling DPM watchdog by default

[Tomasz Figa]

On Tue, Jun 2, 2026 at 11:09 AM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:
>
> On Mon, Jun 01, 2026 at 08:39:42PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 28, 2026 at 12:32 PM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:
> > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > > index e1b550664bab..4f92905f3edf 100644
> > > --- a/drivers/base/power/main.c
> > > +++ b/drivers/base/power/main.c
> > > @@ -527,6 +527,20 @@ module_param(dpm_watchdog_all_cpu_backtrace, bool, 0644);
> > >  MODULE_PARM_DESC(dpm_watchdog_all_cpu_backtrace,
> > >                  "Backtrace all CPUs on DPM watchdog timeout");
> > >
> > > +#ifdef CONFIG_DPM_WATCHDOG_DEFAULT_ENABLED
> > > +static unsigned int __read_mostly dpm_watchdog_enabled = 1;
> > > +#else
> > > +static unsigned int __read_mostly dpm_watchdog_enabled;
> > > +#endif
> > > +
> > > +static int __init dpm_watchdog_setup(char *str)
> > > +{
> > > +       if (kstrtouint(str, 0, &dpm_watchdog_enabled) == 0)
> > > +               return 1;
> > > +       return 0;
> > > +}
> > > +__setup("dpm_watchdog_enabled=", dpm_watchdog_setup);
> >
> > You might as well use a module parameter to allow this to be set or
> > clear at run time.  Is there a particular reason why you only want it
> > to be enabled or disabled via the kernel command line?
>
> Thanks for the suggestion.  Mainly because in our use cases, we only need
> to set it once at boot time.
>
> Also, I was wondering if we need to consider potential races if the flag
> can be set at runtime.  E.g.:
> 1) The flag is set.
> 2) dpm_watchdog_set() is called and the timer is started.
> 3) The flag is then unset.
> 4) The subsequent dpm_watchdog_clear() isn't stop the timer.
>
> Given this, would you still suggest providing the module parameter for
> completeness?

Would that lead to anything bad, though? If I'm remembering correctly,
would that mean that any already ongoing PM operations would still be
subject to the timer, but not any new ones.

My suggestion would also be to actually make the timeout and warning
timeout configurable at runtime.

Best,
Tomasz

>
> >
> > > +
> > >  /**
> > >   * dpm_watchdog_handler - Driver suspend / resume watchdog handler.
> > >   * @t: The timer that PM watchdog depends on.
> > > @@ -570,6 +584,9 @@ static void dpm_watchdog_set(struct dpm_watchdog *wd, struct device *dev)
> > >  {
> > >         struct timer_list *timer = &wd->timer;
> > >
> > > +       if (!dpm_watchdog_enabled)
> > > +               return;
> > > +
> > >         wd->dev = dev;
> > >         wd->tsk = current;
> > >         wd->fatal = CONFIG_DPM_WATCHDOG_TIMEOUT == CONFIG_DPM_WATCHDOG_WARNING_TIMEOUT;
> > > @@ -588,6 +605,9 @@ static void dpm_watchdog_clear(struct dpm_watchdog *wd)
> > >  {
> > >         struct timer_list *timer = &wd->timer;
> > >
> > > +       if (!dpm_watchdog_enabled)
> > > +               return;
> > > +
> > >         timer_delete_sync(timer);
> > >         timer_destroy_on_stack(timer);
> > >  }