linux-kernel - Re: [PATCH v4 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: <aWA/ibb7AyA+BBrV@lizhi-Precision-Tower-5810>
Date: Thu, 8 Jan 2026 18:36:41 -0500
From: Frank Li <Frank.li@....com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: wbg@...nel.org, 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 v4 3/3] counter: Add STM based counter

On Thu, Jan 08, 2026 at 06:11:28PM +0100, Daniel Lezcano wrote:
> On 1/8/26 17:41, Frank Li wrote:
> > On Wed, Jan 07, 2026 at 02:39:52PM +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>
> > > ---
>
> [ ... ]
>
> > > +
> > > +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),
> > > +};
> > > +
> > > +static int nxp_stm_cnt_action_read(struct counter_device *counter,
> > > +				   struct counter_count *count,
> > > +				   struct counter_synapse *synapse,
> > > +				   enum counter_synapse_action *action)
> > > +{
> > > +	*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +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);
> > > +	unsigned long irqflags;
> > > +
> > > +	spin_lock_irqsave(&stm_cnt->lock, irqflags);
> > > +	*val = stm_cnt->counter + nxp_stm_cnt_get_counter(stm_cnt);
> > > +	spin_unlock_irqrestore(&stm_cnt->lock, irqflags);
> >
> > It think there are still rise condition here.
> >
> > cpu 0                                            cpu 1
> > 1:  nxp_stm_cnt_get_counter(stm_cnt); (0)
> > 2: 						 irq_handle()
> > 					         counter += INT_MAX;
> > 3:
> >
> > when wrap happen, nxp_stm_cnt_get_counter() return 0, but, irq still not
> > handle yet.
> >
> > so nxp_stm_cnt_count_read() return 0 at 1.
> >
> > it return INT_MAX at 3 suddently.
>
> Thanks for the review. Yes, it seems like there is a race for the reader
> result.
>
> Except disabling / enabling the counter when reading the value, I don't see
> how to handle that case.
>

I think needn't handler overflow at all.
See stm32_count_function_read()

comparor always is u32 val + delta.  it can work if delta less U32_MAX/2.

If you real want to up to 64bit counter, perf counter have common solution
for that

x_counter()
{
	u32 v = readl():
	stm->count += v - stm->last;
	stm->last = v;
}

you need enable irq, make sure at call x_counter() twice during counter
wrap period.

No lock, no stop needed.

Frank

>
>
> bool is_started = nxp_stm_cnt_is_started(stm_cnt);
>
> if (is_started)
> 	nxp_stm_cnt_disable(stm_cnt);
>
> /* If the counter wrapped, the interrupt fired already */
>
> spin_lock_irqsave(&stm_cnt->lock, irqflags);
>
> /* Counter can be zero (or more), we don't care, UINT_MAX was added */
> *val = stm_cnt->counter + nxp_stm_cnt_get_counter(stm_cnt);
>
> spin_unlock_irqrestore(&stm_cnt->lock, irqflags);
>
> if (is_started)
> 	nxp_stm_cnt_enable(stm_cnt);
>
>
>
>
>
> --
> <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