| 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: <8e2cecbe-aa48-4e84-93cc-8c028c5e649e@roeck-us.net>
Date: Fri, 6 Sep 2024 04:52:06 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Junhao Xie <bigfoot@...ssfun.cn>, devicetree@...r.kernel.org,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-leds@...r.kernel.org, linux-pm@...r.kernel.org,
linux-rtc@...r.kernel.org, linux-watchdog@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Pavel Machek <pavel@....cz>,
Lee Jones <lee@...nel.org>, Sebastian Reichel <sre@...nel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Wim Van Sebroeck <wim@...ux-watchdog.org>, Heiko Stuebner <heiko@...ech.de>,
Chukun Pan <amadeus@....edu.cn>
Subject: Re: [PATCH 3/9] watchdog: Add Photonicat PMU watchdog driver
On 9/6/24 02:36, Junhao Xie wrote:
> This driver provides access to Photonicat PMU watchdog functionality.
>
> Signed-off-by: Junhao Xie <bigfoot@...ssfun.cn>
> ---
> drivers/watchdog/Kconfig | 12 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/photonicat-wdt.c | 124 ++++++++++++++++++++++++++++++
> 3 files changed, 137 insertions(+)
> create mode 100644 drivers/watchdog/photonicat-wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bae1d97cce89..4094216a1c09 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -300,6 +300,18 @@ config MENZ069_WATCHDOG
> This driver can also be built as a module. If so the module
> will be called menz069_wdt.
>
> +config PHOTONICAT_PMU_WDT
> + tristate "Photonicat PMU Watchdog"
> + depends on MFD_PHOTONICAT_PMU
> + select WATCHDOG_CORE
> + help
> + This driver provides access to Photonicat PMU watchdog functionality.
> +
> + Say Y here to include support for the Photonicat PMU Watchdog.
> +
> + This driver can also be built as a module. If so the module
> + will be called photonicat-wdt.
> +
> config WDAT_WDT
> tristate "ACPI Watchdog Action Table (WDAT)"
> depends on ACPI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b51030f035a6..14375af84039 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -234,6 +234,7 @@ obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o
> obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
> obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
> obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
> +obj-$(CONFIG_PHOTONICAT_PMU_WDT) += photonicat-wdt.o
> obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
> obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
> obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> diff --git a/drivers/watchdog/photonicat-wdt.c b/drivers/watchdog/photonicat-wdt.c
> new file mode 100644
> index 000000000000..1e758fcfb49f
> --- /dev/null
> +++ b/drivers/watchdog/photonicat-wdt.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Junhao Xie <bigfoot@...ssfun.cn>
> + */
> +
> +#include <linux/mfd/photonicat-pmu.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +struct pcat_watchdog {
> + struct device *dev;
I don't see what this is used for.
> + struct pcat_pmu *pmu;
> + struct watchdog_device wdd;
> + u8 timeout;
> + bool started;
> +};
> +
> +static const struct watchdog_info pcat_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "Photonicat PMU Watchdog",
> +};
> +
> +static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout)
> +{
> + int ret;
> + u8 time = 0;
Unnecessary initialization.
> + u8 times[3] = { 60, 60, 0 };
> +
> + time = MIN(255, MAX(0, timeout));
> +
> + ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET,
> + times, sizeof(times));
Where does this actually send the timeout to the chip ?
> + if (!ret)
> + data->started = timeout != 0;
> +
> + return ret;
> +}
> +
> +static int pcat_wdt_start(struct watchdog_device *wdev)
> +{
> + struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> + return pcat_wdt_setup(data, data->timeout);
> +}
> +
> +static int pcat_wdt_stop(struct watchdog_device *wdev)
> +{
> + struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> + return pcat_wdt_setup(data, 0);
> +}
> +
> +static int pcat_wdt_ping(struct watchdog_device *wdev)
> +{
> + struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> + return pcat_pmu_send(data->pmu, PCAT_CMD_HEARTBEAT, NULL, 0);
> +}
> +
> +static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
> +{
> + int ret = 0;
> + struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> + data->timeout = val;
This needs to store 'timeout' in wdev. Storing it locally is unnecessary.
> + if (data->started)
> + ret = pcat_wdt_setup(data, data->timeout);
This is misleading because it would permit setting the timeout to
0 when the watchdog isn't running, and then when the watchdog is started
it would not really start it. The code should not use a local "started"
variable but call watchdog_active(). It should also not accept "0"
as a valid timeout.
> +
> + return ret;
> +}
> +
> +static const struct watchdog_ops pcat_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = pcat_wdt_start,
> + .stop = pcat_wdt_stop,
> + .ping = pcat_wdt_ping,
> + .set_timeout = pcat_wdt_set_timeout,
> +};
> +
> +static int pcat_watchdog_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pcat_watchdog *watchdog;
> +
> + watchdog = devm_kzalloc(dev, sizeof(*watchdog), GFP_KERNEL);
> + if (!watchdog)
> + return -ENOMEM;
> +
> + watchdog->dev = dev;
> + watchdog->pmu = dev_get_drvdata(dev->parent);
> + watchdog->wdd.info = &pcat_wdt_info;
> + watchdog->wdd.ops = &pcat_wdt_ops;
> + watchdog->wdd.timeout = 60;
> + watchdog->wdd.max_timeout = U8_MAX;
> + watchdog->wdd.min_timeout = 0;
This effectively lets the user ... kind of ... stop the watchdog
by setting the timeout to 0. This is not acceptable.
> + watchdog->wdd.parent = dev;
> +
> + watchdog_stop_on_reboot(&watchdog->wdd);
> + watchdog_set_drvdata(&watchdog->wdd, watchdog);
> + platform_set_drvdata(pdev, watchdog);
> +
No watchdog_init_timeout() ?
> + return devm_watchdog_register_device(dev, &watchdog->wdd);
> +}
> +
> +static const struct of_device_id pcat_watchdog_dt_ids[] = {
> + { .compatible = "ariaboard,photonicat-pmu-watchdog", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pcat_watchdog_dt_ids);
> +
> +static struct platform_driver pcat_watchdog_driver = {
> + .driver = {
> + .name = "photonicat-watchdog",
> + .of_match_table = pcat_watchdog_dt_ids,
> + },
> + .probe = pcat_watchdog_probe,
> +};
> +module_platform_driver(pcat_watchdog_driver);
> +
> +MODULE_AUTHOR("Junhao Xie <bigfoot@...ssfun.cn>");
> +MODULE_DESCRIPTION("Photonicat PMU watchdog");
> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists