| 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
| ||
|
Message-ID: <20130324031926.GA26545@roeck-us.net>
Date: Sat, 23 Mar 2013 20:19:26 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Ian Lartey <ian@...mlogic.co.uk>
Cc: linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-leds@...r.kernel.org,
linux-watchdog@...r.kernel.org,
devicetree-discuss@...ts.ozlabs.org, swarren@...dotorg.org,
grant.likely@...retlab.ca, broonie@...nsource.wolfsonmicro.com,
rob.h@...per.es
Subject: Re: [PATCH v10 04/12] watchdog: add Palmas Watchdog support
On Fri, Mar 22, 2013 at 02:55:14PM +0000, Ian Lartey wrote:
> From: Graeme Gregory <gg@...mlogic.co.uk>
>
> Add support for the Palmas watchdog timer which has a timeout configurable
> from 1s to 128s.
>
> Signed-off-by: Graeme Gregory <gg@...mlogic.co.uk>
> Signed-off-by: Ian Lartey <ian@...mlogic.co.uk>
> ---
> drivers/watchdog/palmas_wdt.c | 169 +++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 169 insertions(+), 0 deletions(-)
> create mode 100644 drivers/watchdog/palmas_wdt.c
>
> diff --git a/drivers/watchdog/palmas_wdt.c b/drivers/watchdog/palmas_wdt.c
> new file mode 100644
> index 0000000..da7e379
> --- /dev/null
> +++ b/drivers/watchdog/palmas_wdt.c
> @@ -0,0 +1,169 @@
> +/*
> + * Driver for Watchdog part of Palmas PMIC Chips
> + *
> + * Copyright 2011-2013 Texas Instruments Inc.
> + *
> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
> + * Author: Ian Lartey <ian@...mlogic.co.uk>
> + *
> + * Based on twl4030_wdt.c
> + *
> + * Author: Timo Kokkonen <timo.t.kokkonen at nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/mfd/palmas.h>
> +
> +struct palmas_wdt {
> + struct palmas *palmas;
> + struct watchdog_device wdt;
> + struct device *dev;
> +
> + int timer_margin;
> +};
> +
> +
One empty line would be sufficient.
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> + "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static int palmas_wdt_write(struct palmas *palmas, unsigned int data)
> +{
> + unsigned int addr;
> +
> + addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_WATCHDOG);
> +
> + return palmas_write(palmas, PALMAS_PMU_CONTROL_BASE, addr, addr);
2 x addr ? Should the second one (last parameter) be data ?
> +}
> +
> +static int palmas_wdt_enable(struct watchdog_device *wdt)
> +{
> + struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> + struct palmas *palmas = driver_data->palmas;
> +
> + return palmas_wdt_write(palmas, driver_data->timer_margin |
> + PALMAS_WATCHDOG_ENABLE);
> +}
> +
> +static int palmas_wdt_disable(struct watchdog_device *wdt)
> +{
> + struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> + struct palmas *palmas = driver_data->palmas;
> +
> + return palmas_wdt_write(palmas, driver_data->timer_margin);
Just wondering - why not just write 0 ?
> +}
> +
> +static int palmas_wdt_set_timeout(struct watchdog_device *wdt, unsigned timeout)
> +{
> + struct palmas_wdt *driver_data = watchdog_get_drvdata(wdt);
> +
> + if (timeout < 1 || timeout > 128) {
> + dev_warn(driver_data->dev,
> + "Timeout can only be in the range [1-128] seconds");
> + return -EINVAL;
> + }
The watchdog core supports limit checking. Might as well use it.
On a side note, it might be an interesting experience if you try setting
a value of 0 or larger than 128. Unless I am missing something,
driver_data->dev is never initialized.
> + driver_data->timer_margin = fls(timeout) - 1;
> + return 0;
> +}
> +
> +static const struct watchdog_info palmas_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "Palmas Watchdog",
> + .firmware_version = 0,
> +};
> +
> +static const struct watchdog_ops palmas_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = palmas_wdt_enable,
> + .stop = palmas_wdt_disable,
> + .ping = palmas_wdt_enable,
> + .set_timeout = palmas_wdt_set_timeout,
> +};
> +
> +static int palmas_wdt_probe(struct platform_device *pdev)
> +{
> + struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
> + struct palmas_wdt *driver_data;
> + struct watchdog_device *palmas_wdt;
> + int ret = 0;
> +
> + driver_data = devm_kzalloc(&pdev->dev, sizeof(*driver_data),
> + GFP_KERNEL);
> + if (!driver_data) {
> + dev_err(&pdev->dev, "Unable to alloacate watchdog device\n");
AFAIK devm_ functions already issue a message on errors, so it is unnecessary
to issue another one here.
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + driver_data->palmas = palmas;
> +
> + palmas_wdt = &driver_data->wdt;
> +
> + palmas_wdt->info = &palmas_wdt_info;
> + palmas_wdt->ops = &palmas_wdt_ops;
> + watchdog_set_nowayout(palmas_wdt, nowayout);
> + watchdog_set_drvdata(palmas_wdt, driver_data);
> +
There is no call to watchdog_init_timeout, and the timeout value is not initialized
in palmas_wdt. So the driver depends on the limit being set from user space
with WDIOC_SETTIMEOUT. But that can not happen until after it was opened
and started, meaning it is initially 0 when the watchdog is started.
Is that on purpose ? A more common default would be 60 seconds.
Also, if user space does not explicitly set the timeout but requests it
with WDIOC_GETTIMEOUT, it will get a value of 0.
> + ret = watchdog_register_device(&driver_data->wdt);
> + if (ret) {
> + platform_set_drvdata(pdev, NULL);
It is unnecessary to set the platform data to NULL. Besides, it isn't
even set yet (you only set it with dev_set_drvdata below).
> + goto err;
> + }
> +
> + dev_set_drvdata(&pdev->dev, driver_data);
Might use platform_set_drvdata.
> +
> + return 0;
> +err:
> + return ret;
Personal style, but I think this is unnecessary. Coding style examples suggest
to return directly in such cases (ie if there is no cleanup to do).
> +}
> +
> +static int palmas_wdt_remove(struct platform_device *pdev)
> +{
> + struct palmas_wdt *driver_data = dev_get_drvdata(&pdev->dev);
> +
platform_get_drvdata ?
> + watchdog_unregister_device(&driver_data->wdt);
> +
> + return 0;
> +}
> +
> +static struct of_device_id of_palmas_match_tbl[] = {
> + { .compatible = "ti,palmas-wdt", },
> + { .compatible = "ti,twl6035-wdt", },
> + { .compatible = "ti,twl6036-wdt", },
> + { .compatible = "ti,twl6037-wdt", },
> + { .compatible = "ti,tps65913-wdt", },
> + { .compatible = "ti,tps65914-wdt", },
> + { .compatible = "ti,tps80036-wdt", },
> + { /* end */ }
> +};
> +
> +static struct platform_driver palmas_wdt_driver = {
> + .probe = palmas_wdt_probe,
> + .remove = palmas_wdt_remove,
> + .driver = {
> + .owner = THIS_MODULE,
> + .of_match_table = of_palmas_match_tbl,
> + .name = "palmas-wdt",
> + },
> +};
> +
> +module_platform_driver(palmas_wdt_driver);
> +
> +MODULE_AUTHOR("Graeme Gregory <gg@...mlogic.co.uk>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:palmas-wdt");
> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
> --
> 1.7.0.4
>
> --
> 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/
>
>
>
--
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/
Powered by blists - more mailing lists