Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20220705142444.17063-1-pboos@baylibre.com>
 <f756b2d5-56e7-6e52-2739-eca4bb33508b@baylibre.com>
User-agent: mu4e 1.8.2; emacs 27.1
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Neil Armstrong <narmstrong@baylibre.com>
Cc:     Philippe Boos <pboos@baylibre.com>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        linux-watchdog@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] watchdog: meson: keep running if already active
Date:   Tue, 05 Jul 2022 21:29:35 +0200
In-reply-to: <f756b2d5-56e7-6e52-2739-eca4bb33508b@baylibre.com>
Message-ID: <1jmtdnwd7y.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Tue 05 Jul 2022 at 16:39, Neil Armstrong <narmstrong@baylibre.com> wrote:

> Hi,
>
> On 05/07/2022 16:24, Philippe Boos wrote:
>> If the watchdog is already running (e.g.: started by bootloader) then
>> the kernel driver should keep the watchdog active but the amlogic driver
>> turns it off.
>> Let the driver fix the clock rate then restart the watchdog if it was
>> previously active.
>> Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
>
> Please drop this review tag since it was done off-list

Indeed a review was done off-list.

Reviewed-by says a review has been done. I was not aware this applied to
public reviews only. I probably missed that, would you mind pointing me
to that rule please ?

>
>> Signed-off-by: Philippe Boos <pboos@baylibre.com>
>> ---
>>   drivers/watchdog/meson_gxbb_wdt.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>> diff --git a/drivers/watchdog/meson_gxbb_wdt.c
>> b/drivers/watchdog/meson_gxbb_wdt.c
>> index 5a9ca10fbcfa..8c2c6f7f3bb5 100644
>> --- a/drivers/watchdog/meson_gxbb_wdt.c
>> +++ b/drivers/watchdog/meson_gxbb_wdt.c
>> @@ -146,6 +146,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>   	struct device *dev = &pdev->dev;
>>   	struct meson_gxbb_wdt *data;
>>   	int ret;
>> +	u32 regval;
>>     	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>   	if (!data)
>> @@ -177,6 +178,8 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>   	data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>>   	watchdog_set_drvdata(&data->wdt_dev, data);
>>   +	regval = readl(data->reg_base + GXBB_WDT_CTRL_REG); > +
>>   	/* Setup with 1ms timebase */
>>   	writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
>>   		GXBB_WDT_CTRL_EE_RESET |
>> @@ -186,6 +189,13 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>     	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>>   +	if ((regval & GXBB_WDT_CTRL_EN) != 0) {
>> +		ret = meson_gxbb_wdt_start(&data->wdt_dev);
>> +		if (ret)
>> +			return ret;
>> +		set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status);
>> +	}
>> +
>>   	watchdog_stop_on_reboot(&data->wdt_dev);
>>   	return devm_watchdog_register_device(dev, &data->wdt_dev);
>>   }
>
> I think it would be much claner to leave the watchdog enabled, and get the
> parameters
> from the registers and update the wdt_dev accordingly.

The problem is updating the time base (ie, the clock divider) while the
watchdog is running. We should not make an assumption about what the
bootloader is going to leave us.

It could be safe if we could update the divider, the timeout and ping
the watchdog with a single write. There is several registers for
that, so not possible. It is safer to update the divider with the
watchdog off.

There is indeed a small window of opportunity were a crash could happen
with the watchdog off. What is proposed here is progress compared to
what we have right now, which is waiting for userspace to come up to
start the watchdog again.

The only safe way to keep the watchdog on throughout is to keep whatever
time base was set by the bootloader. That means major rework since the whole
driver rely on a 1ms time base. I'm not sure re-using the bootloader
settings is such a great idea anyway. 

>
> Neil