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

[Scott Wood]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/