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?
If that's not the case, adding the ASSERT_UNREACHABLE() would be my
preference.
Thanks, Roger.
