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

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

[Pavankumar Kondeti]

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@quicinc.com>
---
 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.
+	 *
 	 * If WDT is already running, call WDT start which
 	 * will stop the WDT, set timeouts as bootloader
 	 * might use different ones and set running bit
 	 * to inform the WDT subsystem to ping the WDT
 	 */
-	if (qcom_wdt_is_running(&wdt->wdd)) {
+	if (IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED) ||
+	    qcom_wdt_is_running(&wdt->wdd)) {
 		qcom_wdt_start(&wdt->wdd);
 		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
 	}

---
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
change-id: 20240131-qcom-wdt-start-probe-b8e0560aef7d

Best regards,
-- 
Pavankumar Kondeti <quic_pkondeti@quicinc.com>

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

[Guenter Roeck]

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@quicinc.com>
> ---
>   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.

Guenter

>   	 * If WDT is already running, call WDT start which
>   	 * will stop the WDT, set timeouts as bootloader
>   	 * might use different ones and set running bit
>   	 * to inform the WDT subsystem to ping the WDT

>   	 */
> -	if (qcom_wdt_is_running(&wdt->wdd)) {
> +	if (IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED) ||
> +	    qcom_wdt_is_running(&wdt->wdd)) {
>   		qcom_wdt_start(&wdt->wdd);
>   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>   	}
> 
> ---
> base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
> change-id: 20240131-qcom-wdt-start-probe-b8e0560aef7d
> 
> Best regards,

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

[Pavan Kondeti]

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@quicinc.com>
> > ---
> >   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.

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).

[1]
https://lore.kernel.org/linux-watchdog/20210924133509.3454834-1-f.suligoi@asem.it/#t

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@quicinc.com>
>>> ---
>>>    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

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

[Pavan Kondeti]

On Tue, Jan 30, 2024 at 10:37:50PM -0800, Guenter Roeck wrote:
> 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@quicinc.com>
> > > > ---
> > > >    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.

Thanks. I will take a look at your review feedback on the series and
address them before sending the next revision.

> 
> 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.
> 

Got it. I will find ways to start the watchdog in firmware so that we
don't need anything special handling.

Thanks,
Pavan