| 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: <addd7387-b388-3977-575d-d63d77f8fd38@roeck-us.net>
Date: Fri, 6 Mar 2020 07:12:32 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Sergey.Semin@...kalelectronics.ru,
Wim Van Sebroeck <wim@...ux-watchdog.org>
Cc: Serge Semin <fancer.lancer@...il.com>,
Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Paul Burton <paulburton@...nel.org>,
Ralf Baechle <ralf@...ux-mips.org>,
linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] watchdog: dw_wdt: Add DebugFS files
On 3/6/20 5:27 AM, Sergey.Semin@...kalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@...kalelectronics.ru>
>
> For the sake of the easier device-driver debug procedure, we added a
> few DebugFS files with the next content: watchdog timeout, watchdog
> pre-timeout, watchdog ping/kick operation, watchdog start/stop, device
> registers dump. They are available only if kernel is configured with
> DebugFS support.
>
> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> Cc: Paul Burton <paulburton@...nel.org>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> ---
> drivers/watchdog/dw_wdt.c | 150 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 150 insertions(+)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 3000120f7e39..9353d83f3e45 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -27,6 +27,7 @@
> #include <linux/platform_device.h>
> #include <linux/reset.h>
> #include <linux/watchdog.h>
> +#include <linux/debugfs.h>
>
> #define WDOG_CONTROL_REG_OFFSET 0x00
> #define WDOG_CONTROL_REG_WDT_EN_MASK 0x01
> @@ -38,8 +39,14 @@
> #define WDOG_COUNTER_RESTART_KICK_VALUE 0x76
> #define WDOG_INTERRUPT_STATUS_REG_OFFSET 0x10
> #define WDOG_INTERRUPT_CLEAR_REG_OFFSET 0x14
> +#define WDOG_COMP_PARAMS_5_REG_OFFSET 0xe4
> +#define WDOG_COMP_PARAMS_4_REG_OFFSET 0xe8
> +#define WDOG_COMP_PARAMS_3_REG_OFFSET 0xec
> +#define WDOG_COMP_PARAMS_2_REG_OFFSET 0xf0
> #define WDOG_COMP_PARAMS_1_REG_OFFSET 0xf4
> #define WDOG_COMP_PARAMS_1_USE_FIX_TOP BIT(6)
> +#define WDOG_COMP_VERSION_REG_OFFSET 0xf8
> +#define WDOG_COMP_TYPE_REG_OFFSET 0xfc
>
> /* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> #define DW_WDT_NUM_TOPS 16
> @@ -79,6 +86,10 @@ struct dw_wdt {
> /* Save/restore */
> u32 control;
> u32 timeout;
> +
> +#ifdef CONFIG_DEBUG_FS
> + struct dentry *dbgfs_dir;
> +#endif
> };
>
> #define to_dw_wdt(wdd) container_of(wdd, struct dw_wdt, wdd)
> @@ -430,6 +441,141 @@ static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
> }
>
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int dw_wdt_dbgfs_timeout_get(void *priv, u64 *val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + *val = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
> +
> + return 0;
> +}
> +
> +static int dw_wdt_dbgfs_timeout_set(void *priv, u64 val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + return dw_wdt_set_timeout(&dw_wdt->wdd, val);
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_timeout_fops,
> + dw_wdt_dbgfs_timeout_get, dw_wdt_dbgfs_timeout_set, "%llu\n");
> +
> +static int dw_wdt_dbgfs_pretimeout_get(void *priv, u64 *val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + *val = dw_wdt->wdd.pretimeout;
> +
> + return 0;
> +}
> +
> +static int dw_wdt_dbgfs_pretimeout_set(void *priv, u64 val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + return dw_wdt_set_pretimeout(&dw_wdt->wdd, val);
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_pretimeout_fops,
> + dw_wdt_dbgfs_pretimeout_get, dw_wdt_dbgfs_pretimeout_set, "%llu\n");
> +
> +static int dw_wdt_dbgfs_ping_set(void *priv, u64 val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + dw_wdt_ping(&dw_wdt->wdd);
> +
> + return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_ping_fops,
> + NULL, dw_wdt_dbgfs_ping_set, "%llu\n");
> +
> +static int dw_wdt_dbgfs_en_get(void *priv, u64 *val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + *val = !!dw_wdt_is_enabled(dw_wdt);
> +
> + return 0;
> +}
> +
> +static int dw_wdt_dbgfs_en_set(void *priv, u64 val)
> +{
> + struct dw_wdt *dw_wdt = priv;
> +
> + if (val)
> + dw_wdt_start(&dw_wdt->wdd);
> + else
> + dw_wdt_stop(&dw_wdt->wdd);
> +
> + return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(dw_wdt_dbgfs_en_fops,
> + dw_wdt_dbgfs_en_get, dw_wdt_dbgfs_en_set, "%llu\n");
> +
> +#define DW_WDT_DBGFS_REG(_name, _off) \
> +{ \
> + .name = _name, \
> + .offset = _off \
> +}
> +
> +static const struct debugfs_reg32 dw_wdt_dbgfs_regs[] = {
> + DW_WDT_DBGFS_REG("cr", WDOG_CONTROL_REG_OFFSET),
> + DW_WDT_DBGFS_REG("torr", WDOG_TIMEOUT_RANGE_REG_OFFSET),
> + DW_WDT_DBGFS_REG("ccvr", WDOG_CURRENT_COUNT_REG_OFFSET),
> + DW_WDT_DBGFS_REG("crr", WDOG_COUNTER_RESTART_REG_OFFSET),
> + DW_WDT_DBGFS_REG("stat", WDOG_INTERRUPT_STATUS_REG_OFFSET),
> + DW_WDT_DBGFS_REG("param5", WDOG_COMP_PARAMS_5_REG_OFFSET),
> + DW_WDT_DBGFS_REG("param4", WDOG_COMP_PARAMS_4_REG_OFFSET),
> + DW_WDT_DBGFS_REG("param3", WDOG_COMP_PARAMS_3_REG_OFFSET),
> + DW_WDT_DBGFS_REG("param2", WDOG_COMP_PARAMS_2_REG_OFFSET),
> + DW_WDT_DBGFS_REG("param1", WDOG_COMP_PARAMS_1_REG_OFFSET),
> + DW_WDT_DBGFS_REG("version", WDOG_COMP_VERSION_REG_OFFSET),
> + DW_WDT_DBGFS_REG("type", WDOG_COMP_TYPE_REG_OFFSET)
> +};
> +
> +static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt)
> +{
> + struct device *dev = dw_wdt->wdd.parent;
> + struct debugfs_regset32 *regset;
> +
> + regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
> + if (!regset)
> + return;
> +
> + regset->regs = dw_wdt_dbgfs_regs;
> + regset->nregs = ARRAY_SIZE(dw_wdt_dbgfs_regs);
> + regset->base = dw_wdt->regs;
> +
> + dw_wdt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> +
> + debugfs_create_regset32("registers", 0444, dw_wdt->dbgfs_dir, regset);
> +
> + debugfs_create_file_unsafe("timeout", 0600, dw_wdt->dbgfs_dir,
> + dw_wdt, &dw_wdt_dbgfs_timeout_fops);
> +
> + debugfs_create_file_unsafe("pretimeout", 0600, dw_wdt->dbgfs_dir,
> + dw_wdt, &dw_wdt_dbgfs_pretimeout_fops);
> +
> + debugfs_create_file_unsafe("ping", 0200, dw_wdt->dbgfs_dir,
> + dw_wdt, &dw_wdt_dbgfs_ping_fops);
> +
> + debugfs_create_file_unsafe("enable", 0600, dw_wdt->dbgfs_dir,
> + dw_wdt, &dw_wdt_dbgfs_en_fops);
> +}
I don't oppose debugfs support in general, but I really don't see
the point replicating watchdog core functionality for it - even more so
since this bypasses the watchdog core. This lets one, for example,
enable the watchdog and then unload the driver with the watchdog
running. This is not only unnecessary, it is way too dangerous,
debugfs or not.
NACK for that part.
Guenter
> +
> +static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt)
> +{
> + debugfs_remove_recursive(dw_wdt->dbgfs_dir);
> +}
> +
> +#else /* !CONFIG_DEBUG_FS */
> +
> +static void dw_wdt_dbgfs_init(struct dw_wdt *dw_wdt) {}
> +static void dw_wdt_dbgfs_clear(struct dw_wdt *dw_wdt) {}
> +
> +#endif /* !CONFIG_DEBUG_FS */
> +
> static int dw_wdt_drv_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -544,6 +690,8 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> if (ret)
> goto out_disable_pclk;
>
> + dw_wdt_dbgfs_init(dw_wdt);
> +
> return 0;
>
> out_disable_pclk:
> @@ -558,6 +706,8 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
> {
> struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
>
> + dw_wdt_dbgfs_clear(dw_wdt);
> +
> watchdog_unregister_device(&dw_wdt->wdd);
> reset_control_assert(dw_wdt->rst);
> clk_disable_unprepare(dw_wdt->pclk);
>
Powered by blists - more mailing lists