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

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

[Matti Vaittinen]

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(+)

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 =] 

[PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module

[Matti Vaittinen]

The BD70528 watchdog module provides start/stop interface for RTC
driver because the BD70528 watchdog must be stopped when RTC time
is set. (WDG uses RTC counter and setting RTC may accidentally trigger
WDG if WDG is enabled). The BD71828 use same RTC driver as BD70528 but
don't share same WDG logic. When BD70528 is not configured a stub call
to "stop WDG" is implemented and in case when BD71828 is used, this
stub function should be called. Prevent configuring in the BD70528
watchdog when BD71828 is configured to avoid access to real WDG
functions when WDG does not exist in HW.

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

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index fd7968635e6d..40e1b4c69537 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -163,6 +163,7 @@ config SOFT_WATCHDOG_PRETIMEOUT
 config BD70528_WATCHDOG
 	tristate "ROHM BD70528 PMIC Watchdog"
 	depends on MFD_ROHM_BD70528
+	depends on MFD_ROHM_BD71828 = n
 	select WATCHDOG_CORE
 	help
 	  Support for the watchdog in the ROHM BD70528 PMIC. Watchdog trigger
-- 
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 =] 

Re: [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module

[Guenter Roeck]

On Thu, Jan 07, 2021 at 08:37:25AM +0200, Matti Vaittinen wrote:
> The BD70528 watchdog module provides start/stop interface for RTC
> driver because the BD70528 watchdog must be stopped when RTC time
> is set. (WDG uses RTC counter and setting RTC may accidentally trigger
> WDG if WDG is enabled). The BD71828 use same RTC driver as BD70528 but
> don't share same WDG logic. When BD70528 is not configured a stub call
> to "stop WDG" is implemented and in case when BD71828 is used, this
> stub function should be called. Prevent configuring in the BD70528
> watchdog when BD71828 is configured to avoid access to real WDG
> functions when WDG does not exist in HW.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

As mentioned in my response to the other patch, I think this is the
wrong solution.

Guenter

> ---
>  drivers/watchdog/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index fd7968635e6d..40e1b4c69537 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -163,6 +163,7 @@ config SOFT_WATCHDOG_PRETIMEOUT
>  config BD70528_WATCHDOG
>  	tristate "ROHM BD70528 PMIC Watchdog"
>  	depends on MFD_ROHM_BD70528
> +	depends on MFD_ROHM_BD71828 = n
>  	select WATCHDOG_CORE
>  	help
>  	  Support for the watchdog in the ROHM BD70528 PMIC. Watchdog trigger
> -- 
> 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 =] 

Re: [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module

[Matti Vaittinen]

On Thu, Jan 7, 2021 at 5:12 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Jan 07, 2021 at 08:37:25AM +0200, Matti Vaittinen wrote:
> > The BD70528 watchdog module provides start/stop interface for RTC
> > driver because the BD70528 watchdog must be stopped when RTC time
> > is set. (WDG uses RTC counter and setting RTC may accidentally trigger
> > WDG if WDG is enabled). The BD71828 use same RTC driver as BD70528 but
> > don't share same WDG logic. When BD70528 is not configured a stub call
> > to "stop WDG" is implemented and in case when BD71828 is used, this
> > stub function should be called. Prevent configuring in the BD70528
> > watchdog when BD71828 is configured to avoid access to real WDG
> > functions when WDG does not exist in HW.
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>
> As mentioned in my response to the other patch, I think this is the
> wrong solution.

I agree. Please forget this :)
Thanks for the review!

Best Regards
Matti

-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Matti Vaittinen

When you feel blue, no one sees your tears...
When your down, no one understands your struggle...
When you feel happy, no one notices your smile...
But fart just once...
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

[Guenter Roeck]

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 =] 

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

[Vaittinen, Matti]

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

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

[Vaittinen, Matti]

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