| 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: <4aecb77e-f635-9bfd-c2bd-21cb68fa333d@roeck-us.net>
Date: Fri, 27 Mar 2020 14:12:18 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Chris Packham <chris.packham@...iedtelesis.co.nz>,
a.zummo@...ertech.it, alexandre.belloni@...tlin.com,
wim@...ux-watchdog.org
Cc: linux-rtc@...r.kernel.org, linux-watchdog@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rtc: ds1307: add support for watchdog timer on ds1388
On 3/26/20 9:18 PM, Chris Packham wrote:
> The DS1388 variant has watchdog timer capabilities. When using a DS1388
> and having enabled CONFIG_WATCHDOG_CORE register a watchdog device for
> the DS1388.
>
> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> ---
> This is going to the linux-watchdog list as well this time so it's probably the
> first time the watchdog maintainers have seen it.
>
> Changes in v2:
> - Address review comments from Alexandre, the only functional change is setting
> the hundredths of seconds to 0 instead of 99.
>
> drivers/rtc/rtc-ds1307.c | 97 ++++++++++++++++++++++++++++++++++++++++
"select WATCHDOG_CORE if WATCHDOG"
should be added to Kconfig. While it makes sense for watchdog functionality
to depend on WATCHDOG, it should not depend on the existence of another
watchdog driver in the system.
> 1 file changed, 97 insertions(+)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 31a38d468378..1452982c3a6a 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -22,6 +22,7 @@
> #include <linux/hwmon-sysfs.h>
> #include <linux/clk-provider.h>
> #include <linux/regmap.h>
> +#include <linux/watchdog.h>
>
> /*
> * We can't determine type by probing, but if we expect pre-Linux code
> @@ -144,8 +145,15 @@ enum ds_type {
> # define M41TXX_BIT_CALIB_SIGN BIT(5)
> # define M41TXX_M_CALIBRATION GENMASK(4, 0)
>
> +#define DS1388_REG_WDOG_HUN_SECS 0x08
> +#define DS1388_REG_WDOG_SECS 0x09
> #define DS1388_REG_FLAG 0x0b
> +# define DS1388_BIT_WF BIT(6)
> # define DS1388_BIT_OSF BIT(7)
> +#define DS1388_REG_CONTROL 0x0c
> +# define DS1388_BIT_RST BIT(0)
> +# define DS1388_BIT_WDE BIT(1)
> +
> /* negative offset step is -2.034ppm */
> #define M41TXX_NEG_OFFSET_STEP_PPB 2034
> /* positive offset step is +4.068ppm */
> @@ -166,6 +174,9 @@ struct ds1307 {
> #ifdef CONFIG_COMMON_CLK
> struct clk_hw clks[2];
> #endif
> +#ifdef CONFIG_WATCHDOG_CORE
> + struct watchdog_device wdt;
> +#endif
I don't immediately see why this would be necessary. I think it would
be better to allocate struct watchdog_device in ds1307_wdt_register().
> };
>
> struct chip_desc {
> @@ -854,6 +865,58 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset)
> ctrl_reg);
> }
>
> +#ifdef CONFIG_WATCHDOG_CORE
> +static int ds1388_wdt_start(struct watchdog_device *wdt_dev)
> +{
> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
> + u8 regs[2];
> + int ret;
> +
> + ret = regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
> + DS1388_BIT_WF, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
> + DS1388_BIT_WDE | DS1388_BIT_RST, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * watchdog timeouts are measured in seconds. So ignore hundreths of
hundredths
> + * seconds field.
> + */
> + regs[0] = 0;
> + regs[1] = bin2bcd(wdt_dev->timeout);
> +
> + ret = regmap_bulk_write(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs,
> + sizeof(regs));
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
> + DS1388_BIT_WDE | DS1388_BIT_RST,
> + DS1388_BIT_WDE | DS1388_BIT_RST);
> +}
> +
> +static int ds1388_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
> +
> + return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
> + DS1388_BIT_WDE | DS1388_BIT_RST, 0);
> +}
> +
> +static int ds1388_wdt_ping(struct watchdog_device *wdt_dev)
> +{
> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
> + u8 regs[2];
> +
> + return regmap_bulk_read(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs,
> + sizeof(regs));
> +}
> +#endif
> +
> static const struct rtc_class_ops rx8130_rtc_ops = {
> .read_time = ds1307_get_time,
> .set_time = ds1307_set_time,
> @@ -1576,6 +1639,39 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
>
> #endif /* CONFIG_COMMON_CLK */
>
> +#ifdef CONFIG_WATCHDOG_CORE
> +static const struct watchdog_info ds1388_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "DS1388 watchdog",
> +};
> +
> +static const struct watchdog_ops ds1388_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = ds1388_wdt_start,
> + .stop = ds1388_wdt_stop,
> + .ping = ds1388_wdt_ping,
Maybe I am missing something, but I don't see how the timeout is updated
if it is changed while the watchdog is already running.
> +};
> +
> +static void ds1307_wdt_register(struct ds1307 *ds1307)
> +{
> + if (ds1307->type != ds_1388)
> + return;
> +
> + ds1307->wdt.info = &ds1388_wdt_info;
> + ds1307->wdt.ops = &ds1388_wdt_ops;
> + ds1307->wdt.max_timeout = 99;
> + ds1307->wdt.min_timeout = 1;
> +
> + watchdog_init_timeout(&ds1307->wdt, 99, ds1307->dev);
That is quite pointless; just set wdt.timeout to 99 (assuming
that is what you want as default).
watchdog_init_timeout() only makes sense if it is used to set the
timeout from a module parameter or from a devicetree property.
> + watchdog_set_drvdata(&ds1307->wdt, ds1307);
> + watchdog_register_device(&ds1307->wdt);
Please call devm_watchdog_register_device().
> +}
> +#else
> +static void ds1307_wdt_register(struct ds1307 *ds1307)
> +{
> +}
> +#endif /* CONFIG_WATCHDOG_CORE */
> +
> static const struct regmap_config regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -1865,6 +1961,7 @@ static int ds1307_probe(struct i2c_client *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> + ds1307_wdt_register(ds1307);
>
> return 0;
>
>
Powered by blists - more mailing lists