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 <[email protected]>
>>>>>
>>>>> Acked-by: Roger Pau Monné <[email protected]>
>>>>
>>>> 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
