[PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Uwe Kleine-König]

As pointed out in the added comment resetting the rtc also stops the
included watchdog. This is bad if the bootloader started the watchdog to
secure the boot process. So don't reset if the watchdog is running.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-stmp3xxx.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index d578e40d5a50..b76318fd5bb0 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -288,10 +288,22 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rtc_data);
 
-	err = stmp_reset_block(rtc_data->io);
-	if (err) {
-		dev_err(&pdev->dev, "stmp_reset_block failed: %d\n", err);
-		return err;
+	/*
+	 * Resetting the rtc stops the watchdog timer that is potentially
+	 * running. So (assuming it is running on purpose) don't reset if the
+	 * watchdog is enabled.
+	 */
+	if (readl(rtc_data->io + STMP3XXX_RTC_CTRL) &
+	    STMP3XXX_RTC_CTRL_WATCHDOGEN) {
+		dev_info(&pdev->dev,
+			 "Watchdog is running, skip resetting rtc\n");
+	} else {
+		err = stmp_reset_block(rtc_data->io);
+		if (err) {
+			dev_err(&pdev->dev, "stmp_reset_block failed: %d\n",
+				err);
+			return err;
+		}
 	}
 
 	/*
-- 
2.17.1

Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Uwe Kleine-König]

Hello,

On Sat, Jun 09, 2018 at 08:43:13AM +0200, Uwe Kleine-König wrote:
> As pointed out in the added comment resetting the rtc also stops the
> included watchdog. This is bad if the bootloader started the watchdog to
> secure the boot process. So don't reset if the watchdog is running.

I didn't get any feedback for this patch yet. Assuming my patch is
considered ok, my expectation would be that the watchdog people ack it
and that then it goes in via the rtc tree.

Best regards
Uwe

> diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
> index d578e40d5a50..b76318fd5bb0 100644
> --- a/drivers/rtc/rtc-stmp3xxx.c
> +++ b/drivers/rtc/rtc-stmp3xxx.c
> @@ -288,10 +288,22 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, rtc_data);
>  
> -	err = stmp_reset_block(rtc_data->io);
> -	if (err) {
> -		dev_err(&pdev->dev, "stmp_reset_block failed: %d\n", err);
> -		return err;
> +	/*
> +	 * Resetting the rtc stops the watchdog timer that is potentially
> +	 * running. So (assuming it is running on purpose) don't reset if the
> +	 * watchdog is enabled.
> +	 */
> +	if (readl(rtc_data->io + STMP3XXX_RTC_CTRL) &
> +	    STMP3XXX_RTC_CTRL_WATCHDOGEN) {
> +		dev_info(&pdev->dev,
> +			 "Watchdog is running, skip resetting rtc\n");
> +	} else {
> +		err = stmp_reset_block(rtc_data->io);
> +		if (err) {
> +			dev_err(&pdev->dev, "stmp_reset_block failed: %d\n",
> +				err);
> +			return err;
> +		}
>  	}
>  
>  	/*

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Guenter Roeck]

On Mon, Jul 09, 2018 at 10:30:41AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Sat, Jun 09, 2018 at 08:43:13AM +0200, Uwe Kleine-König wrote:
> > As pointed out in the added comment resetting the rtc also stops the
> > included watchdog. This is bad if the bootloader started the watchdog to
> > secure the boot process. So don't reset if the watchdog is running.
> 
> I didn't get any feedback for this patch yet. Assuming my patch is
> considered ok, my expectation would be that the watchdog people ack it
> and that then it goes in via the rtc tree.
> 

Guess there was a good reason for not passing an accessor via regmap ?

Either case, stmp_reset_block() is also called from resume, meaning the
watchdog, if running, will likely be disabled by a suspend/resume cycle.

I have no idea why the rtc block is reset in the first place;
one would assume that there was a reason for that.

The associated watchdog driver does not support telling the watchdog
core that the watchdog is running. As such, a watchdog running at boot
will only be handled if the watchdog daemon is started before the
watchdog times out. I assume this is understood and acceptable.

Given all that, and taking no responsibility for the actual impact
of the changes, I can only confirm that the patch looks syntactically
correct.

Acked-by: Guenter Roeck <linux@roeck-us.net>

I would suggest, however, that it might make more sense to have someone
who knows the chip and the systems using it confirm that the change
is appropriate and will not cause any problems.

Thanks,
Guenter

> Best regards
> Uwe
> 
> > diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
> > index d578e40d5a50..b76318fd5bb0 100644
> > --- a/drivers/rtc/rtc-stmp3xxx.c
> > +++ b/drivers/rtc/rtc-stmp3xxx.c
> > @@ -288,10 +288,22 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, rtc_data);
> >  
> > -	err = stmp_reset_block(rtc_data->io);
> > -	if (err) {
> > -		dev_err(&pdev->dev, "stmp_reset_block failed: %d\n", err);
> > -		return err;
> > +	/*
> > +	 * Resetting the rtc stops the watchdog timer that is potentially
> > +	 * running. So (assuming it is running on purpose) don't reset if the
> > +	 * watchdog is enabled.
> > +	 */
> > +	if (readl(rtc_data->io + STMP3XXX_RTC_CTRL) &
> > +	    STMP3XXX_RTC_CTRL_WATCHDOGEN) {
> > +		dev_info(&pdev->dev,
> > +			 "Watchdog is running, skip resetting rtc\n");
> > +	} else {
> > +		err = stmp_reset_block(rtc_data->io);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "stmp_reset_block failed: %d\n",
> > +				err);
> > +			return err;
> > +		}
> >  	}
> >  
> >  	/*
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Uwe Kleine-König]

Hello,

adding Wolfram to Cc who authored the watchdog bits in the rtc driver.

On Mon, Jul 09, 2018 at 09:19:24AM -0700, Guenter Roeck wrote:
> On Mon, Jul 09, 2018 at 10:30:41AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Sat, Jun 09, 2018 at 08:43:13AM +0200, Uwe Kleine-König wrote:
> > > As pointed out in the added comment resetting the rtc also stops the
> > > included watchdog. This is bad if the bootloader started the watchdog to
> > > secure the boot process. So don't reset if the watchdog is running.
> > 
> > I didn't get any feedback for this patch yet. Assuming my patch is
> > considered ok, my expectation would be that the watchdog people ack it
> > and that then it goes in via the rtc tree.
> > 
> 
> Guess there was a good reason for not passing an accessor via regmap ?

I don't know the details, but maybe this concept wasn't picked because
just passing a single function for setting the timeout was considered
simpler. Also given that the registers for wdg and rtc are not separated
cleanly giving the wdg driver a regmap isn't "nice" IMHO.

Last time I worked on this driver I wondered if it would be cleaner to
merge both drivers into drivers/rtc as the relevant functionality
(stmp3xxx_wdt_set_timeout()) is in that driver anyhow and
drivers/watchdog/stmp3xxx_rtc_wdt.c mostly contains stuff to handle the
separation. Oh, and there is a reboot notifier that looks strange where
I wonder if it is really needed.

> Either case, stmp_reset_block() is also called from resume, meaning the
> watchdog, if running, will likely be disabled by a suspend/resume cycle.

Yeah, that's right. The watchdog driver has the corresponding callbacks
and restarts the timer on resume though.

> I have no idea why the rtc block is reset in the first place;
> one would assume that there was a reason for that.

The usual reason is to ensure a clean hardware state. I wouldn't expect
a deeper reason here. (And if there is one, and removing it really
breaks something, the right way forward is to reintroduce the reset with
an explaining comment and think about how to fix the watchdog issue I
fixed in another way.)

> The associated watchdog driver does not support telling the watchdog
> core that the watchdog is running. As such, a watchdog running at boot
> will only be handled if the watchdog daemon is started before the
> watchdog times out. I assume this is understood and acceptable.

Yeah, that's the use case we have.

> Given all that, and taking no responsibility for the actual impact
> of the changes, I can only confirm that the patch looks syntactically
> correct.
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

Hi,

> adding Wolfram to Cc who authored the watchdog bits in the rtc driver.

Yeah, but this was really long ago. I don't recall many details. It
could be that the only option back then was an MFD driver which seemed
overkill to me. We have more options these days (syscon?). Maybe a merge
makes sense as well. Do as you see fit, I don't mind.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Guenter Roeck]

On 07/10/2018 12:06 AM, Uwe Kleine-König wrote:
> Hello,
> 
> adding Wolfram to Cc who authored the watchdog bits in the rtc driver.
> 
> On Mon, Jul 09, 2018 at 09:19:24AM -0700, Guenter Roeck wrote:
>> On Mon, Jul 09, 2018 at 10:30:41AM +0200, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Sat, Jun 09, 2018 at 08:43:13AM +0200, Uwe Kleine-König wrote:
>>>> As pointed out in the added comment resetting the rtc also stops the
>>>> included watchdog. This is bad if the bootloader started the watchdog to
>>>> secure the boot process. So don't reset if the watchdog is running.
>>>
>>> I didn't get any feedback for this patch yet. Assuming my patch is
>>> considered ok, my expectation would be that the watchdog people ack it
>>> and that then it goes in via the rtc tree.
>>>
>>
>> Guess there was a good reason for not passing an accessor via regmap ?
> 
> I don't know the details, but maybe this concept wasn't picked because
> just passing a single function for setting the timeout was considered
> simpler. Also given that the registers for wdg and rtc are not separated
> cleanly giving the wdg driver a regmap isn't "nice" IMHO.
> 
> Last time I worked on this driver I wondered if it would be cleaner to
> merge both drivers into drivers/rtc as the relevant functionality
> (stmp3xxx_wdt_set_timeout()) is in that driver anyhow and
> drivers/watchdog/stmp3xxx_rtc_wdt.c mostly contains stuff to handle the

I would agree on this one. It is an example where separation doesn't really
add any value.

> separation. Oh, and there is a reboot notifier that looks strange where
> I wonder if it is really needed.
> 

Calling watchdog_stop_on_reboot() would indeed have been more straightforward,
but that may not have existed when the driver was implemented.

>> Either case, stmp_reset_block() is also called from resume, meaning the
>> watchdog, if running, will likely be disabled by a suspend/resume cycle.
> 
> Yeah, that's right. The watchdog driver has the corresponding callbacks
> and restarts the timer on resume though.
> 
.. assuming the watchdog driver's resume call happens after the rtc driver's
resume call. Maybe that is guaranteed in a parent/child situation.

Guenter

>> I have no idea why the rtc block is reset in the first place;
>> one would assume that there was a reason for that.
> 
> The usual reason is to ensure a clean hardware state. I wouldn't expect
> a deeper reason here. (And if there is one, and removing it really
> breaks something, the right way forward is to reintroduce the reset with
> an explaining comment and think about how to fix the watchdog issue I
> fixed in another way.)
> 
>> The associated watchdog driver does not support telling the watchdog
>> core that the watchdog is running. As such, a watchdog running at boot
>> will only be handled if the watchdog daemon is started before the
>> watchdog times out. I assume this is understood and acceptable.
> 
> Yeah, that's the use case we have.
> 
>> Given all that, and taking no responsibility for the actual impact
>> of the changes, I can only confirm that the patch looks syntactically
>> correct.
>>
>> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks
> Uwe
> 

Re: [PATCH] rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running

[Alexandre Belloni]

On 09/07/2018 10:30:41+0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Sat, Jun 09, 2018 at 08:43:13AM +0200, Uwe Kleine-König wrote:
> > As pointed out in the added comment resetting the rtc also stops the
> > included watchdog. This is bad if the bootloader started the watchdog to
> > secure the boot process. So don't reset if the watchdog is running.
> 
> I didn't get any feedback for this patch yet. Assuming my patch is
> considered ok, my expectation would be that the watchdog people ack it
> and that then it goes in via the rtc tree.
> 

I actually applied it a while ago and forgot to say so. I've included
Guenter's ack now.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com