Re: [PATCH 7/7] watchdog: dw_wdt: Add DebugFS files

From: Sergey Semin
Date: Fri Apr 10 2020 - 15:12:07 EST

Next message: Nick Desaulniers: "Re: [PATCH] elfnote: mark all .note sections SHF_ALLOC"
Previous message: Colin King: "[PATCH] net: neterion: remove redundant assignment to variable tmp64"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Mar 06, 2020 at 07:12:32AM -0800, Guenter Roeck wrote:
> On 3/6/20 5:27 AM, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> > From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> > Cc: Paul Burton <paulburton@xxxxxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > ---
> > 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.

I see your point. What if I keep the registers dump and read-only
timeout, pretimeout, enable and timeleft nodes? This won't be harmful,
but the info still can be useful to debug the driver.

-Sergey

>
> 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);
> >
>

Next message: Nick Desaulniers: "Re: [PATCH] elfnote: mark all .note sections SHF_ALLOC"
Previous message: Colin King: "[PATCH] net: neterion: remove redundant assignment to variable tmp64"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]