Re: [PATCH RESEND v7 3/3] riscv: clocksource: Add p8700-gcru driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Resource leak in `p8700_timer_init()` on the error path if `clocksource_mmio_init()` fails.
- [Low] The Kconfig entry lacks `depends on 64BIT`, causing compilation failures on 32-bit RISC-V architectures.
--

commit 4f34b087c1b81aba548d943831bdb092b55bf784
Author: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>

riscv: clocksource: Add p8700-gcru driver

This commit adds a clocksource driver for the MIPS P8700 GCRU. It uses
timer-of and clocksource_mmio helpers for initialization.

> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index ffcd23668763f..861e7b8c93376 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig

[ ... ]

> @@ -672,6 +672,15 @@ config CLINT_TIMER
>  	  This option enables the CLINT timer for RISC-V systems.  The CLINT
>  	  driver is usually used for NoMMU RISC-V systems.
>  
> +config P8700_TIMER
> +	bool "MIPS P8700 timer driver"
> +	depends on GENERIC_SCHED_CLOCK && RISCV && RISCV_SBI

Does this driver also need to depend on 64BIT?

The driver unconditionally uses readq_relaxed() and clocksource_mmio_readq_up.
On 32-bit RISC-V platforms, readq_relaxed() is not defined, which will cause
undeclared identifier errors during compilation.

If we try to fix the build by including linux/io-64-nonatomic-lo-hi.h, it
could introduce torn reads when reading the 64-bit hardware counter with two
non-atomic 32-bit reads, leading to massive sched_clock() time jumps.

> +	select CLKSRC_MMIO
> +	select TIMER_PROBE
> +	select TIMER_OF
> +	help
> +	  Enables support for MIPS P8700 timer driver.

[ ... ]

> diff --git a/drivers/clocksource/timer-p8700.c b/drivers/clocksource/timer-p8700.c
> new file mode 100644
> index 0000000000000..220ed8efdfe55
> --- /dev/null
> +++ b/drivers/clocksource/timer-p8700.c

[ ... ]

> +static int __init p8700_timer_init(struct device_node *node)
> +{
> +	int error = 0;
> +
> +	error = timer_of_init(node, &gcru_of);
> +	if (error)
> +		return error;
> +
> +	p8700_time_val = timer_of_base(&gcru_of);
> +	/* Now init the mmio timer with the address we got from DT */
> +	error = clocksource_mmio_init(p8700_time_val, "mips,p8700-gcru",
> +				      riscv_timebase, 450, 64,
> +				      clocksource_mmio_readq_up);
> +	if (error)
> +		return error;

If clocksource_mmio_init() fails, does this code leak the I/O memory mapped
by timer_of_init()?

It looks like timer_of_init() maps the memory via of_iomap(), but this error
path directly returns without calling timer_of_cleanup() to release those
mappings and other associated timer_of resources.

> +
> +	/* Sched clock */
> +	sched_clock_register(p8700_timer_sched_read, 64, riscv_timebase);
> +
> +	return error;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-riscv-time-mmio-v7-0-f8925ffc226b@htecgroup.com?part=3