| 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: <d37e3fea-d35e-4688-a845-02be6ea5eaa3@roeck-us.net>
Date: Fri, 9 Aug 2024 09:27:18 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Lee Jones <lee@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Andi Shyti <andi.shyti@...nel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-watchdog@...r.kernel.org,
thomas.petazzoni@...tlin.com, blake.vermeer@...sight.com
Subject: Re: [PATCH 4/5] watchdog: Congatec Board Controller watchdog timer
driver
On Fri, Aug 09, 2024 at 04:52:08PM +0200, Thomas Richard wrote:
> Add watchdog timer support for the Congatec Board Controller.
>
> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>
> ---
> drivers/watchdog/Kconfig | 10 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/cgbc_wdt.c | 217 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 228 insertions(+)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bae1d97cce89..07b711fc8bb2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1142,6 +1142,16 @@ config ALIM7101_WDT
>
> Most people will say N.
>
> +config CGBC_WDT
> + tristate "Congatec Board Controller Watchdog Timer"
> + depends on MFD_CGBC
> + select WATCHDOG_CORE
> + help
> + Enables watchdog timer support for the Congatec Board Controller.
> +
> + This driver can also be built as a module. If so, the module will be
> + called cgbc_wdt.
> +
> config EBC_C384_WDT
> tristate "WinSystems EBC-C384 Watchdog Timer"
> depends on (X86 || COMPILE_TEST) && HAS_IOPORT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b51030f035a6..5aa66ba91346 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -106,6 +106,7 @@ obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
> obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
> obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
> obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
> +obj-$(CONFIG_CGBC_WDT) += cgbc_wdt.o
> obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
> obj-$(CONFIG_EXAR_WDT) += exar_wdt.o
> obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
> diff --git a/drivers/watchdog/cgbc_wdt.c b/drivers/watchdog/cgbc_wdt.c
> new file mode 100644
> index 000000000000..9327e87b52e8
> --- /dev/null
> +++ b/drivers/watchdog/cgbc_wdt.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Congatec Board Controller watchdog driver
> + *
> + * Copyright (C) 2024 Bootlin
> + * Author: Thomas Richard <thomas.richard@...tlin.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#include <linux/mfd/cgbc.h>
> +
> +#define CGBC_WDT_CMD_TRIGGER 0x27
> +#define CGBC_WDT_CMD_INIT 0x28
> +#define CGBC_WDT_DISABLE 0x00
> +
> +#define CGBC_WDT_MODE_SINGLE_EVENT 0x02
> +
> +#define DEFAULT_TIMEOUT_SEC 30
> +#define DEFAULT_PRETIMEOUT_SEC 0
> +
> +enum action {
> + ACTION_INT = 0,
> + ACTION_SMI,
> + ACTION_RESET,
> + ACTION_BUTTON,
> +};
> +
> +static unsigned int timeout = DEFAULT_TIMEOUT_SEC;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> + "Watchdog timeout in seconds. (>=0, default="
> + __MODULE_STRING(DEFAULT_TIMEOUT_SEC) ")");
> +
> +static unsigned int pretimeout = DEFAULT_PRETIMEOUT_SEC;
> +module_param(pretimeout, uint, 0);
> +MODULE_PARM_DESC(pretimeout,
> + "Watchdog pretimeout in seconds. (>=0, default="
> + __MODULE_STRING(DEFAULT_PRETIMEOUT_SEC) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> + "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct cgbc_wdt_data {
> + struct cgbc_device_data *cgbc;
> + struct watchdog_device wdd;
> + enum action timeout_action;
> + enum action pretimeout_action;
> +};
> +
> +struct cgbc_wdt_cmd_cfg {
> + u8 cmd;
> + u8 mode;
> + u8 action;
> + u8 timeout1[3];
> + u8 timeout2[3];
> + u8 reserved[3];
> + u8 delay[3];
> +} __packed;
> +
> +static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);
static_assert() is declared in linux/build_bug.h. Please include all
necessary include files explicitly and do not depend on indirect includes.
> +
> +static int cgbc_wdt_start(struct watchdog_device *wdd)
> +{
> + struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
Unusual way to get wdt_data instead of using container_of().
Any special reason ?
> + struct cgbc_device_data *cgbc = wdt_data->cgbc;
> + unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
> + unsigned int timeout2 = wdd->pretimeout * 1000;
> + u8 action;
> +
> + struct cgbc_wdt_cmd_cfg cmd_start = {
> + .cmd = CGBC_WDT_CMD_INIT,
> + .mode = CGBC_WDT_MODE_SINGLE_EVENT,
> + .timeout1[0] = (u8)timeout1,
> + .timeout1[1] = (u8)(timeout1 >> 8),
> + .timeout1[2] = (u8)(timeout1 >> 16),
> + .timeout2[0] = (u8)timeout2,
> + .timeout2[1] = (u8)(timeout2 >> 8),
> + .timeout2[2] = (u8)(timeout2 >> 16),
> + };
> +
> + if (wdd->pretimeout) {
> + action = 2;
> + action |= wdt_data->pretimeout_action << 2;
> + action |= wdt_data->timeout_action << 4;
> + } else {
> + action = 1;
> + action |= wdt_data->timeout_action << 2;
> + }
> +
> + cmd_start.action = action;
> +
> + return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> + struct cgbc_device_data *cgbc = wdt_data->cgbc;
> + struct cgbc_wdt_cmd_cfg cmd_stop = {
> + .cmd = CGBC_WDT_CMD_INIT,
> + .mode = CGBC_WDT_DISABLE,
> + };
> +
> + return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
> +{
> + struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> + struct cgbc_device_data *cgbc = wdt_data->cgbc;
> + u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
> +
> + return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
> +}
> +
> +static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
> + unsigned int pretimeout)
> +{
> + struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +
> + wdd->pretimeout = pretimeout;
> + wdt_data->pretimeout_action = ACTION_SMI;
> +
> + if (watchdog_active(wdd))
> + return cgbc_wdt_start(wdd);
> +
> + return 0;
> +}
> +
> +static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
> + unsigned int timeout)
> +{
> + struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> +
> + if (timeout < wdd->pretimeout) {
> + dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");
That is a normal condition which does not warrant a log message.
Also see drivers/watchdog/watchdog_dev.c around line 385.
> + wdd->pretimeout = 0;
> + }
> +
> + wdd->timeout = timeout;
> + wdt_data->timeout_action = ACTION_RESET;
Both timeout_action and pretimeout_action are set statically.
What is the point of doing that instead of just using
ACTION_RESET and ACTION_SMI as needed irectly ?
> +
> + if (watchdog_active(wdd))
> + return cgbc_wdt_start(wdd);
> +
> + return 0;
> +}
> +
> +static const struct watchdog_info cgbc_wdt_info = {
> + .identity = "CGBC Watchdog",
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
> +};
> +
> +static const struct watchdog_ops cgbc_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = cgbc_wdt_start,
> + .stop = cgbc_wdt_stop,
> + .ping = cgbc_wdt_keepalive,
> + .set_timeout = cgbc_wdt_set_timeout,
> + .set_pretimeout = cgbc_wdt_set_pretimeout,
> +};
> +
> +static int cgbc_wdt_probe(struct platform_device *pdev)
> +{
> + struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct cgbc_wdt_data *wdt_data;
> + struct watchdog_device *wdd;
> + int ret;
> +
> + wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);
devm_kzalloc() is declared in linux/device.h. Again, please include all
necessary include files explicitly.
> + if (!wdt_data)
> + return -ENOMEM;
> +
> + wdt_data->cgbc = cgbc;
> + wdd = &wdt_data->wdd;
> + wdd->parent = dev;
> +
No limits ? That is unusual. Are you sure the driver accepts all
timeouts from 0 to UINT_MAX ?
> + wdd->info = &cgbc_wdt_info;
> + wdd->ops = &cgbc_wdt_ops;
> +
> + watchdog_set_drvdata(wdd, wdt_data);
> + watchdog_set_nowayout(wdd, nowayout);
> +
> + cgbc_wdt_set_timeout(wdd, timeout);
> + cgbc_wdt_set_pretimeout(wdd, pretimeout);
The more common approach would be to set default limits in wdd->{timout,pretimeout}
and only override the values if needed, ie if provided using module parameters.
That implies initializing the module parameters with 0. YOur call, though.
> +
> + platform_set_drvdata(pdev, wdt_data);
> + watchdog_stop_on_reboot(wdd);
> + watchdog_stop_on_unregister(wdd);
> +
> + ret = devm_watchdog_register_device(dev, wdd);
> + if (ret)
> + return ret;
> +
> + return 0;
Why not just
return devm_watchdog_register_device(dev, wdd);
?
> +}
> +
> +static struct platform_driver cgbc_wdt_driver = {
> + .driver = {
> + .name = "cgbc-wdt",
> + },
> + .probe = cgbc_wdt_probe,
> +};
> +
> +module_platform_driver(cgbc_wdt_driver);
> +
> +MODULE_DESCRIPTION("Congatec Board Controller Watchdog Driver");
> +MODULE_AUTHOR("Thomas Richard <thomas.richard@...tlin.com>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.39.2
>
>
Powered by blists - more mailing lists