From: Guenter Roeck <linux@roeck-us•net>
To: leroy christophe <christophe.leroy@c-s•fr>
Cc: Scott Wood <scottwood@freescale•com>,
Wim Van Sebroeck <wim@iguana•be>,
linuxppc-dev@lists•ozlabs.org, linux-kernel@vger•kernel.org,
linux-watchdog@vger•kernel.org
Subject: Re: [v2] Enhanced support for MPC8xx/8xxx watchdog
Date: Thu, 08 Aug 2013 01:49:41 -0700 [thread overview]
Message-ID: <52035BA5.1030405@roeck-us.net> (raw)
In-Reply-To: <520331C1.4060406@c-s.fr>
On 08/07/2013 10:50 PM, leroy christophe wrote:
> Le 26/06/2013 01:04, Scott Wood a écrit :
>> On Thu, Feb 28, 2013 at 09:52:22AM +0100, LEROY Christophe wrote:
>>> This patch modifies the behaviour of the MPC8xx/8xxx watchdog. On the MPC8xx,
>>> at 133Mhz, the maximum timeout of the watchdog timer is 1s, which means it must
>>> be pinged twice a second. This is not in line with the Linux watchdog concept
>>> which is based on a default watchdog timeout around 60s.
>>> This patch introduces an intermediate layer between the CPU and the userspace.
>>> The kernel pings the watchdog at the required frequency at the condition that
>>> userspace tools refresh it regularly.
>>> Existing parameter 'timeout' is renamed 'hw_time'.
>>> The new parameter 'timeout' allows to set up the userspace timeout.
>>> The driver also implements the WDIOC_SETTIMEOUT ioctl.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s•fr>
>>>
>>>
>>> diff -ur linux-3.7.9/drivers/watchdog/mpc8xxx_wdt.c linux/drivers/watchdog/mpc8xxx_wdt.c
>>> --- linux-3.7.9/drivers/watchdog/mpc8xxx_wdt.c 2013-02-17 19:53:32.000000000 +0100
>>> +++ linux/drivers/watchdog/mpc8xxx_wdt.c 2013-02-27 16:00:07.000000000 +0100
>>> @@ -52,10 +52,17 @@
>>> static struct mpc8xxx_wdt __iomem *wd_base;
>>> static int mpc8xxx_wdt_init_late(void);
>>> -static u16 timeout = 0xffff;
>>> -module_param(timeout, ushort, 0);
>>> +#define WD_TIMO 10 /* Default timeout = 10 seconds */
>> If the default Linux watchdog timeout is normally 60 seconds, why is it 10
>> here?
> Looks like each driver has its own default value, but agreed, I change it to 60 seconds
>
>>> +static uint timeout = WD_TIMO;
>>> +module_param(timeout, uint, 0);
>>> MODULE_PARM_DESC(timeout,
>>> - "Watchdog timeout in ticks. (0<timeout<65536, default=65535)");
>>> + "Watchdog SW timeout in seconds. (0 < timeout < 65536s, default = "
>>> + __MODULE_STRING(WD_TIMO) "s)");
>>> +static u16 hw_timo = 0xffff;
>>> +module_param(hw_timo, ushort, 0);
>>> +MODULE_PARM_DESC(hw_timo,
>>> + "Watchdog HW timeout in ticks. (0 < hw_timo < 65536, default = 65535)");
>> hw_timeout would be more legibile -- this is a public interface.
> Ok
>>
>>> static bool reset = 1;
>>> module_param(reset, bool, 0);
>>> @@ -72,10 +79,12 @@
>>> * to 0
>>> */
>>> static int prescale = 1;
>>> -static unsigned int timeout_sec;
>>> +static unsigned int hw_timo_sec;
>>> +static int wdt_auto = 1;
>> bool, and add a comment indicating what this means.
> Ok
>>
>>> static unsigned long wdt_is_open;
>>> static DEFINE_SPINLOCK(wdt_spinlock);
>>> +static unsigned long wdt_last_ping;
>>> static void mpc8xxx_wdt_keepalive(void)
>>> {
>>> @@ -91,9 +100,20 @@
>>> static void mpc8xxx_wdt_timer_ping(unsigned long arg)
>>> {
>>> - mpc8xxx_wdt_keepalive();
>>> - /* We're pinging it twice faster than needed, just to be sure. */
>>> - mod_timer(&wdt_timer, jiffies + HZ * timeout_sec / 2);
>>> + if (wdt_auto)
>>> + wdt_last_ping = jiffies;
>>> +
>>> + if (jiffies - wdt_last_ping <= timeout * HZ) {
>> So timeout cannot be more than UINT_MAX / HZ... Might want to check for
>> that, just in case.
> Ok.
>>
>> What happens if there's a race? If another CPU updates wdt_last_ping in
>> parallel, then you could see wdt_last_ping greater than the value you
Using the watchdog infrastructure (which has a mutex to avoid the problem)
would help avoiding this race.
>> read for jiffies. Since this is an unsigned comparison, it will fail to
>> call keepalive. You might get saved by pinging it twice as often as
>> necessary, but you shouldn't rely on that.
> Euh ... This watchdog is integrated inside the CPU, so there is no chance that any external CPU get access to it.
Unless I am missing something, neither jiffies nor wdt_last_ping nor timeout
is integrated inside a CPU.
There are macros for well defined time comparison which you possibly
might want to consider using (such as time_after() and time_before() etc).
My overall feedback is that I believe it would make more sense to convert
the driver to the watchdog infrastructure first, then add the softdog as
second patch.
Guenter
>>
>>> + mpc8xxx_wdt_keepalive();
>>> + /* We're pinging it twice faster than needed, to be sure. */
>>> + mod_timer(&wdt_timer, jiffies + HZ * hw_timo_sec / 2);
>>> + }
>>> +}
>>> +
>>> +static void mpc8xxx_wdt_sw_keepalive(void)
>>> +{
>>> + wdt_last_ping = jiffies;
>>> + mpc8xxx_wdt_timer_ping(0);
>>> }
>> This isn't new with this patch, but it looks like
>> mpc8xxx_wdt_keepalive() can be called either from timer context, or with
>> interrupts enabled... yet it uses a bare spin_lock() rather than an
>> irq-safe version. This should be fixed.
> Ok, I'll propose another patch for that. Indeed, is the spin_lock needed at all ? If we get two writes interleaved, it will make it anyway.
>
> Christophe
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger•kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
next prev parent reply other threads:[~2013-08-08 8:56 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-28 8:52 [PATCH v2] Enhanced support for MPC8xx/8xxx watchdog Christophe Leroy
2013-06-25 23:04 ` [v2] " Scott Wood
2013-08-08 5:50 ` leroy christophe
2013-08-08 8:49 ` Guenter Roeck [this message]
2013-08-10 0:02 ` Scott Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52035BA5.1030405@roeck-us.net \
--to=linux@roeck-us$(echo .)net \
--cc=christophe.leroy@c-s$(echo .)fr \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-watchdog@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=scottwood@freescale$(echo .)com \
--cc=wim@iguana$(echo .)be \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox