linux-kernel - Re: [PATCH v5 3/3] counter: Add STM based counter

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20260116055708.428641-1-wbg@kernel.org>
Date: Fri, 16 Jan 2026 14:57:06 +0900
From: William Breathitt Gray <wbg@...nel.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: William Breathitt Gray <wbg@...nel.org>,
	Frank.li@....com,
	robh@...nel.org,
	conor+dt@...nel.org,
	krzk+dt@...nel.org,
	s32@....com,
	devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH v5 3/3] counter: Add STM based counter

On Tue, Jan 13, 2026 at 05:52:20PM +0100, Daniel Lezcano wrote:
> The NXP S32G2 automotive platform integrates four Cortex-A53 cores and
> three Cortex-M7 cores, along with a large number of timers and
> counters. These hardware blocks can be used as clocksources or
> clockevents, or as timestamp counters shared across the various
> subsystems running alongside the Linux kernel, such as firmware
> components. Their actual usage depends on the overall platform
> software design.
> 
> In a Linux-based system, the kernel controls the counter, which is a
> read-only shared resource for the other subsystems. One of its primary
> purposes is to act as a common timestamp source for messages or
> traces, allowing correlation of events occurring in different
> operating system contexts.
> 
> These changes introduce a basic counter driver that can start, stop,
> and reset the counter. It also handles overflow accounting and
> configures the prescaler value.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>

Hi Daniel,

A few of the features in this driver are implemented using somewhat of
anti-patterns in the Generic Counter paradigm. In particular, I believe
the overflow and accumulation features would be best implemented using
the Counter events and watches idioms rather than in the Counter
attributes. I'll explain further inline below.

> +static void nxp_stm_cnt_cfg_overflow(struct nxp_stm_cnt *stm_cnt)
> +{
> +	/*
> +	 * The STM does not have a dedicated interrupt when the
> +	 * counter wraps. We need to use the comparator to check if it
> +	 * wrapped or not.
> +	 */
> +	writel(0, STM_CMP0(stm_cnt->base));
> +}

So to implement overflows, you're currently setting up a compare against
0 to fire off interrupts.

The problem, besides the added complexity in your driver, is that users
now lose control over the comparator for other threshold checks. A
better approach is to define a COUNTER_COMP_COMPARE() attribute for this
comparator which users can set to the value they desire, and push
COUNTER_EVENT_THRESHOLD on the interrupts. Userspace can then watch for
these COUNTER_EVENT_THRESHOLD events and use them to detect overflows,
or for any other general purpose now that they can control the
particular threshold value.

> +static irqreturn_t nxp_stm_cnt_irq(int irq, void *dev_id)
> +{
> +	struct counter_device *counter = dev_id;
> +	struct nxp_stm_cnt *stm_cnt = counter_priv(counter);
> +
> +	nxp_stm_cnt_irq_ack(stm_cnt);
> +
> +	atomic_inc(&stm_cnt->nr_wraps);
> +
> +	counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);

Remove nr_wraps and push COUNTER_EVENT_THRESHOLD here instead if you
implement the comparator design I suggested above.

> +static struct counter_comp stm_cnt_count_ext[] = {
> +	COUNTER_COMP_COUNT_U8("prescaler", nxp_stm_cnt_prescaler_read, nxp_stm_cnt_prescaler_write),
> +	COUNTER_COMP_ENABLE(nxp_stm_cnt_count_enable_read, nxp_stm_cnt_count_enable_write),

Add COUNTER_COMP_COMPARE() here for your Counter's comparator.

> +static int nxp_stm_cnt_count_read(struct counter_device *dev,
> +				  struct counter_count *count, u64 *val)
> +{
> +	struct nxp_stm_cnt *stm_cnt = counter_priv(dev);
> +	u32 w1, w2, cnt;
> +
> +	do {
> +		w1 = atomic_read(&stm_cnt->nr_wraps);
> +		cnt = nxp_stm_cnt_get_counter(stm_cnt);
> +		w2 = atomic_read(&stm_cnt->nr_wraps);
> +	} while (w1 != w2);
> +
> +	*val = ((u64)w1 << 32) | cnt;

Just report the raw counter value back directly. If a user wants to
count wraparounds and accumulate the counter, they can do so by setting
a Counter watch in userspace for the COUNTER_EVENT_THRESHOLD event.
Userspace can then keep track of the nr_wraps and perform the
accumulation calculation with the current count value.

> +static int nxp_stm_cnt_watch_validate(struct counter_device *counter,
> +				      const struct counter_watch *watch)
> +{
> +	switch (watch->event) {
> +	case COUNTER_EVENT_OVERFLOW:

This becomes COUNTER_EVENT_THRESHOLD.

> +static const struct counter_ops nxp_stm_cnt_counter_ops = {
> +	.action_read = nxp_stm_cnt_action_read,
> +	.count_read  = nxp_stm_cnt_count_read,
> +	.count_write = nxp_stm_cnt_count_write,
> +	.function_read = nxp_stm_cnt_function_read,
> +	.watch_validate = nxp_stm_cnt_watch_validate,
> +};
> +
> +static struct counter_signal nxp_stm_cnt_signals[] = {
> +	{
> +		.id = 0,
> +		.name = "Counter wrap signal",

Surely this can't be how the signal is described in the datasheet (is
it?). Although you do not have to match the datasheet exactly, the
description should make it obvious which Signal is the source trigger
for the Count state changes. I haven't read the manual for this device,
but I suspect to this signal is named after some sort of clock
oscillator if it's for a timer module; this could be named "System Timer
Module Clock" if the signal is identified simply as that in the manual.

William Breathitt Gray