[PATCH 0/2] watchdog: imgpdc: Runtime fixes

[PATCH 0/2] watchdog: imgpdc: Runtime fixes

[James Hogan]

A couple of basic runtime fixes for the IMG PDC watchdog driver. These
were found by attempting to enable the driver for TZ1090 and running
under Meta QEMU. Note that QEMU doesn't emulate the actual watchdog
functionality beyond the soft reset register, so I cannot claim to have
tested the driver's watchdog timer functionality yet.

James Hogan (2):
  watchdog: imgpdc: Fix probe NULL pointer dereference
  watchdog: imgpdc: Fix default heartbeat

 drivers/watchdog/imgpdc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Cc: Jude Abraham <Jude.Abraham@imgtec.com>
Cc: linux-watchdog@vger.kernel.org
-- 
2.0.5

[PATCH 1/2] watchdog: imgpdc: Fix probe NULL pointer dereference

[James Hogan]

The IMG PDC watchdog probe function calls pdc_wdt_stop() prior to
watchdog_set_drvdata(), causing a NULL pointer dereference when
pdc_wdt_stop() retrieves the struct pdc_wdt_dev pointer using
watchdog_get_drvdata() and reads the register base address through it.

Fix by moving the watchdog_set_drvdata() call earlier, to where various
other pdc_wdt->wdt_dev fields are initialised.

Fixes: 93937669e9b5 ("watchdog: ImgTec PDC Watchdog Timer Driver")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Cc: Jude Abraham <Jude.Abraham@imgtec.com>
Cc: linux-watchdog@vger.kernel.org
---
 drivers/watchdog/imgpdc_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index c8def68..32c35eb 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -191,6 +191,7 @@ static int pdc_wdt_probe(struct platform_device *pdev)
 	pdc_wdt->wdt_dev.ops = &pdc_wdt_ops;
 	pdc_wdt->wdt_dev.max_timeout = 1 << PDC_WDT_CONFIG_DELAY_MASK;
 	pdc_wdt->wdt_dev.parent = &pdev->dev;
+	watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
 
 	ret = watchdog_init_timeout(&pdc_wdt->wdt_dev, heartbeat, &pdev->dev);
 	if (ret < 0) {
@@ -232,7 +233,6 @@ static int pdc_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(&pdc_wdt->wdt_dev, nowayout);
 
 	platform_set_drvdata(pdev, pdc_wdt);
-	watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
 
 	ret = watchdog_register_device(&pdc_wdt->wdt_dev);
 	if (ret)
-- 
2.0.5

Re: [PATCH 1/2] watchdog: imgpdc: Fix probe NULL pointer dereference

[Guenter Roeck]

On Fri, Feb 20, 2015 at 11:45:44PM +0000, James Hogan wrote:
> The IMG PDC watchdog probe function calls pdc_wdt_stop() prior to
> watchdog_set_drvdata(), causing a NULL pointer dereference when
> pdc_wdt_stop() retrieves the struct pdc_wdt_dev pointer using
> watchdog_get_drvdata() and reads the register base address through it.
> 
> Fix by moving the watchdog_set_drvdata() call earlier, to where various
> other pdc_wdt->wdt_dev fields are initialised.
> 
> Fixes: 93937669e9b5 ("watchdog: ImgTec PDC Watchdog Timer Driver")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> Cc: Jude Abraham <Jude.Abraham@imgtec.com>
> Cc: linux-watchdog@vger.kernel.org

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

[PATCH 2/2] watchdog: imgpdc: Fix default heartbeat

[James Hogan]

The IMG PDC watchdog driver heartbeat module parameter has no default so
it is initialised to zero. This results in the following warning during
probe:

imgpdc-wdt 2006000.wdt: Initial timeout out of range! setting max timeout

The module parameter description implies that the default value should
be PDC_WDT_DEF_TIMEOUT, which isn't yet used, so initialise it to that.

Also tweak the heartbeat module parameter description for consistency.

Fixes: 93937669e9b5 ("watchdog: ImgTec PDC Watchdog Timer Driver")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Cc: Jude Abraham <Jude.Abraham@imgtec.com>
Cc: linux-watchdog@vger.kernel.org
---
 drivers/watchdog/imgpdc_wdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index 32c35eb..0deaa4f 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -42,10 +42,10 @@
 #define PDC_WDT_MIN_TIMEOUT		1
 #define PDC_WDT_DEF_TIMEOUT		64
 
-static int heartbeat;
+static int heartbeat = PDC_WDT_DEF_TIMEOUT;
 module_param(heartbeat, int, 0);
-MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. "
-	"(default = " __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")");
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds "
+	"(default=" __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")");
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
-- 
2.0.5

Re: [PATCH 2/2] watchdog: imgpdc: Fix default heartbeat

[Guenter Roeck]

On Fri, Feb 20, 2015 at 11:45:45PM +0000, James Hogan wrote:
> The IMG PDC watchdog driver heartbeat module parameter has no default so
> it is initialised to zero. This results in the following warning during
> probe:
> 
> imgpdc-wdt 2006000.wdt: Initial timeout out of range! setting max timeout
> 
> The module parameter description implies that the default value should
> be PDC_WDT_DEF_TIMEOUT, which isn't yet used, so initialise it to that.
> 
> Also tweak the heartbeat module parameter description for consistency.
> 
> Fixes: 93937669e9b5 ("watchdog: ImgTec PDC Watchdog Timer Driver")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> Cc: Jude Abraham <Jude.Abraham@imgtec.com>
> Cc: linux-watchdog@vger.kernel.org

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

Re: [PATCH 0/2] watchdog: imgpdc: Runtime fixes

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

Hi,

On Fri, Feb 20, 2015 at 11:45:43PM +0000, James Hogan wrote:
> A couple of basic runtime fixes for the IMG PDC watchdog driver. These
> were found by attempting to enable the driver for TZ1090 and running
> under Meta QEMU. Note that QEMU doesn't emulate the actual watchdog
> functionality beyond the soft reset register, so I cannot claim to have
> tested the driver's watchdog timer functionality yet.
> 
> James Hogan (2):
>   watchdog: imgpdc: Fix probe NULL pointer dereference
>   watchdog: imgpdc: Fix default heartbeat

Thanks Guenter for the reviews.

Any chance of these fixes getting merged in time for v4.0?

FWIW I've now tried the driver on real TZ1090 hardware (with these
patches) and it appears to work as expected.

Cheers
James

> 
>  drivers/watchdog/imgpdc_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> Cc: Jude Abraham <Jude.Abraham@imgtec.com>
> Cc: linux-watchdog@vger.kernel.org
> -- 
> 2.0.5
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] watchdog: imgpdc: Runtime fixes

[Guenter Roeck]

On 03/20/2015 01:53 PM, James Hogan wrote:
> Hi,
>
> On Fri, Feb 20, 2015 at 11:45:43PM +0000, James Hogan wrote:
>> A couple of basic runtime fixes for the IMG PDC watchdog driver. These
>> were found by attempting to enable the driver for TZ1090 and running
>> under Meta QEMU. Note that QEMU doesn't emulate the actual watchdog
>> functionality beyond the soft reset register, so I cannot claim to have
>> tested the driver's watchdog timer functionality yet.
>>
>> James Hogan (2):
>>    watchdog: imgpdc: Fix probe NULL pointer dereference
>>    watchdog: imgpdc: Fix default heartbeat
>
> Thanks Guenter for the reviews.
>
> Any chance of these fixes getting merged in time for v4.0?
>

I agree that would be good. There is also 'watchdog: mtk_wdt:
signedness bug in mtk_wdt_start()' which should be in 4.0 as well.

Wim, if you are busy, want me to prepare and send a pull request to Linus ?

Thanks,
Guenter

Re: [PATCH 0/2] watchdog: imgpdc: Runtime fixes

[Wim Van Sebroeck]

Hi All,

> >>A couple of basic runtime fixes for the IMG PDC watchdog driver. These
> >>were found by attempting to enable the driver for TZ1090 and running
> >>under Meta QEMU. Note that QEMU doesn't emulate the actual watchdog
> >>functionality beyond the soft reset register, so I cannot claim to have
> >>tested the driver's watchdog timer functionality yet.
> >>
> >>James Hogan (2):
> >>   watchdog: imgpdc: Fix probe NULL pointer dereference
> >>   watchdog: imgpdc: Fix default heartbeat
> >
> >Thanks Guenter for the reviews.
> >
> >Any chance of these fixes getting merged in time for v4.0?
> >
> 
> I agree that would be good. There is also 'watchdog: mtk_wdt:
> signedness bug in mtk_wdt_start()' which should be in 4.0 as well.

Correct.

> Wim, if you are busy, want me to prepare and send a pull request to Linus ?

I'm preparing the 3 patches so that they can go upstream relatively soon.

Kind regards,
Wim.

Re: [PATCH 0/2] watchdog: imgpdc: Runtime fixes

[Wim Van Sebroeck]

Hi James,

> A couple of basic runtime fixes for the IMG PDC watchdog driver. These
> were found by attempting to enable the driver for TZ1090 and running
> under Meta QEMU. Note that QEMU doesn't emulate the actual watchdog
> functionality beyond the soft reset register, so I cannot claim to have
> tested the driver's watchdog timer functionality yet.
> 
> James Hogan (2):
>   watchdog: imgpdc: Fix probe NULL pointer dereference
>   watchdog: imgpdc: Fix default heartbeat
> 
>  drivers/watchdog/imgpdc_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> Cc: Jude Abraham <Jude.Abraham@imgtec.com>
> Cc: linux-watchdog@vger.kernel.org

Both patches added to linux-watchdog-next. Will go upstream in 2 days.

Kind regards,
Wim.

RE: [PATCH 0/2] watchdog: imgpdc: Runtime fixes

[Naidu Tellapati]

Hi Wim & James,

> Hi James,

>> A couple of basic runtime fixes for the IMG PDC watchdog driver. These
>> were found by attempting to enable the driver for TZ1090 and running
>> under Meta QEMU. Note that QEMU doesn't emulate the actual watchdog
>> functionality beyond the soft reset register, so I cannot claim to have
>> tested the driver's watchdog timer functionality yet.
>>
>> James Hogan (2):
>>   watchdog: imgpdc: Fix probe NULL pointer dereference
>>   watchdog: imgpdc: Fix default heartbeat
>>
>>  drivers/watchdog/imgpdc_wdt.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> Cc: Wim Van Sebroeck <wim@iguana.be>
>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>> Cc: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>> Cc: Jude Abraham <Jude.Abraham@imgtec.com>
>> Cc: linux-watchdog@vger.kernel.org

> Both patches added to linux-watchdog-next. Will go upstream in 2 days.

Many thanks for your help in this regard.

> Kind regards,
> Wim.

Regards,
Naidu.