Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828
@ 2021-01-07  6:37 Matti Vaittinen
  2021-01-07  6:37 ` [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module Matti Vaittinen
  2021-01-07 15:12 ` [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Guenter Roeck
  0 siblings, 2 replies; 7+ messages in thread
From: Matti Vaittinen @ 2021-01-07  6:37 UTC (permalink / raw)
  To: matti.vaittinen, mazziesaccount
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-power, linux-watchdog,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module
  2021-01-07  6:37 [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Matti Vaittinen
@ 2021-01-07  6:37 ` Matti Vaittinen
  2021-01-07 15:12   ` Guenter Roeck
  2021-01-07 15:12 ` [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Guenter Roeck
  1 sibling, 1 reply; 7+ messages in thread
From: Matti Vaittinen @ 2021-01-07  6:37 UTC (permalink / raw)
  To: matti.vaittinen, mazziesaccount
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-power, linux-watchdog,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828
  2021-01-07  6:37 [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Matti Vaittinen
  2021-01-07  6:37 ` [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module Matti Vaittinen
@ 2021-01-07 15:12 ` Guenter Roeck
  2021-01-07 18:15   ` Vaittinen, Matti
  1 sibling, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2021-01-07 15:12 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: mazziesaccount, Wim Van Sebroeck, linux-power, linux-watchdog,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module
  2021-01-07  6:37 ` [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module Matti Vaittinen
@ 2021-01-07 15:12   ` Guenter Roeck
  2021-01-08  6:34     ` Matti Vaittinen
  0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2021-01-07 15:12 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: mazziesaccount, Wim Van Sebroeck, linux-power, linux-watchdog,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828
  2021-01-07 15:12 ` [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Guenter Roeck
@ 2021-01-07 18:15   ` Vaittinen, Matti
  2021-01-08  6:10     ` Vaittinen, Matti
  0 siblings, 1 reply; 7+ messages in thread
From: Vaittinen, Matti @ 2021-01-07 18:15 UTC (permalink / raw)
  To: linux; +Cc: mazziesaccount, wim, linux-power, linux-kernel, linux-watchdog

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828
  2021-01-07 18:15   ` Vaittinen, Matti
@ 2021-01-08  6:10     ` Vaittinen, Matti
  0 siblings, 0 replies; 7+ messages in thread
From: Vaittinen, Matti @ 2021-01-08  6:10 UTC (permalink / raw)
  To: linux; +Cc: mazziesaccount, wim, linux-power, linux-kernel, linux-watchdog


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module
  2021-01-07 15:12   ` Guenter Roeck
@ 2021-01-08  6:34     ` Matti Vaittinen
  0 siblings, 0 replies; 7+ messages in thread
From: Matti Vaittinen @ 2021-01-08  6:34 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Matti Vaittinen, Wim Van Sebroeck, linux-power, LINUXWATCHDOG,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-07  6:37 [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Matti Vaittinen
2021-01-07  6:37 ` [PATCH 2/2] watchdog: BD70528: conditionally allow BD70528 module Matti Vaittinen
2021-01-07 15:12   ` Guenter Roeck
2021-01-08  6:34     ` Matti Vaittinen
2021-01-07 15:12 ` [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828 Guenter Roeck
2021-01-07 18:15   ` Vaittinen, Matti
2021-01-08  6:10     ` Vaittinen, Matti

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git