diff mbox series

[1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828

Message ID 671ac57ad53ab1614da7fe9a3d0f78bdb5b51fda.1610001365.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive)
State Changes Requested
Headers show
Series [1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 | expand

Commit Message

Vaittinen, Matti Jan. 7, 2021, 6:37 a.m. UTC 
If config for BD70528 watchdog is enabled when BD71828 or BD71815
are used the RTC module will issue call to BD70528 watchdog with
NULL data. Ignore this call and don't crash.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/watchdog/bd70528_wdt.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)


base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442

Comments

Guenter Roeck Jan. 7, 2021, 3:12 p.m. UTC | #1 
On Thu, Jan 07, 2021 at 08:37:03AM +0200, Matti Vaittinen wrote:
> If config for BD70528 watchdog is enabled when BD71828 or BD71815
> are used the RTC module will issue call to BD70528 watchdog with
> NULL data. Ignore this call and don't crash.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

I really think this should be handled in the calling code.
Also, I am curious how this is supposed to work.

The code is called with

	ret = bd70528_wdt_set(r->parent, new_state & BD70528_WDT_STATE_BIT,
                              old_state);

from bd70528_set_rtc_based_timers(). That same function subsequently
calls bd70528_set_elapsed_tmr() with the same parameter, and that
parameter is dereferenced in bd70528_set_elapsed_tmr() without checking.

Conceptually, it should not be necessary to determine at compile-time
which of the chips is in the system. It should be posible to compile
a single kernel which supports all chips.

Guenter

> ---
>  drivers/watchdog/bd70528_wdt.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/watchdog/bd70528_wdt.c b/drivers/watchdog/bd70528_wdt.c
> index 0170b37e6674..fde242b8a4a6 100644
> --- a/drivers/watchdog/bd70528_wdt.c
> +++ b/drivers/watchdog/bd70528_wdt.c
> @@ -49,6 +49,21 @@ int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
>  	u8 wd_ctrl_arr[3] = { WD_CTRL_MAGIC1, WD_CTRL_MAGIC2, 0 };
>  	u8 *wd_ctrl = &wd_ctrl_arr[2];
>  
> +	/*
> +	 * BD71828 and BD71815 use same RTC driver as BD70528.
> +	 * BD71815 and BD71828 do not need MFD data as they do not share
> +	 * RTC counter with watchdog. The BD70528 watchdog should not be
> +	 * compiled in with BD71815 or BD71828 and the stub implementation
> +	 * for the bd70528_wdt_set should be provided instead.
> +	 *
> +	 * If one compiles this watchdog with BD71828 or BD71815 - the call
> +	 * from RTC may get here and the data pointer is NULL. In that case,
> +	 * warn and go out.
> +	 */
> +	if (!data) {
> +		pr_warn("BD70528_WATCHDOG misconfigured\n");
> +		return 0;
> +	}
>  	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &tmp);
>  	if (ret)
>  		return ret;
> 
> base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442
> -- 
> 2.25.4
> 
> 
> -- 
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]
Vaittinen, Matti Jan. 7, 2021, 6:15 p.m. UTC | #2 
Thanks a lot for taking a careful look at this Guenter!

On Thu, 2021-01-07 at 07:12 -0800, Guenter Roeck wrote:
> On Thu, Jan 07, 2021 at 08:37:03AM +0200, Matti Vaittinen wrote:
> > If config for BD70528 watchdog is enabled when BD71828 or BD71815
> > are used the RTC module will issue call to BD70528 watchdog with
> > NULL data. Ignore this call and don't crash.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 
> I really think this should be handled in the calling code.
> Also, I am curious how this is supposed to work.
> 
> The code is called with
> 
> 	ret = bd70528_wdt_set(r->parent, new_state &
> BD70528_WDT_STATE_BIT,
>                               old_state);

My brainfart.
The bd70528_wdt_set is not called as it is protected in RTC by
has_rtc_timers flag.

I inserted this check in wrong function. The bd70528_wdt_lock()
is where we may hit the problem as it is not protected.
> 
> from bd70528_set_rtc_based_timers(). That same function subsequently
> calls bd70528_set_elapsed_tmr() with the same parameter, and that
> parameter is dereferenced in bd70528_set_elapsed_tmr() without
> checking.
> 
> Conceptually, it should not be necessary to determine at compile-time
> which of the chips is in the system. It should be posible to compile
> a single kernel which supports all chips.

I agree. The information whether WDT should be accessed should be
judged by dt-compatible. MFD has this knowledge and passes it to RTC.
So yes, RTC should omit the call if BD70528 is not used. Please ignore
these patches, I'll do changes to RTC driver :)


Best Regards
Matti
Vaittinen, Matti Jan. 8, 2021, 6:10 a.m. UTC | #3 
On Thu, 2021-01-07 at 20:15 +0200, Matti Vaittinen wrote:
> Thanks a lot for taking a careful look at this Guenter!
> 
> On Thu, 2021-01-07 at 07:12 -0800, Guenter Roeck wrote:
> > On Thu, Jan 07, 2021 at 08:37:03AM +0200, Matti Vaittinen wrote:
> > > If config for BD70528 watchdog is enabled when BD71828 or BD71815
> > > are used the RTC module will issue call to BD70528 watchdog with
> > > NULL data. Ignore this call and don't crash.
> > > 
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com
> > > >
> > 
> > I really think this should be handled in the calling code.
> > Also, I am curious how this is supposed to work.
> > 
> > The code is called with
> > 
> > 	ret = bd70528_wdt_set(r->parent, new_state &
> > BD70528_WDT_STATE_BIT,
> >                               old_state);
> 
> My brainfart.
> The bd70528_wdt_set is not called as it is protected in RTC by
> has_rtc_timers flag.
> 
> I inserted this check in wrong function. The bd70528_wdt_lock()
> is where we may hit the problem as it is not protected.

Actually, after a fresh look - it seems the bd70528_wdt_lock() is also
just fine. The RTC is not grabbing the lock on other PMICs but the
BD70528. I'm not really sure what I have been thinking of. @_@ I
should've been more careful. Thanks for spotting this in the review!

Best Regards
	Matti
diff mbox series

Patch

diff --git a/drivers/watchdog/bd70528_wdt.c b/drivers/watchdog/bd70528_wdt.c
index 0170b37e6674..fde242b8a4a6 100644
--- a/drivers/watchdog/bd70528_wdt.c
+++ b/drivers/watchdog/bd70528_wdt.c
@@ -49,6 +49,21 @@  int bd70528_wdt_set(struct rohm_regmap_dev *data, int enable, int *old_state)
 	u8 wd_ctrl_arr[3] = { WD_CTRL_MAGIC1, WD_CTRL_MAGIC2, 0 };
 	u8 *wd_ctrl = &wd_ctrl_arr[2];
 
+	/*
+	 * BD71828 and BD71815 use same RTC driver as BD70528.
+	 * BD71815 and BD71828 do not need MFD data as they do not share
+	 * RTC counter with watchdog. The BD70528 watchdog should not be
+	 * compiled in with BD71815 or BD71828 and the stub implementation
+	 * for the bd70528_wdt_set should be provided instead.
+	 *
+	 * If one compiles this watchdog with BD71828 or BD71815 - the call
+	 * from RTC may get here and the data pointer is NULL. In that case,
+	 * warn and go out.
+	 */
+	if (!data) {
+		pr_warn("BD70528_WATCHDOG misconfigured\n");
+		return 0;
+	}
 	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &tmp);
 	if (ret)
 		return ret;