Re: [PATCH v4 2/4] clocksource/drivers: Add support for the Renesas RZ/N1 timers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Down-counter returned without bitwise inversion for clocksource and sched_clock.
- [High] Uninitialized event_handler dereference when an interrupt fires early.
- [High] Inappropriate assignment of a 16-bit timer for system clocksource and sched_clock.
--

--- Patch [2]: [PATCH v4 2/4] clocksource/drivers: Add support for the Renesas RZ/N1 timers ---
commit eef53d5f7f215d9a1bbe8d768ba6df4cf250b8ae
Author: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>

clocksource/drivers: Add support for the Renesas RZ/N1 timers

This commit introduces support for the Renesas RZ/N1 SoC timer block.
It manages 6 16-bit timers and 2 32-bit timers, allowing configuration
for periodic or one-shot modes.

> diff --git a/drivers/clocksource/timer-rzn1.c b/drivers/clocksource/timer-rzn1.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clocksource/timer-rzn1.c

[ ... ]

> +static irqreturn_t rzn1_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct rzn1_timer *timer = dev_id;
> +
> +	/* Ack the interrupt */
> +	readl(timer->base + RZN1_TIMER_CLEAR_INT_REG);
> +
> +	timer->ced.event_handler(&timer->ced);

[Severity: High]
Can timer->ced.event_handler be NULL here?

If a spurious interrupt fires after devm_request_irq() or enable_irq() is
called, but before clockevents_config_and_register() populates the event
handler, will this unconditionally dereference a NULL pointer?

> +	return IRQ_HANDLED;
> +}

[ ... ]

> +static u64 notrace rzn1_sched_read(void)
> +{
> +	return readl_relaxed(rzn1_sched_clock->base + RZN1_TIMER_CURRENT_COUNT_REG);
> +}
> +
> +static u64 rzn1_clksrc_read(struct clocksource *cs)
> +{
> +	return readl_relaxed(rzn1_sched_clock->base + RZN1_TIMER_CURRENT_COUNT_REG);
> +}

[Severity: Critical]
Is this hardware timer a down-counter?

Since the timer is configured with RZN1_TIMER_LOAD_COUNT_REG, if the raw
CURRENT_COUNT_REG value is decrementing, will the timekeeping and scheduler
core observe a massive positive delta on every read due to unsigned
arithmetic wrap-around?

Does this value need a bitwise inversion to ensure a monotonically increasing
counter for the clocksource framework?

[ ... ]

> +static int rzn1_local_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct rzn1_timer *timer;
> +
> +	if (cpu >= RZN1_TIMER_NB_32BIT_TIMERS)
> +		return 0;
> +
> +	timer = &rzn1_tab_timers[cpu + RZN1_TIMER_BASE_INDEX_32BIT_TIMERS];
> +
> +	timer->ced.cpumask = cpumask_of(cpu);
> +	irq_force_affinity(timer->ced.irq, cpumask_of(cpu));
> +	enable_irq(timer->ced.irq);
> +
> +	clockevents_config_and_register(&timer->ced, timer->rate, 1,
> +					GENMASK(timer->width - 1, 0));

[Severity: High]
Is it safe to call enable_irq() before registering the clockevent device?

If an interrupt occurs immediately after enable_irq(), rzn1_timer_interrupt()
will be called before clockevents_config_and_register() sets up the
event_handler, potentially causing the NULL pointer dereference mentioned
above.

[ ... ]

> +static int rzn1_timer_setup_timers(struct platform_device *pdev,
> +				   struct rzn1_timer *tab_timers,
> +				   unsigned int first_timer_index,
> +				   unsigned int last_timer_index,
> +				   void __iomem *base, unsigned long clock_rate,
> +				   bool irq_disabled)
> +{

[ ... ]

> +		if (irq_disabled)
> +			irq_set_status_flags(timer->ced.irq, IRQ_NOAUTOEN);
> +
> +		ret = devm_request_irq(dev, timer->ced.irq, rzn1_timer_interrupt,
> +				       IRQF_TIMER, timer->ced.name, timer);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "timer%u: Failed to request IRQ\n", i);
> +
> +		rzn1_timer_int_enable(timer);
> +	}

[Severity: High]
Could this also trigger a NULL pointer dereference?

Since the IRQ is requested and enabled at the controller level here, before
clockevents_config_and_register() is called in the subsequent setup steps, an
early interrupt could trigger a crash.

[ ... ]

> +static int rzn1_timer_probe_first(struct platform_device *pdev,
> +				  struct rzn1_timer *tab_timers,
> +				  void __iomem *base, unsigned long clock_rate)
> +{

[ ... ]

> +	/*
> +	 * At this point, no more operations can fail. Perform remaining
> +	 * operations. Starting by handling the first 16-bit timer
> +	 */
> +	timer = &tab_timers[RZN1_TIMER_BASE_INDEX_16BIT_TIMERS];
> +
> +	rzn1_timer_init(timer, RZN1_TIMER_BASE_INDEX_16BIT_TIMERS, base, clock_rate);
> +	rzn1_timer_config(timer, GENMASK(timer->width - 1, 0), true);
> +
> +	rzn1_sched_clock = timer;
> +
> +	sched_clock_register(rzn1_sched_read, rzn1_sched_clock->width, rzn1_sched_clock->rate);
> +
> +	rzn1_clocksource.mask = CLOCKSOURCE_MASK(rzn1_sched_clock->width);
> +	clocksource_register_hz(&rzn1_clocksource, rzn1_sched_clock->rate);

[Severity: High]
Will using a 16-bit timer for the system clocksource break NO_HZ_IDLE?

Since the 16-bit timer is fed by a 1 MHz clock (clock_rate / 25), it will
wrap every ~65.5 milliseconds. Will this short wrap period force the core
timekeeping code to wake the system extremely frequently just to accumulate
time?

Would it be better to dedicate one of the available 32-bit timers to act
as the system clocksource and sched_clock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721082952.215691-1-herve.codina@bootlin.com?part=2