linux-kernel - RE: [PATCH V1 5/6] watchdog: da9062: DA9062 watchdog driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 8 May 2015 13:46:42 +0000
From:	"Opensource [Steve Twiss]" <stwiss.opensource@...semi.com>
To:	Guenter Roeck <linux@...ck-us.net>,
	LINUXKERNEL <linux-kernel@...r.kernel.org>,
	LINUXWATCHDOG <linux-watchdog@...r.kernel.org>,
	Wim Van Sebroeck <wim@...ana.be>
CC:	Alessandro Zummo <a.zummo@...ertech.it>,
	DEVICETREE <devicetree@...r.kernel.org>,
	David Dajun Chen <david.chen@...semi.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	LINUXINPUT <linux-input@...r.kernel.org>,
	Lee Jones <lee.jones@...aro.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"Mark Rutland" <mark.rutland@....com>,
	Pawel Moll <pawel.moll@....com>,
	RTCLINUX <rtc-linux@...glegroups.com>,
	Rob Herring <robh+dt@...nel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [PATCH V1 5/6] watchdog: da9062: DA9062 watchdog driver

On 18 April 2015 16:53 Guenter Roeck wrote:

Hi Guenter,

There were some missing explanations in my previous e-mails for some of 
your comments. Please find them below.

During the da9062_wdt_probe() there were several ignored error paths and
a missing cleanup . I intend to rectify this with the following:

--- a/linux-next/v4.0/drivers/watchdog/da9062_wdt.c
+++ b/linux-next/v4.0/drivers/watchdog/da9062_wdt.c
@@ -232,8 +232,11 @@ static int da9062_wdt_probe(struct platform_device *pdev)
        dev_set_drvdata(&pdev->dev, wdt);
 
        irq = platform_get_irq_byname(pdev, "WDG_WARN");
-       if (irq < 0)
+       if (irq < 0) {
                dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
+               ret = irq;
+               goto error;
+       }
 
        ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
                           da9062_wdt_wdg_warn_irq_handler,
@@ -242,20 +245,23 @@ static int da9062_wdt_probe(struct platform_device *pdev)
        if (ret) {
                dev_err(wdt->hw->dev,
                        "Failed to request watchdog device IRQ.\n");
+               goto error;
        }
 
        ret = watchdog_register_device(&wdt->wdtdev);
-       if (ret < 0)
+       if (ret < 0) {
                dev_err(wdt->hw->dev,
                        "watchdog registration incomplete (%d)\n", ret);
+               goto error;
+       }
 
        da9062_set_window_start(wdt);
 
        ret = da9062_wdt_ping(&wdt->wdtdev);
        if (ret < 0)
-               dev_err(wdt->hw->dev,
-                       "failed to ping the watchdog (%d)\n", ret);
+               watchdog_unregister_device(&wdt->wdtdev);
 
+error:
        return ret;
 }

Also there was an explanation required for the confusing ping() function
inside the driver probe() ...

> > +	da9062_wdt_ping(&wdt->wdtdev);
[...]
> That ping is asking for an explanation. Does it imply that the watchdog is
> known to be running and cannot be stopped ?

Pinging the DA9062 watchdog has no effect if it is disabled. But I guessed that
in a real application if the watchdog -was- enabled from start-up then the first
thing the driver needed to do was kick the watchdog.

I could have put protection around it, say by reading the TWDSCALE bit for 
whether the watchdog was enabled, and that would have made it more explicit
but in this case it wouldn't matter.

This can be removed if you prefer.

Regards,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/