| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aC4Ma3E461XMBig0@mai.linaro.org>
Date: Wed, 21 May 2025 19:24:59 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Alexey Charkov <alchark@...il.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Guenter Roeck <linux@...ck-us.net>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for
watchdog functionality
On Wed, May 21, 2025 at 05:00:11PM +0400, Alexey Charkov wrote:
> VIA/WonderMedia system timer can generate a watchdog reset when its
> clocksource counter matches the value in the match register 0 and
> watchdog function is enabled. For this to work, obvously the clock event
> device must use a different match register (1~3) and respective interrupt.
>
> Check if at least two interrupts are provided by the device tree, then use
> match register 1 for system clock events and reserve match register 0 for
> the watchdog. Instantiate an auxiliary device for the watchdog
>
> Signed-off-by: Alexey Charkov <alchark@...il.com>
> ---
> MAINTAINERS | 1 +
> drivers/clocksource/Kconfig | 1 +
> drivers/clocksource/timer-vt8500.c | 111 ++++++++++++++++++++++++++++++++++---
> include/linux/vt8500-timer.h | 18 ++++++
It should endup in include/clocksource/vt8500-timer.h
> 4 files changed, 122 insertions(+), 9 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783e5ee6854b69cca87b6f0763844d28b4b2213f..5362095240627f613638197fda275db6edc16cf7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
> F: drivers/video/fbdev/vt8500lcdfb.*
> F: drivers/video/fbdev/wm8505fb*
> F: drivers/video/fbdev/wmt_ge_rops.*
> +F: include/linux/vt8500-timer.h
>
> ARM/ZYNQ ARCHITECTURE
> M: Michal Simek <michal.simek@....com>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 487c8525996724fbf9c6e9726dabb478d86513b9..92f071aade10b7c0f0bba4b47dc6228a5e50360f 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -178,6 +178,7 @@ config TEGRA186_TIMER
> config VT8500_TIMER
> bool "VT8500 timer driver" if COMPILE_TEST
> depends on HAS_IOMEM
> + select AUXILIARY_BUS
> help
> Enables support for the VT8500 driver.
>
> diff --git a/drivers/clocksource/timer-vt8500.c b/drivers/clocksource/timer-vt8500.c
> index 9f28f30dcaf83ab4e9c89952175b0d4c75bd6b40..cdea5245f8e41d65b8b9bebad3fe3a55f43a18fa 100644
> --- a/drivers/clocksource/timer-vt8500.c
> +++ b/drivers/clocksource/timer-vt8500.c
> @@ -11,6 +11,7 @@
> * Alexey Charkov. Minor changes have been made for Device Tree Support.
> */
>
> +#include <linux/auxiliary_bus.h>
> #include <linux/io.h>
> #include <linux/irq.h>
> #include <linux/interrupt.h>
> @@ -22,9 +23,6 @@
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
>
> -#define VT8500_TIMER_OFFSET 0x0100
> -#define VT8500_TIMER_HZ 3000000
> -
> #define TIMER_MATCH_REG(x) (4 * (x))
> #define TIMER_COUNT_REG 0x0010 /* clocksource counter */
>
> @@ -53,8 +51,14 @@
> #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>
> #define MIN_OSCR_DELTA 16
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/vt8500-timer.h>
>
> static void __iomem *regbase;
> +static unsigned int sys_timer_ch; /* which match register to use
> + * for the system timer
> + */
The comment format is a bit odd. It would be nicer on top of the
variable.
/*
* Which match register to use for the system timer
*/
> static u64 vt8500_timer_read(struct clocksource *cs)
> {
> @@ -75,21 +79,26 @@ static struct clocksource clocksource = {
> .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> };
>
> +static u64 vt8500_timer_next(u64 cycles)
> +{
> + return clocksource.read(&clocksource) + cycles;
> +}
> +
> static int vt8500_timer_set_next_event(unsigned long cycles,
> struct clock_event_device *evt)
> {
> int loops = msecs_to_loops(10);
> - u64 alarm = clocksource.read(&clocksource) + cycles;
> + u64 alarm = vt8500_timer_next(cycles);
>
> - while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(0)
> + while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(sys_timer_ch)
> && --loops)
> cpu_relax();
> - writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(0));
> + writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(sys_timer_ch));
>
> if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
> return -ETIME;
>
> - writel(TIMER_INT_EN_MATCH(0), regbase + TIMER_INT_EN_REG);
> + writel(TIMER_INT_EN_MATCH(sys_timer_ch), regbase + TIMER_INT_EN_REG);
>
> return 0;
> }
> @@ -131,7 +140,9 @@ static int __init vt8500_timer_init(struct device_node *np)
> return -ENXIO;
> }
>
> - timer_irq = irq_of_parse_and_map(np, 0);
It may be worth to repeat part of what is said in the changelog
> + sys_timer_ch = of_irq_count(np) > 1 ? 1 : 0;
> +
> + timer_irq = irq_of_parse_and_map(np, sys_timer_ch);
> if (!timer_irq) {
> pr_err("%s: Missing irq description in Device Tree\n",
> __func__);
> @@ -140,7 +151,7 @@ static int __init vt8500_timer_init(struct device_node *np)
>
> writel(TIMER_CTRL_ENABLE, regbase + TIMER_CTRL_REG);
> writel(TIMER_STATUS_CLEARALL, regbase + TIMER_STATUS_REG);
> - writel(~0, regbase + TIMER_MATCH_REG(0));
> + writel(~0, regbase + TIMER_MATCH_REG(sys_timer_ch));
>
> ret = clocksource_register_hz(&clocksource, VT8500_TIMER_HZ);
> if (ret) {
> @@ -166,4 +177,86 @@ static int __init vt8500_timer_init(struct device_node *np)
> return 0;
> }
>
> +static void vt8500_timer_aux_uninit(void *data)
> +{
> + auxiliary_device_uninit(data);
> +}
> +
> +static void vt8500_timer_aux_delete(void *data)
> +{
> + auxiliary_device_delete(data);
> +}
> +
> +static void vt8500_timer_aux_release(struct device *dev)
> +{
> + struct auxiliary_device *aux;
> +
> + aux = container_of(dev, struct auxiliary_device, dev);
> + kfree(aux);
That will result in a double kfree because the data belongs to the
wdt_info structure. It is not a pointer allocated. So when the
wdt_info will be freed, it will free the area already freed by this
function.
Please note, a timer should never be unloaded, so not sure if the wdt
should handle the case.
> +}
> +
> +/*
> + * This probe gets called after the timer is already up and running. This will
> + * create the watchdog device as a child since the registers are shared.
> + */
> +static int vt8500_timer_probe(struct platform_device *pdev)
> +{
> + struct vt8500_wdt_info *wdt_info;
> + struct device *dev = &pdev->dev;
> + int ret;
>>>>>
> + if (!sys_timer_ch) {
> + dev_info(dev, "Not enabling watchdog: only one irq was given");
> + return 0;
> + }
> +
> + if (!regbase)
> + return dev_err_probe(dev, -ENOMEM,
> + "Timer not initialized, cannot create watchdog");
The block above seems to be a bit wobbly as it relies on
vt8500_timer_init() to have succeeded.
Why not have vt8500_timer_probe() called by vt8500_timer_init() (with
a proper name like vt8500_timer_wdt_init()) ?
<<<<<
> + wdt_info = kzalloc(sizeof(*wdt_info), GFP_KERNEL);
devm_kzalloc()
> + if (!wdt_info)
> + return dev_err_probe(dev, -ENOMEM,
> + "Failed to allocate vt8500-wdt info");
Is it possible kzalloc to return -EPROBE_DEFER ?
> +
> + wdt_info->timer_next = &vt8500_timer_next;
> + wdt_info->wdt_en = regbase + TIMER_WATCHDOG_EN_REG;
> + wdt_info->wdt_match = regbase + TIMER_MATCH_REG(0);
The two fields above can be merged into one : wdt_info->regbase
Move TIMER_WATCHDOG_EN_REG to the watchdog driver code.
And as TIMER_MATCH_REG(__channel) == 4 * (__channel),
then TIMER_MATCH_REG == 0, so regbase + 0 == regbase
> + wdt_info->auxdev.name = "vt8500-wdt";
> + wdt_info->auxdev.dev.parent = dev;
> + wdt_info->auxdev.dev.release = &vt8500_timer_aux_release;
> +
> + ret = auxiliary_device_init(&wdt_info->auxdev);
> + if (ret) {
> + kfree(wdt_info);
Remove kfree because of devm_kzalloc
> + return ret;
> + }
nit: add line
> + ret = devm_add_action_or_reset(dev, vt8500_timer_aux_uninit,
> + &wdt_info->auxdev);
> + if (ret)
> + return ret;
> +
> + ret = auxiliary_device_add(&wdt_info->auxdev);
> + if (ret)
> + return ret;
nit: add line
> + return devm_add_action_or_reset(dev, vt8500_timer_aux_delete,
> + &wdt_info->auxdev);
> +}
> +
> +static const struct of_device_id vt8500_timer_of_match[] = {
> + { .compatible = "via,vt8500-timer", },
> + {},
> +};
> +
> +static struct platform_driver vt8500_timer_driver = {
> + .probe = vt8500_timer_probe,
> + .driver = {
> + .name = "vt8500-timer",
> + .of_match_table = vt8500_timer_of_match,
> + .suppress_bind_attrs = true,
> + },
> +};
> +
> +builtin_platform_driver(vt8500_timer_driver);
>
>
> TIMER_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
> diff --git a/include/linux/vt8500-timer.h b/include/linux/vt8500-timer.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b8e9000495c509e9c8e8f4098d6bd33de27b3ec4
> --- /dev/null
> +++ b/include/linux/vt8500-timer.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LINUX_VT8500_TIMER_H_
> +#define LINUX_VT8500_TIMER_H_
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/io.h>
> +#include <linux/types.h>
> +
> +#define VT8500_TIMER_HZ 3000000
> +
> +struct vt8500_wdt_info {
> + struct auxiliary_device auxdev;
> + u64 (*timer_next)(u64 cycles);
> + void __iomem *wdt_en;
> + void __iomem *wdt_match;
> +};
> +
> +#endif /* LINUX_VT8500_TIMER_H_ */
>
> --
> 2.49.0
>
--
<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
Powered by blists - more mailing lists