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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>>>>
>>>>> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>
>>>> 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