| 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: <7cb271d2-1617-b18c-adc7-466c60f7ad70@c-s.fr>
Date: Wed, 8 Nov 2017 08:11:45 +0100
From: Christophe LEROY <christophe.leroy@....fr>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Wim Van Sebroeck <wim@...ana.be>, linux-kernel@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-watchdog@...r.kernel.org
Subject: Re: [PATCH] watchdog: mpc8xxx: use the core worker function
Le 07/11/2017 à 23:56, Guenter Roeck a écrit :
> On Tue, Nov 07, 2017 at 05:23:56PM +0100, Christophe Leroy wrote:
>> The watchdog core includes a worker function which pings the
>> watchdog until user app starts pinging it and which also
>> pings it if the HW require more frequent pings.
>> Use that function instead of the dedicated timer.
>> In the mean time, we can allow the user to change the timeout.
>>
>> Then change the timeout module parameter to use seconds and
>> use the watchdog_init_timeout() core function.
>>
>> On some HW (eg: the 8xx), SWCRR contains bits unrelated to the
>> watchdog which have to be preserved upon write.
>>
>> This driver has nothing preventing the use of the magic close, so
>> enable it.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@....fr>
>> ---
>> drivers/watchdog/mpc8xxx_wdt.c | 86 ++++++++++++++++++++----------------------
>> 1 file changed, 40 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
>> index 366e5c7e650b..c2ac4b6b1253 100644
>> --- a/drivers/watchdog/mpc8xxx_wdt.c
>> +++ b/drivers/watchdog/mpc8xxx_wdt.c
>> @@ -22,7 +22,6 @@
>> #include <linux/fs.h>
>> #include <linux/init.h>
>> #include <linux/kernel.h>
>> -#include <linux/timer.h>
>> #include <linux/of_address.h>
>> #include <linux/of_platform.h>
>> #include <linux/module.h>
>> @@ -31,10 +30,13 @@
>> #include <linux/uaccess.h>
>> #include <sysdev/fsl_soc.h>
>>
>> +#define WATCHDOG_TIMEOUT 10
>> +
>> struct mpc8xxx_wdt {
>> __be32 res0;
>> __be32 swcrr; /* System watchdog control register */
>> #define SWCRR_SWTC 0xFFFF0000 /* Software Watchdog Time Count. */
>> +#define SWCRR_SWF 0x00000008 /* Software Watchdog Freeze (mpc8xx). */
>> #define SWCRR_SWEN 0x00000004 /* Watchdog Enable bit. */
>> #define SWCRR_SWRI 0x00000002 /* Software Watchdog Reset/Interrupt Select bit.*/
>> #define SWCRR_SWPR 0x00000001 /* Software Watchdog Counter Prescale bit. */
>> @@ -52,14 +54,15 @@ struct mpc8xxx_wdt_type {
>> struct mpc8xxx_wdt_ddata {
>> struct mpc8xxx_wdt __iomem *base;
>> struct watchdog_device wdd;
>> - struct timer_list timer;
>> spinlock_t lock;
>> + int prescaler;
>> };
>>
>> -static u16 timeout = 0xffff;
>> +static u16 timeout;
>> module_param(timeout, ushort, 0);
>> MODULE_PARM_DESC(timeout,
>> - "Watchdog timeout in ticks. (0<timeout<65536, default=65535)");
>> + "Watchdog timeout in seconds. (1<timeout<65535, default="
>> + __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
>>
>> static bool reset = 1;
>> module_param(reset, bool, 0);
>> @@ -80,31 +83,35 @@ static void mpc8xxx_wdt_keepalive(struct mpc8xxx_wdt_ddata *ddata)
>> spin_unlock(&ddata->lock);
>> }
>>
>> -static void mpc8xxx_wdt_timer_ping(unsigned long arg)
>> -{
>> - struct mpc8xxx_wdt_ddata *ddata = (void *)arg;
>> -
>> - mpc8xxx_wdt_keepalive(ddata);
>> - /* We're pinging it twice faster than needed, just to be sure. */
>> - mod_timer(&ddata->timer, jiffies + HZ * ddata->wdd.timeout / 2);
>> -}
>> -
>> static int mpc8xxx_wdt_start(struct watchdog_device *w)
>> {
>> struct mpc8xxx_wdt_ddata *ddata =
>> container_of(w, struct mpc8xxx_wdt_ddata, wdd);
>> -
>> - u32 tmp = SWCRR_SWEN | SWCRR_SWPR;
>> + u32 tmp;
>> + u32 timeout;
>>
>> /* Good, fire up the show */
>> + tmp = in_be32(&ddata->base->swcrr);
>> + tmp &= ~(SWCRR_SWTC | SWCRR_SWF | SWCRR_SWEN | SWCRR_SWRI | SWCRR_SWPR);
>> + tmp |= SWCRR_SWEN | SWCRR_SWPR;
>> +
>> if (reset)
>> tmp |= SWCRR_SWRI;
>>
>> - tmp |= timeout << 16;
>> + timeout = ddata->wdd.timeout * fsl_get_sys_freq() / ddata->prescaler;
>> + tmp |= min(timeout, 0xffffU) << 16;
>>
>> out_be32(&ddata->base->swcrr, tmp);
>>
>> - del_timer_sync(&ddata->timer);
>> + tmp = in_be32(&ddata->base->swcrr);
>> + if (!(tmp & SWCRR_SWEN))
>> + return -EOPNOTSUPP;
>> +
>> + ddata->wdd.max_hw_heartbeat_ms = ((tmp >> 16) * ddata->prescaler) /
>> + (fsl_get_sys_freq() / 1000);
>> + ddata->wdd.min_timeout = ddata->wdd.max_hw_heartbeat_ms / 1000;
>
> That is odd. Why set the minimum timeout period to the maximum hardware
> timeout, rounded down to a full second ?
Once the HW watchdog timer has been configured with a given timeout
value, it cannot be changed anymore. This means that any higher timeout
can be managed via the watchdog core worker function, but no timeout
below the one configured in HW can be used.
>
> Note that it is wrong to set the values for min_timeout and
> max_hw_heartbeat_ms in the _start function; they should be set
> in the probe function. The start function should also not update
> ddata->wdd.timeout; that should happen in set_timeout if supported,
> or in probe.
The mpc8xxx HW watchdog can be (re)configured only once after startup,
after that it cannot be changed anymore. Therefore we have the following
cases to handle:
1/ The HW watchdog is started at reset and not (re)configured yet.
2/ The HW watchdog is no started at reset and not configured yet.
3/ The HW watchdog is configured by the boot loader.
Case 1/ The kernel has to poll it from the begining in accordance with
the default reset timeout. It can however reconfigure it once.
Case 2/ The kernel has nothing to do until a user app want to start it.
Case 3/ The kernel has to poll it from the begining iaw the configured
timeout. It cannot reconfigure it anymore.
The only way for the kernel to know if we are in case 1/ or case 3/ is
to try and configure the HW watchdog: if the new configuration is taken
into account, it means we are in case 1/, otherwise it is case 3/
The idea behind doing the configuration in the start function was:
a/ to not start the watchdog before an app decide to do so in the case
where the HW watchdog is not already started.
b/ to let the HW watchdog run with the reset timeout (which is usually
the max possible timeout) then provide an opportunity to get it reduced
to a lower value once the system is up and running.
But it is probably also OK to just take into account the given timeout
in the probe, the core watchdog work function will do its job anyway. I
will rework it in that way.
>
>> + if (ddata->wdd.timeout < ddata->wdd.min_timeout)
>> + ddata->wdd.timeout = ddata->wdd.min_timeout;
>>
>> return 0;
>> }
>> @@ -118,17 +125,8 @@ static int mpc8xxx_wdt_ping(struct watchdog_device *w)
>> return 0;
>> }
>>
>> -static int mpc8xxx_wdt_stop(struct watchdog_device *w)
>> -{
>> - struct mpc8xxx_wdt_ddata *ddata =
>> - container_of(w, struct mpc8xxx_wdt_ddata, wdd);
>> -
>> - mod_timer(&ddata->timer, jiffies);
>> - return 0;
>> -}
>> -
>> static struct watchdog_info mpc8xxx_wdt_info = {
>> - .options = WDIOF_KEEPALIVEPING,
>> + .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT,
>> .firmware_version = 1,
>> .identity = "MPC8xxx",
>> };
>> @@ -137,7 +135,6 @@ static struct watchdog_ops mpc8xxx_wdt_ops = {
>> .owner = THIS_MODULE,
>> .start = mpc8xxx_wdt_start,
>> .ping = mpc8xxx_wdt_ping,
>> - .stop = mpc8xxx_wdt_stop,
>
> That means the watchdog can not be stopped ?
That's correct, as you can see the stop function was previously just
(re)starting the polling timer. Now it is handled by the core watchdog work.
>
>> };
>>
>> static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>> @@ -148,7 +145,6 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>> struct mpc8xxx_wdt_ddata *ddata;
>> u32 freq = fsl_get_sys_freq();
>> bool enabled;
>> - unsigned int timeout_sec;
>>
>> wdt_type = of_device_get_match_data(&ofdev->dev);
>> if (!wdt_type)
>> @@ -173,35 +169,34 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
>> }
>>
>> spin_lock_init(&ddata->lock);
>> - setup_timer(&ddata->timer, mpc8xxx_wdt_timer_ping,
>> - (unsigned long)ddata);
>>
>> ddata->wdd.info = &mpc8xxx_wdt_info,
>> ddata->wdd.ops = &mpc8xxx_wdt_ops,
>>
>> - /* Calculate the timeout in seconds */
>> - timeout_sec = (timeout * wdt_type->prescaler) / freq;
>> -
>> - ddata->wdd.timeout = timeout_sec;
>> + ddata->wdd.timeout = WATCHDOG_TIMEOUT;
>> + watchdog_init_timeout(&ddata->wdd, timeout, &ofdev->dev);
>> + ddata->prescaler = wdt_type->prescaler;
>>
>> watchdog_set_nowayout(&ddata->wdd, nowayout);
>>
>> + /*
>> + * If the watchdog was previously enabled or we're running on
>> + * MPC8xxx, we should ping the wdt from the kernel until the
>> + * userspace handles it.
>> + */
>> + if (enabled) {
>> + mpc8xxx_wdt_start(&ddata->wdd);
>> + set_bit(WDOG_HW_RUNNING, &ddata->wdd.status);
>> + }
>> +
>> ret = watchdog_register_device(&ddata->wdd);
>> if (ret) {
>> pr_err("cannot register watchdog device (err=%d)\n", ret);
>> return ret;
>> }
>>
>> - pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d (%d seconds)\n",
>> - reset ? "reset" : "interrupt", timeout, timeout_sec);
>> -
>> - /*
>> - * If the watchdog was previously enabled or we're running on
>> - * MPC8xxx, we should ping the wdt from the kernel until the
>> - * userspace handles it.
>> - */
>> - if (enabled)
>> - mod_timer(&ddata->timer, jiffies);
>> + pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d sec\n",
>> + reset ? "reset" : "interrupt", ddata->wdd.timeout);
>>
>> platform_set_drvdata(ofdev, ddata);
>> return 0;
>> @@ -213,7 +208,6 @@ static int mpc8xxx_wdt_remove(struct platform_device *ofdev)
>>
>> pr_crit("Watchdog removed, expect the %s soon!\n",
>> reset ? "reset" : "machine check exception");
>> - del_timer_sync(&ddata->timer);
>> watchdog_unregister_device(&ddata->wdd);
>>
>> return 0;
>> --
>> 2.13.3
>>
Powered by blists - more mailing lists