RE: [PATCH 1/2] clocksource: imx-tpm: fix return -ETIME when delta exceeds INT_MAX

["Jacky Bai (OSS)" <ping.bai@oss.nxp.com>]

> Subject: Re: [PATCH 1/2] clocksource: imx-tpm: fix return -ETIME when delta
> exceeds INT_MAX
> 
> On Mon, Aug 19, 2024 at 03:21:36PM +0200, Daniel Lezcano wrote:
> > On 25/07/2024 21:33, Frank Li wrote:
> > > From: Jacky Bai <ping.bai@nxp.com>
> > >
> > > In tpm_set_next_event(delta), return -ETIME by wrong cast to int
> > > when delta is larger than INT_MAX.
> >
> > Is it something you observed or from your understanding of the code ?
> 
> System use this timer to wakeup from lower idle, which core and local timer turn
> off. -ETIME may prevent system enter lower idle, or failure wakeup from lower
> idle utill counter run overflow. I am not sure how to trigger this problem.
> 
> Assume run at 24Mhz, 1/24us * 0x8000_0000 is around 89s. Problem trigger
> only next event bigger 89s, which relative quite big number.
> 
> Anyways, the original code is wrong, it claim max delta is 32bit.
> 
> Jack bai: any additonal supplement for this?
>

The issue can be triggered in below function, it is possible max_delta will be assigned to delta,
So the original int case logic will report wrong '-ETIME' error, then 'clockevents_program_min_delta'
Will be called to reprogram the timer, due to the tpm limitation mentioned in patch 2/2, this min_delta
program will not take effect.

int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
                        bool force) {
	...
	delta = min(delta, (int64_t) dev->max_delta_ns);
	delta = max(delta, (int64_t) dev->min_delta_ns);

	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
	rc = dev->set_next_event((unsigned long) clc, dev);

	return (rc && force) ? clockevents_program_min_delta(dev) : rc;
}

BR
> Frank
> 
> >
> > > For example:
> > >
> > > tpm_set_next_event(delta = 0xffff_fffe) {
> > >          ...
> > >          next = tpm_read_counter(); // assume next is 0x10
> > >          next += delta; // next will 0xffff_fffe + 0x10 = 0x1_0000_000e
> > >          now = tpm_read_counter();  // now is 0x10
> > >          ...
> > >
> > >          return (int)(next - now) <= 0 ? -ETIME : 0;
> > >                       ^^^^^^^^^^
> > >                       0x1_0000_000e - 0x10 = 0xffff_fffe, which is -2
> when
> > >                       cast to int. So return -ETIME.
> > > }
> > >
> > > To fix this, introduce a 'prev' variable and check if 'now - prev'
> > > is larger than delta.
> > >
> > > Cc: <stable@vger.kernel.org>
> > > Fixes: 059ab7b82eec ("clocksource/drivers/imx-tpm: Add imx tpm timer
> > > support")
> > > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > Reviewed-by: Jason Liu <jason.hui.liu@nxp.com>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >   drivers/clocksource/timer-imx-tpm.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/timer-imx-tpm.c
> > > b/drivers/clocksource/timer-imx-tpm.c
> > > index bd64a8a8427f3..cd23caf1e5999 100644
> > > --- a/drivers/clocksource/timer-imx-tpm.c
> > > +++ b/drivers/clocksource/timer-imx-tpm.c
> > > @@ -83,10 +83,10 @@ static u64 notrace tpm_read_sched_clock(void)
> > >   static int tpm_set_next_event(unsigned long delta,
> > >   				struct clock_event_device *evt)
> > >   {
> > > -	unsigned long next, now;
> > > +	unsigned long next, prev, now;
> > > -	next = tpm_read_counter();
> > > -	next += delta;
> > > +	prev = tpm_read_counter();
> > > +	next = prev + delta;
> > >   	writel(next, timer_base + TPM_C0V);
> > >   	now = tpm_read_counter();
> > > @@ -96,7 +96,7 @@ static int tpm_set_next_event(unsigned long delta,
> > >   	 * of writing CNT registers which may cause the min_delta event got
> > >   	 * missed, so we need add a ETIME check here in case it happened.
> > >   	 */
> > > -	return (int)(next - now) <= 0 ? -ETIME : 0;
> > > +	return (now - prev) >= delta ? -ETIME : 0;
> > >   }
> > >   static int tpm_set_state_oneshot(struct clock_event_device *evt)
> >
> >
> > --
> > <http://www.linaro.org/> Linaro.org │ Open source software for ARM
> > SoCs
> >
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog