races in omap_wdt

races in omap_wdt

[Uwe Kleine-König]

Hello,

I'm working on an AM335x machine and want to make use of the hardware
watchdog. I write to you because you contributed to the respective Linux
driver and judging from your email address you might be in a position to
help me with a problem I see.

To get reliable behaviour, I don't want to have situations where the
watchdog is disabled. There are currently two situation where this
happens with the current state of linux/drivers/watchdog/omap_wdt.c:

 a) in omap_wdt_probe() there is an unconditional call to
    omap_wdt_disable() which stops the counter. Ideally I'd want to take
    over the hardware in the state that it is in when Linux is started.
    This is a bit complicated, but should be doable. Would such a patch
    be welcome?

 b) The reference manual demands:

	To modify the timer counter value (the WDT_WCRR register),
	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
	watchdog timer must be disabled by using the start/stop sequence
	(the WDT_WSPR register).

    This effectively means (and is implemented accordingly in the
    driver) that on .set_timeout the driver has to stop the counter.
    So we have an (admittedly small) unprotected window each time the
    timeout is set (which happens for example when /dev/watchdog is
    opened). Is there something that can be done here? I imagine that
    the above statement from the reference manual could be holding out
    some details like "When writing a new load value (WDT_WLDR) without
    stopping the timer first, the new load value doesn't have any
    influence on the running timer. It is only used on the next trigger
    event." Is this too much to wish for?

    Maybe you can forward this problem to one of the hardware guys at TI
    who can give a definite picture what happens at the hardware level
    when the load value is updated for a running watchdog?
    (This would also simplify the patch for case a). For a) it would be
    nice to know the according details for the WDT_WCLR register, too.)

Thanks in advance,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: races in omap_wdt

[Uwe Kleine-König]

[dropped Varadarajan Charulatha and Shubhrajyoti D from the addressees
because the ti mailserver doesn't know them :-(, added Felipe instead]

On Thu, Apr 23, 2015 at 11:43:54AM +0200, Uwe Kleine-König wrote:
> I'm working on an AM335x machine and want to make use of the hardware
> watchdog. I write to you because you contributed to the respective Linux
> driver and judging from your email address you might be in a position to
> help me with a problem I see.
> 
> To get reliable behaviour, I don't want to have situations where the
> watchdog is disabled. There are currently two situation where this
> happens with the current state of linux/drivers/watchdog/omap_wdt.c:
> 
>  a) in omap_wdt_probe() there is an unconditional call to
>     omap_wdt_disable() which stops the counter. Ideally I'd want to take
>     over the hardware in the state that it is in when Linux is started.
>     This is a bit complicated, but should be doable. Would such a patch
>     be welcome?
> 
>  b) The reference manual demands:
> 
> 	To modify the timer counter value (the WDT_WCRR register),
> 	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
> 	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
> 	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
> 	watchdog timer must be disabled by using the start/stop sequence
> 	(the WDT_WSPR register).
> 
>     This effectively means (and is implemented accordingly in the
>     driver) that on .set_timeout the driver has to stop the counter.
>     So we have an (admittedly small) unprotected window each time the
>     timeout is set (which happens for example when /dev/watchdog is
>     opened). Is there something that can be done here? I imagine that
>     the above statement from the reference manual could be holding out
>     some details like "When writing a new load value (WDT_WLDR) without
>     stopping the timer first, the new load value doesn't have any
>     influence on the running timer. It is only used on the next trigger
>     event." Is this too much to wish for?
As Felipe predicted (via irc) it's really needed to stop the timer to
reprogram the reload value. I proved this by hacking my bootloader's mw
command to correctly write to the watchdog registers (i.e wait until the
corresponding bit in the Watchdog Write Posting Bits Register disappears
to signal the write has reached the hardware) and did:

	# make the wdog slow
	mw 0x44e35024 0x3c

	# set reload value (WLDR)
	mw 0x44e3502c 0xff000000

	# start wdog
	mw 0x44e35048 0xbbbb
	mw 0x44e35048 0x4444

	# set reload value lower (WLDR)
	mw 0x44e35024 0x00000000

	# trigger
	mw 0x44e35030 0x12345678
	mw 0x44e35030 0x87654321

	# show current counter (WCRR)
	md 0x44e35028+4

	sleep 4

	# trigger again
	mw 0x44e35030 0x12345678
	mw 0x44e35030 0x87654321

	# show current counter (WCRR)
	md 0x44e35028+4

	# stop wdog
	mw 0x44e35048 0xaaaa
	mw 0x44e35048 0x5555

The output is twice

	44e35028: ff000001

:-(.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: races in omap_wdt

[Uwe Kleine-König]

Hello,

just a small addition for the archive:

On Fri, Apr 24, 2015 at 11:18:40AM +0200, Uwe Kleine-König wrote:
> [dropped Varadarajan Charulatha and Shubhrajyoti D from the addressees
> because the ti mailserver doesn't know them :-(, added Felipe instead]
> 
> On Thu, Apr 23, 2015 at 11:43:54AM +0200, Uwe Kleine-König wrote:
> > I'm working on an AM335x machine and want to make use of the hardware
> > watchdog. I write to you because you contributed to the respective Linux
> > driver and judging from your email address you might be in a position to
> > help me with a problem I see.
> > 
> > To get reliable behaviour, I don't want to have situations where the
> > watchdog is disabled. There are currently two situation where this
> > happens with the current state of linux/drivers/watchdog/omap_wdt.c:
> > 
> >  a) in omap_wdt_probe() there is an unconditional call to
> >     omap_wdt_disable() which stops the counter. Ideally I'd want to take
> >     over the hardware in the state that it is in when Linux is started.
> >     This is a bit complicated, but should be doable. Would such a patch
> >     be welcome?
> > 
> >  b) The reference manual demands:
> > 
> > 	To modify the timer counter value (the WDT_WCRR register),
> > 	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
> > 	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
> > 	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
> > 	watchdog timer must be disabled by using the start/stop sequence
> > 	(the WDT_WSPR register).
> > 
> >     This effectively means (and is implemented accordingly in the
> >     driver) that on .set_timeout the driver has to stop the counter.
> >     So we have an (admittedly small) unprotected window each time the
> >     timeout is set (which happens for example when /dev/watchdog is
> >     opened). Is there something that can be done here? I imagine that
> >     the above statement from the reference manual could be holding out
> >     some details like "When writing a new load value (WDT_WLDR) without
> >     stopping the timer first, the new load value doesn't have any
> >     influence on the running timer. It is only used on the next trigger
> >     event." Is this too much to wish for?
> As Felipe predicted (via irc) it's really needed to stop the timer to
> reprogram the reload value. I proved this by hacking my bootloader's mw
> command to correctly write to the watchdog registers (i.e wait until the
> corresponding bit in the Watchdog Write Posting Bits Register disappears
> to signal the write has reached the hardware) and did:
> 
> 	# make the wdog slow
> 	mw 0x44e35024 0x3c
> 
> 	# set reload value (WLDR)
> 	mw 0x44e3502c 0xff000000
> 
> 	# start wdog
> 	mw 0x44e35048 0xbbbb
> 	mw 0x44e35048 0x4444
> 
> 	# set reload value lower (WLDR)
> 	mw 0x44e35024 0x00000000
I just noticed that ^ is wrong, it must be 0x44e3502c. But this doesn't
change the outcome. I also tried to redo the start wdog sequence here
without a preceding stop. Doesn't help also. So there doesn't seem to be
a way to avoid the need to disable the watchdog for programming.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: races in omap_wdt

[Uwe Kleine-König]

On Fri, Apr 24, 2015 at 11:18:40AM +0200, Uwe Kleine-König wrote:
> [dropped Varadarajan Charulatha and Shubhrajyoti D from the addressees
> because the ti mailserver doesn't know them :-(, added Felipe instead]
> 
> On Thu, Apr 23, 2015 at 11:43:54AM +0200, Uwe Kleine-König wrote:
> > I'm working on an AM335x machine and want to make use of the hardware
> > watchdog. I write to you because you contributed to the respective Linux
> > driver and judging from your email address you might be in a position to
> > help me with a problem I see.
> > 
> > To get reliable behaviour, I don't want to have situations where the
> > watchdog is disabled. There are currently two situation where this
> > happens with the current state of linux/drivers/watchdog/omap_wdt.c:
> > 
> >  a) in omap_wdt_probe() there is an unconditional call to
> >     omap_wdt_disable() which stops the counter. Ideally I'd want to take
> >     over the hardware in the state that it is in when Linux is started.
> >     This is a bit complicated, but should be doable. Would such a patch
> >     be welcome?
> > 
> >  b) The reference manual demands:
> > 
> > 	To modify the timer counter value (the WDT_WCRR register),
> > 	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
> > 	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
> > 	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
> > 	watchdog timer must be disabled by using the start/stop sequence
> > 	(the WDT_WSPR register).
> > 
> >     This effectively means (and is implemented accordingly in the
> >     driver) that on .set_timeout the driver has to stop the counter.
> >     So we have an (admittedly small) unprotected window each time the
> >     timeout is set (which happens for example when /dev/watchdog is
> >     opened). Is there something that can be done here? I imagine that
> >     the above statement from the reference manual could be holding out
> >     some details like "When writing a new load value (WDT_WLDR) without
> >     stopping the timer first, the new load value doesn't have any
> >     influence on the running timer. It is only used on the next trigger
> >     event." Is this too much to wish for?
> As Felipe predicted (via irc) it's really needed to stop the timer to
> reprogram the reload value. I proved this by hacking my bootloader's mw
> command to correctly write to the watchdog registers (i.e wait until the
> corresponding bit in the Watchdog Write Posting Bits Register disappears
> to signal the write has reached the hardware) and did:
> 
> 	# make the wdog slow
> 	mw 0x44e35024 0x3c
> 
> 	# set reload value (WLDR)
> 	mw 0x44e3502c 0xff000000
> 
> 	# start wdog
> 	mw 0x44e35048 0xbbbb
> 	mw 0x44e35048 0x4444
> 
> 	# set reload value lower (WLDR)
> 	mw 0x44e35024 0x00000000
> 
> 	# trigger
> 	mw 0x44e35030 0x12345678
> 	mw 0x44e35030 0x87654321
> 
> 	# show current counter (WCRR)
> 	md 0x44e35028+4
> 
> 	sleep 4
> 
> 	# trigger again
> 	mw 0x44e35030 0x12345678
> 	mw 0x44e35030 0x87654321
> 
> 	# show current counter (WCRR)
> 	md 0x44e35028+4
> 
> 	# stop wdog
> 	mw 0x44e35048 0xaaaa
> 	mw 0x44e35048 0x5555
> 
> The output is twice
> 
> 	44e35028: ff000001
> 
> :-(.
I guess none of you from inside TI checked any deeper, right? Honestly I
doubt that it would result in a positive surprise, but still I'm pinging
these questions once more. Do you can provide a way how to reprogram the
hardware without stopping it first?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: races in omap_wdt

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4158 bytes --]

On Fri, May 22, 2015 at 08:16:47PM +0200, Uwe Kleine-König wrote:
> On Fri, Apr 24, 2015 at 11:18:40AM +0200, Uwe Kleine-König wrote:
> > [dropped Varadarajan Charulatha and Shubhrajyoti D from the addressees
> > because the ti mailserver doesn't know them :-(, added Felipe instead]
> > 
> > On Thu, Apr 23, 2015 at 11:43:54AM +0200, Uwe Kleine-König wrote:
> > > I'm working on an AM335x machine and want to make use of the hardware
> > > watchdog. I write to you because you contributed to the respective Linux
> > > driver and judging from your email address you might be in a position to
> > > help me with a problem I see.
> > > 
> > > To get reliable behaviour, I don't want to have situations where the
> > > watchdog is disabled. There are currently two situation where this
> > > happens with the current state of linux/drivers/watchdog/omap_wdt.c:
> > > 
> > >  a) in omap_wdt_probe() there is an unconditional call to
> > >     omap_wdt_disable() which stops the counter. Ideally I'd want to take
> > >     over the hardware in the state that it is in when Linux is started.
> > >     This is a bit complicated, but should be doable. Would such a patch
> > >     be welcome?
> > > 
> > >  b) The reference manual demands:
> > > 
> > > 	To modify the timer counter value (the WDT_WCRR register),
> > > 	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
> > > 	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
> > > 	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
> > > 	watchdog timer must be disabled by using the start/stop sequence
> > > 	(the WDT_WSPR register).
> > > 
> > >     This effectively means (and is implemented accordingly in the
> > >     driver) that on .set_timeout the driver has to stop the counter.
> > >     So we have an (admittedly small) unprotected window each time the
> > >     timeout is set (which happens for example when /dev/watchdog is
> > >     opened). Is there something that can be done here? I imagine that
> > >     the above statement from the reference manual could be holding out
> > >     some details like "When writing a new load value (WDT_WLDR) without
> > >     stopping the timer first, the new load value doesn't have any
> > >     influence on the running timer. It is only used on the next trigger
> > >     event." Is this too much to wish for?
> > As Felipe predicted (via irc) it's really needed to stop the timer to
> > reprogram the reload value. I proved this by hacking my bootloader's mw
> > command to correctly write to the watchdog registers (i.e wait until the
> > corresponding bit in the Watchdog Write Posting Bits Register disappears
> > to signal the write has reached the hardware) and did:
> > 
> > 	# make the wdog slow
> > 	mw 0x44e35024 0x3c
> > 
> > 	# set reload value (WLDR)
> > 	mw 0x44e3502c 0xff000000
> > 
> > 	# start wdog
> > 	mw 0x44e35048 0xbbbb
> > 	mw 0x44e35048 0x4444
> > 
> > 	# set reload value lower (WLDR)
> > 	mw 0x44e35024 0x00000000
> > 
> > 	# trigger
> > 	mw 0x44e35030 0x12345678
> > 	mw 0x44e35030 0x87654321
> > 
> > 	# show current counter (WCRR)
> > 	md 0x44e35028+4
> > 
> > 	sleep 4
> > 
> > 	# trigger again
> > 	mw 0x44e35030 0x12345678
> > 	mw 0x44e35030 0x87654321
> > 
> > 	# show current counter (WCRR)
> > 	md 0x44e35028+4
> > 
> > 	# stop wdog
> > 	mw 0x44e35048 0xaaaa
> > 	mw 0x44e35048 0x5555
> > 
> > The output is twice
> > 
> > 	44e35028: ff000001
> > 
> > :-(.
> I guess none of you from inside TI checked any deeper, right? Honestly I
> doubt that it would result in a positive surprise, but still I'm pinging
> these questions once more. Do you can provide a way how to reprogram the
> hardware without stopping it first?

heh, as I said before, this IP requires a stop to be reprogrammed. I
went all the way to back the OMAP1710 TRM (the earliest TRM I still have
around) and lo and behold that in section 17.1.7 it states that "to
modify the timer counter value, prescaler ratio, or load valud, the
32-bit watchdog must be disabled by using a specific start/stop
sequence".

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]