[PATCH v1] watchdog: meson: keep running if already active

[PATCH v1] watchdog: meson: keep running if already active

[Philippe Boos]

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>
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);
 }
-- 
2.20.1

Re: [PATCH v1] watchdog: meson: keep running if already active

[Neil Armstrong]

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

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

Neil

Re: [PATCH v1] watchdog: meson: keep running if already active

[Jerome Brunet]

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

Re: [PATCH v1] watchdog: meson: keep running if already active

[Guenter Roeck]

On Tue, Jul 05, 2022 at 09:29:35PM +0200, Jerome Brunet wrote:
> 
> 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 ?
> 

Public or not doesn't really matter. However, you can only apply a
Reviewed-by: tag (or any tag, really) if you explicitly received one.
The exchange seems to suggest that you did not receive that tag.
Please never add any tags on your own.

On the other side, if the reviewer did send a Reviewed-by: tag off list,
I would kindly ask the reviewer to not do that in the future to avoid
misunderstandings. If you don't want your Reviewed-by: tag attached to
a patch, don't send one. Not everyone will even realize that you sent
your tag off-list, and no one can be expected to know that you didn't
really mean it when you sent your tag.

Thanks,
Guenter

Re: [PATCH v1] watchdog: meson: keep running if already active

[Jerome Brunet]

On Wed 06 Jul 2022 at 05:41, Guenter Roeck <linux@roeck-us.net> wrote:

> On Tue, Jul 05, 2022 at 09:29:35PM +0200, Jerome Brunet wrote:
>> 
>> 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 ?
>> 
>
> Public or not doesn't really matter. However, you can only apply a
> Reviewed-by: tag (or any tag, really) if you explicitly received one.
> The exchange seems to suggest that you did not receive that tag.

Philippe did receive that Reviewed-by. I gave it to him.
Doing his first public patches, he first requested a review off-list to
try to get things right and not bother people too much (so much for that :/)

> Please never add any tags on your own.

He did not.

>
> On the other side, if the reviewer did send a Reviewed-by: tag off list,
> I would kindly ask the reviewer to not do that in the future to avoid
> misunderstandings.

No worries. That being said, I have gone over 

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst

It just says that Reviewed-by sent on the list should be collected for
the following version. Nothing against adding the tag if the job has
been done, on or off list. Same goes for Suggested-by, Tested-by, etc.

If I missed something or it is non-written rule, please let me know.

> If you don't want your Reviewed-by: tag attached to
> a patch, don't send one. Not everyone will even realize that you sent
> your tag off-list, and no one can be expected to know that you didn't
> really mean it when you sent your tag.

I do want Philippe to add it. I would not have given it the first place
if it was not the case.

>
> Thanks,
> Guenter

Re: [PATCH v1] watchdog: meson: keep running if already active

[Guenter Roeck]

On Wed, Jul 06, 2022 at 03:24:27PM +0200, Jerome Brunet wrote:
> 
[ ... ]

> 
> No worries. That being said, I have gone over 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
> 
> It just says that Reviewed-by sent on the list should be collected for
> the following version. Nothing against adding the tag if the job has
> been done, on or off list. Same goes for Suggested-by, Tested-by, etc.
> 
> If I missed something or it is non-written rule, please let me know.

Your interpretation is quite a strict one. I don't think there is a rule
that states that tags not sent to a list must not be collected.

Anyway, I would have called it common sense, especially since it does
happen that someone accidentally hits "reply" instead of "reply all"
and replies end up not being sent to the list. If you expect me to dig
through e-mail headers to determine if you meant your tags to be published
or not, sorry, that won't happen. Do not send me e-mails with any tags
unless you accept that they may be published.

Guenter

Re: [PATCH v1] watchdog: meson: keep running if already active

[Jerome Brunet]

On Wed 06 Jul 2022 at 09:43, Guenter Roeck <linux@roeck-us.net> wrote:

> On Wed, Jul 06, 2022 at 03:24:27PM +0200, Jerome Brunet wrote:
>> 
> [ ... ]
>
>> 
>> No worries. That being said, I have gone over 
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
>> 
>> It just says that Reviewed-by sent on the list should be collected for
>> the following version. Nothing against adding the tag if the job has
>> been done, on or off list. Same goes for Suggested-by, Tested-by, etc.
>> 
>> If I missed something or it is non-written rule, please let me know.
>
> Your interpretation is quite a strict one. I don't think there is a rule
> that states that tags not sent to a list must not be collected.
>
> Anyway, I would have called it common sense, especially since it does
> happen that someone accidentally hits "reply" instead of "reply all"
> and replies end up not being sent to the list. If you expect me to dig
> through e-mail headers to determine if you meant your tags to be published
> or not, sorry, that won't happen. Do not send me e-mails with any tags
> unless you accept that they may be published.
>
> Guenter

Exactly. Not expecting anything. I'm 100% aligned with you.
Maybe this is where is misunderstanding is.

The Reviewed-by was there when Philippe sent his mail.
I was meant to be there and taken. Plain and simple.
No other expectation whatsoever.

I'm not asking for the tag to dropped, quite the opposite actually.