Message-ID: <1376092951.15633.93.camel@snotra.buserror.net>
Subject: Re: [v2] Enhanced support for MPC8xx/8xxx watchdog
From: Scott Wood <scottwood@freescale.com>
To: leroy christophe <christophe.leroy@c-s.fr>
CC: Wim Van Sebroeck <wim@iguana.be>, <linuxppc-dev@lists.ozlabs.org>,
        <linux-kernel@vger.kernel.org>, <linux-watchdog@vger.kernel.org>
Date: Fri, 9 Aug 2013 19:02:31 -0500
In-Reply-To: <520331C1.4060406@c-s.fr>
References: <201302280852.r1S8qMYu003742@localhost.localdomain>
	 <20130625230431.GA29393@home.buserror.net> <520331C1.4060406@c-s.fr>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1808
Lines: 43

On Thu, 2013-08-08 at 07:50 +0200, leroy christophe wrote:
> Le 26/06/2013 01:04, Scott Wood a écrit :
> > 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
> > 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.

Hmm, it looks like mpc8641d (which is the only multi-core SoC among mpc8xx/mpc83xx/mpc86xx) does not have this watchdog, even though mpc8610 does.

So pretend I said "what if you get preempted?". :-)

> >> +		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.

I suppose...  I don't like relying on things like that, though.

-Scott


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