| Message ID | 20180609064313.18763-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: stmp3xxx: Don't reset the rtc in .probe() when watchdog is running | expand |
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; > + } > } > > /*
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/ |
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
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
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 >
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.
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; + } } /*
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(-)