Re: [v2] Enhanced support for MPC8xx/8xxx watchdog

[Guenter Roeck]

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@xxxxxx>
> > >
> > >
> > > 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/