[PATCH 0/3] watchdog: honour hw_margin_ms property

[Stefan Roese]

On 04.06.20 10:28, Rasmus Villemoes wrote:
> On 02/06/2020 17.38, Rasmus Villemoes wrote:
>> On 02/06/2020 16.53, Stefan Roese wrote:
>>> On 02.06.20 15:29, Rasmus Villemoes wrote:
>>>> On 16/03/2020 16.52, Rasmus Villemoes wrote:
>>>>> On 14/03/2020 13.04, Stefan Roese wrote:
>>>>>> On 13.03.20 17:04, Rasmus Villemoes wrote:
>>>>>
> 
>>>> But, as I suspected, I do have a problem when loading a compressed
>>>> kernel image - what I write above "so even in U-Boot proper, time as
>>>> measured by get_timer() ceases to pass after that point, so all the
>>>> WATCHDOG_RESET() calls from the inflate code effectively get ignored."
>>>> is indeed the case.
>>>>
>>>> So, what's the best way to proceed? Should there be a hook disabling the
>>>>    rate-limiting logic that bootm_disable_interrupts() can call? Or must
>>>> get_timer() always return a sensible result even with interrupts
>>>> disabled?
>>>
>>> Wouldn't it make sense to move the bootm_disable_interrupts() call to
>>> after loading and uncompressing the OS image? To right before jumping
>>> to the OS?
>>
>> No, because the point of disabling interrupts is that we may start
>> writing to physical address 0 (e.g. if that's the load= address in the
>> FIT image), which is also where the interrupt vectors reside - i.e.,
>> we're about to overwrite 0x900 (the decrementer interrupt vector), so if
>> we don't disable interrupts, we'll crash on the very next decrementer
>> interrupt (i.e., within one millisecond).

Ah, thanks for refreshing me on this.

> FWIW, the below very hacky patch makes get_timer() return sensible
> results on ppc even when interrupts are disabled, and hence ensures that
> the watchdog does get petted. It's obviously not meant for inclusion as
> is (it's prepared for being a proper config option, but for toying
> around it's easier to have it all in one file - also, I don't really
> like the name of the config knob). But it's also kind of expensive to do
> that do_div(), so I'm not sure I think this is even the right approach.

I agree. This does not look as its going into the right direction. But
thanks for working on this anyways.

> You previously rejected allowing the board to provide an override for
> the rate-limiting,

 From my memory, I "just" suggested working on a different, more generic
approach. But if this fails, I'm open to re-visit the options.

> and at least the "hw_margin_ms" parsing now solves
> part of what I wanted to use that for. What about implementing the
> rate-limiting instead in terms of get_ticks() (the hw_margin_ms can
> trivially be translated to a number of ticks at init time - there's
> already a usec_to_tick helper)? Are there any boards where get_ticks()
> doesn't return something sensible?

Could you please send a new version of a patch(set) to address these
issues by using the "override for the rate-limiting" or some other
idea you have right now for this? I'll review it soon'ish.

Thanks,
Stefan

> Rasmus
> 
> _Not_ for inclusion:
> 
> diff --git a/arch/powerpc/lib/interrupts.c b/arch/powerpc/lib/interrupts.c
> index 23ac5bca1e..e6b6a967ae 100644
> --- a/arch/powerpc/lib/interrupts.c
> +++ b/arch/powerpc/lib/interrupts.c
> @@ -10,11 +10,14 @@
>   #include <common.h>
>   #include <irq_func.h>
>   #include <asm/processor.h>
> +#include <div64.h>
>   #include <watchdog.h>
>   #ifdef CONFIG_LED_STATUS
>   #include <status_led.h>
>   #endif
> 
> +#define CONFIG_GET_TIMER_IRQ 1
> +
>   #ifndef CONFIG_MPC83XX_TIMER
>   #ifndef CONFIG_SYS_WATCHDOG_FREQ
>   #define CONFIG_SYS_WATCHDOG_FREQ (CONFIG_SYS_HZ / 2)
> @@ -39,8 +42,33 @@ static __inline__ void set_dec (unsigned long val)
>   }
>   #endif /* !CONFIG_MPC83XX_TIMER */
> 
> +static u64 irq_off_ticks = 0;
> +static int interrupts_enabled = 0;
> +static volatile ulong timestamp = 0;
> +
> +static u32 irq_off_msecs(void)
> +{
> +	u64 t;
> +	u32 d = get_tbclk();
> +
> +	if (!d)
> +		return 0;
> +	t = get_ticks() - irq_off_ticks;
> +	t *= 1000;
> +	do_div(t, d);
> +	return t;
> +}
> +
>   void enable_interrupts(void)
>   {
> +	ulong msr = get_msr ();
> +
> +	if (IS_ENABLED(CONFIG_GET_TIMER_IRQ) && !(msr & MSR_EE)) {
> +		/* Account for the time interrupts were off. */
> +		timestamp += irq_off_msecs();
> +		interrupts_enabled = 1;
> +	}
> +
>   	set_msr (get_msr () | MSR_EE);
>   }
> 
> @@ -50,6 +78,13 @@ int disable_interrupts(void)
>   	ulong msr = get_msr ();
> 
>   	set_msr (msr & ~MSR_EE);
> +
> +	if (IS_ENABLED(CONFIG_GET_TIMER_IRQ) && (msr & MSR_EE)) {
> +		/* Record when interrupts were disabled. */
> +		irq_off_ticks = get_ticks();
> +		interrupts_enabled = 0;
> +	}
> +
>   	return ((msr & MSR_EE) != 0);
>   }
> 
> @@ -61,13 +96,11 @@ int interrupt_init(void)
> 
>   	set_dec (decrementer_count);
> 
> -	set_msr (get_msr () | MSR_EE);
> +	enable_interrupts();
> 
>   	return (0);
>   }
> 
> -static volatile ulong timestamp = 0;
> -
>   void timer_interrupt(struct pt_regs *regs)
>   {
>   	/* call cpu specific function from $(CPU)/interrupts.c */
> @@ -90,6 +123,12 @@ void timer_interrupt(struct pt_regs *regs)
> 
>   ulong get_timer (ulong base)
>   {
> -	return (timestamp - base);
> +	ulong ts = timestamp;
> +
> +	/* If called within an irq-off section, account for the time since
> irqs were turned off. */
> +	if (IS_ENABLED(CONFIG_GET_TIMER_IRQ) && !interrupts_enabled)
> +		ts += irq_off_msecs();
> +
> +	return (ts - base);
>   }
>   #endif /* !CONFIG_MPC83XX_TIMER */
> 
> 
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de