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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

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

Thanks, Roger.