[PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[cuiguoqi]

When PREEMPT_RT is disabled,there is a possibility that
timer->is_hard will be incorrectly initialized.
When creating a high-resolution timer and setting the mode to
HRTIMER_MODE_ABS,the timer->is_hard will be incorrectly initialized.

Pseudocode logic:
1. mode
   >> HRTIMER_MODE_ABS	= 0x00

2. bool softtimer = !!(mode & HRTIMER_MODE_SOFT);	//!PREEMPT_RT
   base = softtimer ? HRTIMER_MAX_CLOCK_BASES / 2 : 0;	//!PREEMPT_RT
   > softtimer = false；				//!PREEMPT_RT
   > base = 0;						//hard mode

3. timer->is_soft = softtimer;
   > timer->is_soft = flase;

4. timer->is_hard = !!(mode & HRTIMER_MODE_HARD);	!PREEMPT_RT
   >  timer->is_hard = flase;				//error
   >  Based on point 2, by default, the hard mode is selected.

Of course, if PREEMPT_RT is enabled, timer->is_hard is correctly set.
   bool softtimer = !!(mode & HRTIMER_MODE_SOFT);
   if (IS_ENABLED(CONFIG_PREEMPT_RT) && !(mode & HRTIMER_MODE_HARD))
	softtimer = true;
   timer->is_soft = softtimer;				//true
   timer->is_hard = !!(mode & HRTIMER_MODE_HARD);	//flase

Signed-off-by: cuiguoqi <cuiguoqi@kylinos.cn>
---
 kernel/time/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 80fe3749d2db..9d8defb1e9b1 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1571,7 +1571,7 @@ static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
 	base = softtimer ? HRTIMER_MAX_CLOCK_BASES / 2 : 0;
 	base += hrtimer_clockid_to_base(clock_id);
 	timer->is_soft = softtimer;
-	timer->is_hard = !!(mode & HRTIMER_MODE_HARD);
+	timer->is_hard = !softtimer;
 	timer->base = &cpu_base->clock_base[base];
 	timerqueue_init(&timer->node);
 }
-- 
2.25.1

Re: [PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[Thomas Gleixner]

On Fri, Mar 21 2025 at 10:03, cuiguoqi@kylinos.cn wrote:

I have no idea why you think that sending this patch to
sales@linutronix.de makes any sense.

It's equally useful as me cc'ing sales@kylinos.cn on my reply.

> When PREEMPT_RT is disabled,there is a possibility that
> timer->is_hard will be incorrectly initialized.
> When creating a high-resolution timer and setting the mode to
> HRTIMER_MODE_ABS,the timer->is_hard will be incorrectly initialized.

What is the consequence, i.e. malfunction, of this being incorrectly
initialized?

Nothing as far as I can tell.

So what's incorrect? It's slightly inconsistent and confusing, but not
incorrect in the way that it causes malfunction, right?

If so, then please say so. If it causes malfunction, then describe it.

>  	timer->is_soft = softtimer;
> -	timer->is_hard = !!(mode & HRTIMER_MODE_HARD);
> +	timer->is_hard = !softtimer;

Now timer->is_soft is always !timer->is_hard, right?

Which means that one of those flags is superfluous, no?

Thanks,

        tglx

Re: [PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[cuiguoqi]

Hi tglx,
 
First of all, I'd like to thank you for your reply and for pointing out the email issue. 

As you've accurately noted, when PREEMPT_RT is disabled, it introduces a certain
degree of ambiguity for kernel developers. So far, no malfunctions have been
observed as a direct consequence of this.
 
This issue came to light during kernel development when using the is_hard flag, 
where the expected logic was not achieved.Naturally, we opted for the more reliable 
is_soft flag to address the situation.
 
Consequently, when PREEMPT_RT is disabled, there is a potential risk if developers
choose to use the is_hard flag. I firmly believe this risk should not be overlooked.

I concur with your analysis that one of these flags is redundant. In fact,
it might be possible to consolidate them based on the overall requirements. 
 
Thanks.
 

Cui Guoqi

Re: [PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[Sebastian Andrzej Siewior]

On 2025-03-22 16:24:03 [+0800], cuiguoqi wrote:
> Hi tglx,
>  
> First of all, I'd like to thank you for your reply and for pointing out the email issue. 
> 
> As you've accurately noted, when PREEMPT_RT is disabled, it introduces a certain
> degree of ambiguity for kernel developers. So far, no malfunctions have been
> observed as a direct consequence of this.
>  
> This issue came to light during kernel development when using the is_hard flag, 
> where the expected logic was not achieved.Naturally, we opted for the more reliable 
> is_soft flag to address the situation.
>  
> Consequently, when PREEMPT_RT is disabled, there is a potential risk if developers
> choose to use the is_hard flag. I firmly believe this risk should not be overlooked.

You shouldn't try to set the is_hard flag without knowing its
implications. HRTIMER_MODE_HARD is what you should pay attention to.
Everything else is implementation specific.

> I concur with your analysis that one of these flags is redundant. In fact,
> it might be possible to consolidate them based on the overall requirements. 

A timer that is missing HRTIMER_MODE_HARD is not a automatically a
HRTIMER_MODE_SOFT and also a timer missing HRTIMER_MODE_SOFT is not
automatically HRTIMER_MODE_HARD. These depends on RT vs non-RT mode.
Wouldn't your suggestion influence the warning in
hrtimer_start_range_ns()?

> Thanks.
>  
> 
> Cui Guoqi

Sebastian

Re: [PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[cuiguoqi]

Here,it is not set but referenced. When !PREEMPT, is_hard does not match
the actual situation.In the original logic, the actual mode selection is 
mainly based on softtimer.

When the HARD/SOFT mode is not explicitly configured, the following logic 
is used to select the hard/soft mode:

> bool softtimer = !!(mode & HRTIMER_MODE_SOFT); //PREEMPT_RT:flase/!PREEMPT_RT: flase
> if (IS_ENABLED(CONFIG_PREEMPT_RT) && !(mode & HRTIMER_MODE_HARD))
	softtimer = true;                        //PREEMPT_RT:true  
> base = softtimer ? HRTIMER_MAX_CLOCK_BASES/2:0;//PREEMPT_RT:soft mode/!PREEMPT_RT: hard mode
> base += hrtimer_clockid_to_base(clock_id);
> timer->base = &cpu_base->clock_base[base];
> timer->is_soft = softtimer;                   //PREEMPT_RT:true /!PREEMPT_RT: false
- timer->is_hard = !!(mode & HRTIMER_MODE_HARD);//PREEMPT_RT:false/!PREEMPT_RT: false
+ timer->is_hard = !softtimer;                  //PREEMPT_RT:false/!PREEMPT_RT: true 

Thus, under !PREEMPT, the distinction in is_hard is clear.
while it does not affect the logic of the hrtimer_start_range_ns: 
>	if (!IS_ENABLED(CONFIG_PREEMPT_RT))             
>		WARN_ON_ONCE(!(mode & HRTIMER_MODE_SOFT) ^ !timer->is_soft);
>	else
>		WARN_ON_ONCE(!(mode & HRTIMER_MODE_HARD) ^ !timer->is_hard);

[PATCH] hrtimer: Fix the incorrect initialization of timer->is_hard

[cuiguoqi]

When PREEMPT_RT is disabled,there is a possibility that
timer->is_hard will be incorrectly initialized.
When creating a high-resolution timer and setting the mode to
HRTIMER_MODE_ABS,the timer->is_hard will be incorrectly initialized.

Pseudocode logic:
1. mode
   >> HRTIMER_MODE_ABS	= 0x00

2. bool softtimer = !!(mode & HRTIMER_MODE_SOFT);	//!PREEMPT_RT
   base = softtimer ? HRTIMER_MAX_CLOCK_BASES / 2 : 0;	//!PREEMPT_RT
   > softtimer = false；				//!PREEMPT_RT
   > base = 0;						//hard mode

3. timer->is_soft = softtimer;
   > timer->is_soft = flase;

4. timer->is_hard = !!(mode & HRTIMER_MODE_HARD);	!PREEMPT_RT
   >  timer->is_hard = flase;				//error
   >  Based on point 2, by default, the hard mode is selected.

Of course, if PREEMPT_RT is enabled, timer->is_hard is correctly set.
   bool softtimer = !!(mode & HRTIMER_MODE_SOFT);
   if (IS_ENABLED(CONFIG_PREEMPT_RT) && !(mode & HRTIMER_MODE_HARD))
	softtimer = true;
   timer->is_soft = softtimer;				//true
   timer->is_hard = !!(mode & HRTIMER_MODE_HARD);	//flase

Signed-off-by: cuiguoqi <cuiguoqi@kylinos.cn>
---
 kernel/time/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 80fe3749d2db..9d8defb1e9b1 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1571,7 +1571,7 @@ static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
 	base = softtimer ? HRTIMER_MAX_CLOCK_BASES / 2 : 0;
 	base += hrtimer_clockid_to_base(clock_id);
 	timer->is_soft = softtimer;
-	timer->is_hard = !!(mode & HRTIMER_MODE_HARD);
+	timer->is_hard = !softtimer;
 	timer->base = &cpu_base->clock_base[base];
 	timerqueue_init(&timer->node);
 }
-- 
2.25.1