watchdog: fixes for menz069_wdt driver
Subsystems (2)
Johannes Thumshirn
maintainercover watchdog: fixes for menz069_wdt driver
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(-)
Signature · 1 line
2.39.2
Johannes Thumshirn
maintainerPATCH 1/2 watchdog: menz069_wdt: fix watchdog initialisation
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.
--- drivers/watchdog/menz69_wdt.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/watchdog/menz69_wdt.c +6 -10
@@ -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);
Signature · 1 line
2.39.2
Re: [PATCH 1/2] watchdog: menz069_wdt: fix watchdog initialisation
Quoted 9 lines · Johannes Thumshirn
Doing a 'cat
/dev/watchdog0' withmenz069_wdtas 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 staticwatchdog_devicestructure and use the one embedded into the driver's per-instance private data.Signed-off-by Johannes Thumshirn <[email protected]>
Quoted 45 lines
--- drivers/watchdog/menz69_wdt.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)drivers/watchdog/menz69_wdt.c +6
-10@@ -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);
Johannes Thumshirn
maintainerPATCH 2/2 watchdog: menz069_wdt: fix timeout setting
In reply to Johannes ThumshirnWhen 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.
--- drivers/watchdog/menz69_wdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/menz69_wdt.c +1 -1
@@ -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 );
Signature · 1 line
2.39.2
Re: [PATCH 2/2] watchdog: menz069_wdt: fix timeout setting
Quoted 7 lines · Johannes Thumshirn
When setting the timeout for the
menz069_wdtwatchdog 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 <[email protected]>
Quoted 17 lines
--- drivers/watchdog/menz69_wdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/watchdog/menz69_wdt.c +1
-1@@ -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 );