Re: [PATCH] watchdog: qcom: Start the watchdog in probe

[Guenter Roeck]

On 1/30/24 22:16, Pavan Kondeti wrote:
> On Tue, Jan 30, 2024 at 10:01:15PM -0800, Guenter Roeck wrote:
> > On 1/30/24 20:15, Pavankumar Kondeti wrote:
> > > When CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled, kernel can pet the
> > > watchdog until user space takes over. Make use of this feature and
> > > start the watchdog in probe.
> > >
> > > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@xxxxxxxxxxx>
> > > ---
> > >    drivers/watchdog/qcom-wdt.c | 6 +++++-
> > >    1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > > index 9e790f0c2096..4fb5dbf5faee 100644
> > > --- a/drivers/watchdog/qcom-wdt.c
> > > +++ b/drivers/watchdog/qcom-wdt.c
> > > @@ -276,12 +276,16 @@ static int qcom_wdt_probe(struct platform_device *pdev)
> > >    	watchdog_init_timeout(&wdt->wdd, 0, dev);
> > >    	/*
> > > +	 * Kernel can pet the watchdog until user space takes over.
> > > +	 * Start the watchdog here to make use of this feature.
> > > +	
> >
> > No, that is not what CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is about.
> > Please see its description.
> >
> > NACK.
> Thanks for taking a look Guenter. I thought of using
> WATCHDOG_HANDLE_BOOT_ENABLED and infiniite open timeout as a hint to start
> the watchdog in probe. After seeing your NACK for this patch, I guess
> that would also would have been rejected.
WATCHDOG_HANDLE_BOOT_ENABLED is not supposed to be used in drivers.
It is a flag for the watchdog core. Before you bring it up, stm32_iwdg.c
is a corner case because (presumably) the driver can not determine
if the watchdog is running.

> Do you think we can revive this [1] to add support for watchdog pet from
> the kernel? It would be helpful in cases where the user space has no
> support for watchdog pet (say minimal ramdisk).

If done properly, sure. Looking at the exchange, the patch still had issues
which I don't think were ever resolved.

Personally I would not want to rely on this, though. It won't address situations
where userspace hangs but low level kernel interrupts still work. I think
it is mostly useful to cover the time from loading the watchdog driver
to starting the watchdog daemon, but even that would better be solved by
starting the watchdog in the ROM monitor or BIOS. A minimal watchdog daemon
would not consume that much memory, so I don't think "user space has no
support for watchdog pet (say minimal ramdisk)" would ever be a real problem.
Such a minimal system would probably (hopefully) be based on busybox which
does support a watchdog.

Guenter