x86/HPET: tidying / improvements

[PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Jan Beulich 2 months, 3 weeks ago

When this was added, the log message was updated correctly, but the zero
case was needlessly checked separately: hpet_broadcast_enter() had a zero
check added at the same time, while handle_hpet_broadcast() can't possibly
pass 0 here anyway.

Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -147,10 +147,10 @@ static int reprogram_hpet_evt_channel(
     int64_t delta;
     int ret;
 
-    if ( (ch->flags & HPET_EVT_DISABLE) || (expire == 0) )
+    if ( ch->flags & HPET_EVT_DISABLE )
         return 0;
 
-    if ( unlikely(expire < 0) )
+    if ( unlikely(expire <= 0) )
     {
         printk(KERN_DEBUG "reprogram: expire <= 0\n");
         return -ETIME;

Re: [PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Roger Pau Monné 2 weeks, 3 days ago

On Mon, Nov 17, 2025 at 03:39:30PM +0100, Jan Beulich wrote:
> When this was added, the log message was updated correctly, but the zero
> case was needlessly checked separately: hpet_broadcast_enter() had a zero
> check added at the same time, while handle_hpet_broadcast() can't possibly
> pass 0 here anyway.
> 
> Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Similar to the previous commit, I wonder whether it would make sense
to add an ASSERT_UNREACHABLE() if that error path is not reachable
given the logic in the callers.

Thanks, Roger.

Re: [PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Jan Beulich 2 weeks, 3 days ago

On 22.01.2026 10:18, Roger Pau Monné wrote:
> On Mon, Nov 17, 2025 at 03:39:30PM +0100, Jan Beulich wrote:
>> When this was added, the log message was updated correctly, but the zero
>> case was needlessly checked separately: hpet_broadcast_enter() had a zero
>> check added at the same time, while handle_hpet_broadcast() can't possibly
>> pass 0 here anyway.
>>
>> Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Similar to the previous commit, I wonder whether it would make sense
> to add an ASSERT_UNREACHABLE() if that error path is not reachable
> given the logic in the callers.

That would mean

    if ( unlikely(expire < 0) )
    {
        printk(KERN_DEBUG "reprogram: expire <= 0\n");
        return -ETIME;
    }

    if ( unlikely(expire == 0) )
    {
        ASSERT_UNREACHABLE();
        return -ETIME;
    }

which I fear I don't like (for going too far). Even

    if ( unlikely(expire <= 0) )
    {
        printk(KERN_DEBUG "reprogram: expire <= 0\n");
        ASSERT(expire);
        return -ETIME;
    }

I'd be uncertain about, as that needlessly gives 0 a meaning that isn't
required anymore in this function.

Jan

Re: [PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Roger Pau Monné 2 weeks, 3 days ago

On Thu, Jan 22, 2026 at 10:28:51AM +0100, Jan Beulich wrote:
> On 22.01.2026 10:18, Roger Pau Monné wrote:
> > On Mon, Nov 17, 2025 at 03:39:30PM +0100, Jan Beulich wrote:
> >> When this was added, the log message was updated correctly, but the zero
> >> case was needlessly checked separately: hpet_broadcast_enter() had a zero
> >> check added at the same time, while handle_hpet_broadcast() can't possibly
> >> pass 0 here anyway.
> >>
> >> Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > Similar to the previous commit, I wonder whether it would make sense
> > to add an ASSERT_UNREACHABLE() if that error path is not reachable
> > given the logic in the callers.
> 
> That would mean
> 
>     if ( unlikely(expire < 0) )
>     {
>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>         return -ETIME;
>     }
> 
>     if ( unlikely(expire == 0) )
>     {
>         ASSERT_UNREACHABLE();
>         return -ETIME;
>     }
> 
> which I fear I don't like (for going too far). Even
> 
>     if ( unlikely(expire <= 0) )
>     {
>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>         ASSERT(expire);
>         return -ETIME;
>     }
> 
> I'd be uncertain about, as that needlessly gives 0 a meaning that isn't
> required anymore in this function.

Hm, OK, I was under the impression that both < 0 and 0 should never be
passed by the callers.  If expire == 0 is a possible input then I
don't think the ASSERT() is that helpful.

Thanks, Roger.

Re: [PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Jan Beulich 2 weeks, 3 days ago

On 22.01.2026 11:10, Roger Pau Monné wrote:
> On Thu, Jan 22, 2026 at 10:28:51AM +0100, Jan Beulich wrote:
>> On 22.01.2026 10:18, Roger Pau Monné wrote:
>>> On Mon, Nov 17, 2025 at 03:39:30PM +0100, Jan Beulich wrote:
>>>> When this was added, the log message was updated correctly, but the zero
>>>> case was needlessly checked separately: hpet_broadcast_enter() had a zero
>>>> check added at the same time, while handle_hpet_broadcast() can't possibly
>>>> pass 0 here anyway.
>>>>
>>>> Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Thanks.
>>
>>> Similar to the previous commit, I wonder whether it would make sense
>>> to add an ASSERT_UNREACHABLE() if that error path is not reachable
>>> given the logic in the callers.
>>
>> That would mean
>>
>>     if ( unlikely(expire < 0) )
>>     {
>>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>>         return -ETIME;
>>     }
>>
>>     if ( unlikely(expire == 0) )
>>     {
>>         ASSERT_UNREACHABLE();
>>         return -ETIME;
>>     }
>>
>> which I fear I don't like (for going too far). Even
>>
>>     if ( unlikely(expire <= 0) )
>>     {
>>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>>         ASSERT(expire);
>>         return -ETIME;
>>     }
>>
>> I'd be uncertain about, as that needlessly gives 0 a meaning that isn't
>> required anymore in this function.
> 
> Hm, OK, I was under the impression that both < 0 and 0 should never be
> passed by the callers.  If expire == 0 is a possible input then I
> don't think the ASSERT() is that helpful.

Oh, so you were after

    if ( unlikely(expire <= 0) )
    {
        printk(KERN_DEBUG "reprogram: expire <= 0\n");
        ASSERT_UNREACHABLE();
        return -ETIME;
    }

(perhaps even with the printk() dropped)? That I could buy off on, as NOW()
is expected to only ever return valid (positive) s_time_t values.

Jan

Re: [PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Roger Pau Monné 2 weeks, 3 days ago

On Thu, Jan 22, 2026 at 11:15:49AM +0100, Jan Beulich wrote:
> On 22.01.2026 11:10, Roger Pau Monné wrote:
> > On Thu, Jan 22, 2026 at 10:28:51AM +0100, Jan Beulich wrote:
> >> On 22.01.2026 10:18, Roger Pau Monné wrote:
> >>> On Mon, Nov 17, 2025 at 03:39:30PM +0100, Jan Beulich wrote:
> >>>> When this was added, the log message was updated correctly, but the zero
> >>>> case was needlessly checked separately: hpet_broadcast_enter() had a zero
> >>>> check added at the same time, while handle_hpet_broadcast() can't possibly
> >>>> pass 0 here anyway.
> >>>>
> >>>> Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>
> >>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Thanks.
> >>
> >>> Similar to the previous commit, I wonder whether it would make sense
> >>> to add an ASSERT_UNREACHABLE() if that error path is not reachable
> >>> given the logic in the callers.
> >>
> >> That would mean
> >>
> >>     if ( unlikely(expire < 0) )
> >>     {
> >>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
> >>         return -ETIME;
> >>     }
> >>
> >>     if ( unlikely(expire == 0) )
> >>     {
> >>         ASSERT_UNREACHABLE();
> >>         return -ETIME;
> >>     }
> >>
> >> which I fear I don't like (for going too far). Even
> >>
> >>     if ( unlikely(expire <= 0) )
> >>     {
> >>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
> >>         ASSERT(expire);
> >>         return -ETIME;
> >>     }
> >>
> >> I'd be uncertain about, as that needlessly gives 0 a meaning that isn't
> >> required anymore in this function.
> > 
> > Hm, OK, I was under the impression that both < 0 and 0 should never be
> > passed by the callers.  If expire == 0 is a possible input then I
> > don't think the ASSERT() is that helpful.
> 
> Oh, so you were after
> 
>     if ( unlikely(expire <= 0) )
>     {
>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>         ASSERT_UNREACHABLE();
>         return -ETIME;
>     }
> 
> (perhaps even with the printk() dropped)? That I could buy off on, as NOW()
> is expected to only ever return valid (positive) s_time_t values.

Yes, that's what I was thinking off, but your previous reply made me
think there are possible cases where expire < 0 gets passed to the
function?

If that's not the case, adding the ASSERT_UNREACHABLE() would be my
preference.

Thanks, Roger.

Re: [PATCH v4 6/8] x86/HPET: simplify "expire" check a little in reprogram_hpet_evt_channel()

Posted by Jan Beulich 2 weeks, 3 days ago

On 22.01.2026 12:30, Roger Pau Monné wrote:
> On Thu, Jan 22, 2026 at 11:15:49AM +0100, Jan Beulich wrote:
>> On 22.01.2026 11:10, Roger Pau Monné wrote:
>>> On Thu, Jan 22, 2026 at 10:28:51AM +0100, Jan Beulich wrote:
>>>> On 22.01.2026 10:18, Roger Pau Monné wrote:
>>>>> On Mon, Nov 17, 2025 at 03:39:30PM +0100, Jan Beulich wrote:
>>>>>> When this was added, the log message was updated correctly, but the zero
>>>>>> case was needlessly checked separately: hpet_broadcast_enter() had a zero
>>>>>> check added at the same time, while handle_hpet_broadcast() can't possibly
>>>>>> pass 0 here anyway.
>>>>>>
>>>>>> Fixes: 7145897cfb81 ("cpuidle: Fix for timer_deadline==0 case")
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Thanks.
>>>>
>>>>> Similar to the previous commit, I wonder whether it would make sense
>>>>> to add an ASSERT_UNREACHABLE() if that error path is not reachable
>>>>> given the logic in the callers.
>>>>
>>>> That would mean
>>>>
>>>>     if ( unlikely(expire < 0) )
>>>>     {
>>>>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>>>>         return -ETIME;
>>>>     }
>>>>
>>>>     if ( unlikely(expire == 0) )
>>>>     {
>>>>         ASSERT_UNREACHABLE();
>>>>         return -ETIME;
>>>>     }
>>>>
>>>> which I fear I don't like (for going too far). Even
>>>>
>>>>     if ( unlikely(expire <= 0) )
>>>>     {
>>>>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>>>>         ASSERT(expire);
>>>>         return -ETIME;
>>>>     }
>>>>
>>>> I'd be uncertain about, as that needlessly gives 0 a meaning that isn't
>>>> required anymore in this function.
>>>
>>> Hm, OK, I was under the impression that both < 0 and 0 should never be
>>> passed by the callers.  If expire == 0 is a possible input then I
>>> don't think the ASSERT() is that helpful.
>>
>> Oh, so you were after
>>
>>     if ( unlikely(expire <= 0) )
>>     {
>>         printk(KERN_DEBUG "reprogram: expire <= 0\n");
>>         ASSERT_UNREACHABLE();
>>         return -ETIME;
>>     }
>>
>> (perhaps even with the printk() dropped)? That I could buy off on, as NOW()
>> is expected to only ever return valid (positive) s_time_t values.
> 
> Yes, that's what I was thinking off, but your previous reply made me
> think there are possible cases where expire < 0 gets passed to the
> function?

No, I don't think there are any.

> If that's not the case, adding the ASSERT_UNREACHABLE() would be my
> preference.

Okay, that's what I'll commit then.

Jan