[PATCH 0/2] watchdog: fixes for menz069_wdt driver

[PATCH 0/2] watchdog: fixes for menz069_wdt driver

[Johannes Thumshirn]

While testing the Qemu emulation of the menz069_wdt watchdog core, I've found
two issues in the kernel driver.

On is a NULL pointer dereference, because the driver can't access it's
private data and one is a wrong register read resulting in incorrect timeout
values being set.

Johannes Thumshirn (2):
  watchdog: menz069_wdt: fix watchdog initialisation
  watchdog: menz069_wdt: fix timeout setting

 drivers/watchdog/menz69_wdt.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

-- 
2.39.2

[PATCH 1/2] watchdog: menz069_wdt: fix watchdog initialisation

[Johannes Thumshirn]

Doing a 'cat /dev/watchdog0' with menz069_wdt as watchdog0 will result in
a NULL pointer dereference.

This happens because we're passing the wrong pointer to
watchdog_register_device(). Fix this by getting rid of the static
watchdog_device structure and use the one embedded into the driver's
per-instance private data.

Signed-off-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/watchdog/menz69_wdt.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
index 8973f98bc6a5..bca0938f3429 100644
--- a/drivers/watchdog/menz69_wdt.c
+++ b/drivers/watchdog/menz69_wdt.c
@@ -98,14 +98,6 @@ static const struct watchdog_ops men_z069_ops = {
 	.set_timeout = men_z069_wdt_set_timeout,
 };
 
-static struct watchdog_device men_z069_wdt = {
-	.info = &men_z069_info,
-	.ops = &men_z069_ops,
-	.timeout = MEN_Z069_DEFAULT_TIMEOUT,
-	.min_timeout = 1,
-	.max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ,
-};
-
 static int men_z069_probe(struct mcb_device *dev,
 			  const struct mcb_device_id *id)
 {
@@ -125,15 +117,19 @@ static int men_z069_probe(struct mcb_device *dev,
 		goto release_mem;
 
 	drv->mem = mem;
+	drv->wdt.info = &men_z069_info;
+	drv->wdt.ops = &men_z069_ops;
+	drv->wdt.timeout = MEN_Z069_DEFAULT_TIMEOUT;
+	drv->wdt.min_timeout = 1;
+	drv->wdt.max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ;
 
-	drv->wdt = men_z069_wdt;
 	watchdog_init_timeout(&drv->wdt, 0, &dev->dev);
 	watchdog_set_nowayout(&drv->wdt, nowayout);
 	watchdog_set_drvdata(&drv->wdt, drv);
 	drv->wdt.parent = &dev->dev;
 	mcb_set_drvdata(dev, drv);
 
-	return watchdog_register_device(&men_z069_wdt);
+	return watchdog_register_device(&drv->wdt);
 
 release_mem:
 	mcb_release_mem(mem);
-- 
2.39.2

Re: [PATCH 1/2] watchdog: menz069_wdt: fix watchdog initialisation

[Guenter Roeck]

On 4/18/23 10:25, Johannes Thumshirn wrote:
> Doing a 'cat /dev/watchdog0' with menz069_wdt as watchdog0 will result in
> a NULL pointer dereference.
> 
> This happens because we're passing the wrong pointer to
> watchdog_register_device(). Fix this by getting rid of the static
> watchdog_device structure and use the one embedded into the driver's
> per-instance private data.
> 
> Signed-off-by: Johannes Thumshirn <jth@kernel.org>

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

> ---
>   drivers/watchdog/menz69_wdt.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
> index 8973f98bc6a5..bca0938f3429 100644
> --- a/drivers/watchdog/menz69_wdt.c
> +++ b/drivers/watchdog/menz69_wdt.c
> @@ -98,14 +98,6 @@ static const struct watchdog_ops men_z069_ops = {
>   	.set_timeout = men_z069_wdt_set_timeout,
>   };
>   
> -static struct watchdog_device men_z069_wdt = {
> -	.info = &men_z069_info,
> -	.ops = &men_z069_ops,
> -	.timeout = MEN_Z069_DEFAULT_TIMEOUT,
> -	.min_timeout = 1,
> -	.max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ,
> -};
> -
>   static int men_z069_probe(struct mcb_device *dev,
>   			  const struct mcb_device_id *id)
>   {
> @@ -125,15 +117,19 @@ static int men_z069_probe(struct mcb_device *dev,
>   		goto release_mem;
>   
>   	drv->mem = mem;
> +	drv->wdt.info = &men_z069_info;
> +	drv->wdt.ops = &men_z069_ops;
> +	drv->wdt.timeout = MEN_Z069_DEFAULT_TIMEOUT;
> +	drv->wdt.min_timeout = 1;
> +	drv->wdt.max_timeout = MEN_Z069_WDT_COUNTER_MAX / MEN_Z069_TIMER_FREQ;
>   
> -	drv->wdt = men_z069_wdt;
>   	watchdog_init_timeout(&drv->wdt, 0, &dev->dev);
>   	watchdog_set_nowayout(&drv->wdt, nowayout);
>   	watchdog_set_drvdata(&drv->wdt, drv);
>   	drv->wdt.parent = &dev->dev;
>   	mcb_set_drvdata(dev, drv);
>   
> -	return watchdog_register_device(&men_z069_wdt);
> +	return watchdog_register_device(&drv->wdt);
>   
>   release_mem:
>   	mcb_release_mem(mem);

[PATCH 2/2] watchdog: menz069_wdt: fix timeout setting

[Johannes Thumshirn]

When setting the timeout for the menz069_wdt watchdog driver, we
erroneously read from the 'watchdog value register' (WVR) instead of the
'watchdog timer register' (WTR) and then write the value back into WTR.

This can potentially lead to wrong timeouts and wrong enable bit settings.

Signed-off-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/watchdog/menz69_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
index bca0938f3429..3c98030b9fcd 100644
--- a/drivers/watchdog/menz69_wdt.c
+++ b/drivers/watchdog/menz69_wdt.c
@@ -77,7 +77,7 @@ static int men_z069_wdt_set_timeout(struct watchdog_device *wdt,
 	wdt->timeout = timeout;
 	val = timeout * MEN_Z069_TIMER_FREQ;
 
-	reg = readw(drv->base + MEN_Z069_WVR);
+	reg = readw(drv->base + MEN_Z069_WTR);
 	ena = reg & MEN_Z069_WTR_WDEN;
 	reg = ena | val;
 	writew(reg, drv->base + MEN_Z069_WTR);
-- 
2.39.2

Re: [PATCH 2/2] watchdog: menz069_wdt: fix timeout setting

[Guenter Roeck]

On 4/18/23 10:25, Johannes Thumshirn wrote:
> When setting the timeout for the menz069_wdt watchdog driver, we
> erroneously read from the 'watchdog value register' (WVR) instead of the
> 'watchdog timer register' (WTR) and then write the value back into WTR.
> 
> This can potentially lead to wrong timeouts and wrong enable bit settings.
> 
> Signed-off-by: Johannes Thumshirn <jth@kernel.org>

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

> ---
>   drivers/watchdog/menz69_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/menz69_wdt.c b/drivers/watchdog/menz69_wdt.c
> index bca0938f3429..3c98030b9fcd 100644
> --- a/drivers/watchdog/menz69_wdt.c
> +++ b/drivers/watchdog/menz69_wdt.c
> @@ -77,7 +77,7 @@ static int men_z069_wdt_set_timeout(struct watchdog_device *wdt,
>   	wdt->timeout = timeout;
>   	val = timeout * MEN_Z069_TIMER_FREQ;
>   
> -	reg = readw(drv->base + MEN_Z069_WVR);
> +	reg = readw(drv->base + MEN_Z069_WTR);
>   	ena = reg & MEN_Z069_WTR_WDEN;
>   	reg = ena | val;
>   	writew(reg, drv->base + MEN_Z069_WTR);