Received-SPF: pass (google.com: domain of linux-kernel+bounces-45843-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 31 Jan 2024 12:21:33 +0530
From: Pavan Kondeti <quic_pkondeti@quicinc.com>
To: Guenter Roeck <linux@roeck-us.net>
CC: Pavan Kondeti <quic_pkondeti@quicinc.com>,
        Bjorn Andersson
	<andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        "Wim Van
 Sebroeck" <wim@linux-watchdog.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-watchdog@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, Mukesh Ojha
	<quic_mojha@quicinc.com>
Subject: Re: [PATCH] watchdog: qcom: Start the watchdog in probe
Message-ID: <22141056-bc6b-48bf-a4af-405f747aaa29@quicinc.com>
References: <20240131-qcom-wdt-start-probe-v1-1-bee0a86e2bba@quicinc.com>
 <1a996038-bcc7-4c0f-8f27-ca36a2eb9d3d@roeck-us.net>
 <cfaaed23-8f59-4447-af0b-b94b35ce68ba@quicinc.com>
 <a908d04b-8068-4831-87ef-44175250c226@roeck-us.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <a908d04b-8068-4831-87ef-44175250c226@roeck-us.net>

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