[PATCH v1] counter: interrupt-cnt: add counter_push_event()

[PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Oleksij Rempel]

Add counter_push_event() to notify user space about new pulses

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/counter/interrupt-cnt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 8514a87fcbee..b237137b552b 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
 
 	atomic_inc(&priv->count);
 
+	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.30.2

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
> Add counter_push_event() to notify user space about new pulses
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/counter/interrupt-cnt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> index 8514a87fcbee..b237137b552b 100644
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
>  
>  	atomic_inc(&priv->count);
>  
> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> +
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.30.2

Hi Oleksij,

It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
an interrupt is handled, which I suspect is not what you want to happen.
The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
so you'll need to check for a count value overflow before pushing the
event.

It would be good idea to implement a ceiling extension as well (you can
use the COUNTER_COMP_CEILING() macro) so that users can configure the
particular point where the value overflows.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Oleksij Rempel]

Hi William,

On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:
> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
> > Add counter_push_event() to notify user space about new pulses
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/counter/interrupt-cnt.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > index 8514a87fcbee..b237137b552b 100644
> > --- a/drivers/counter/interrupt-cnt.c
> > +++ b/drivers/counter/interrupt-cnt.c
> > @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> >  
> >  	atomic_inc(&priv->count);
> >  
> > +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > +
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -- 
> > 2.30.2
> 
> Hi Oleksij,
> 
> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> an interrupt is handled, which I suspect is not what you want to happen.
> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> so you'll need to check for a count value overflow before pushing the
> event.
> 
> It would be good idea to implement a ceiling extension as well (you can
> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> particular point where the value overflows.

Thank you!

What would be the best and resource effective strategy for periodically
getting frequency of interrupts/pulses? This is actual information which is
needed for my use case.

So far, I was pushing every event to the user space, which is working
but probably not the most resource effective method of doing it.

Regards,
Oleskij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:
> Hi William,
> 
> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:
> > On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
> > > Add counter_push_event() to notify user space about new pulses
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  drivers/counter/interrupt-cnt.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > index 8514a87fcbee..b237137b552b 100644
> > > --- a/drivers/counter/interrupt-cnt.c
> > > +++ b/drivers/counter/interrupt-cnt.c
> > > @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > >  
> > >  	atomic_inc(&priv->count);
> > >  
> > > +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > +
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > -- 
> > > 2.30.2
> > 
> > Hi Oleksij,
> > 
> > It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > an interrupt is handled, which I suspect is not what you want to happen.
> > The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > so you'll need to check for a count value overflow before pushing the
> > event.
> > 
> > It would be good idea to implement a ceiling extension as well (you can
> > use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > particular point where the value overflows.
> 
> Thank you!
> 
> What would be the best and resource effective strategy for periodically
> getting frequency of interrupts/pulses? This is actual information which is
> needed for my use case.
> 
> So far, I was pushing every event to the user space, which is working
> but probably not the most resource effective method of doing it.
> 
> Regards,
> Oleskij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

We could introduce a new Counter change-of-state event type which would
trigger whenever the count value changes, but I agree with you that this
is likely not the best way for us to derive the frequency of the
interrupts due to the indirection of handling and parsing the event
data.

Instead, perhaps introducing a "frequency" or "period" Count extension
would make more sense here. This extension could report the value delta
between counts, or alternatively the time delta from which you can
derive frequency. Regarding implementation, you can store the previous
value in a variable, updating it whenever an interrupt occurs, and
compute the particular delta every time a read is requested by the user.

David Lechner is implementing something similar for the TI eQEP driver
to expose speed, so I'm CCing them here in case this is of interest to
them.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Jander]

On Thu, 25 Nov 2021 10:58:23 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:
> > Hi William,
> > 
> > On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:  
> > > On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:  
> > > > Add counter_push_event() to notify user space about new pulses
> > > > 
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > ---
> > > >  drivers/counter/interrupt-cnt.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > > index 8514a87fcbee..b237137b552b 100644
> > > > --- a/drivers/counter/interrupt-cnt.c
> > > > +++ b/drivers/counter/interrupt-cnt.c
> > > > @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > > >  
> > > >  	atomic_inc(&priv->count);
> > > >  
> > > > +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > > +
> > > >  	return IRQ_HANDLED;
> > > >  }
> > > >  
> > > > -- 
> > > > 2.30.2  
> > > 
> > > Hi Oleksij,
> > > 
> > > It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > > an interrupt is handled, which I suspect is not what you want to happen.
> > > The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > > so you'll need to check for a count value overflow before pushing the
> > > event.
> > > 
> > > It would be good idea to implement a ceiling extension as well (you can
> > > use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > > particular point where the value overflows.  
> > 
> > Thank you!
> > 
> > What would be the best and resource effective strategy for periodically
> > getting frequency of interrupts/pulses? This is actual information which is
> > needed for my use case.
> > 
> > So far, I was pushing every event to the user space, which is working
> > but probably not the most resource effective method of doing it.
> > 
> > Regards,
> > Oleskij
> > -- 
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |  
> 
> We could introduce a new Counter change-of-state event type which would
> trigger whenever the count value changes, but I agree with you that this
> is likely not the best way for us to derive the frequency of the
> interrupts due to the indirection of handling and parsing the event
> data.

Also something I am worried about, is the overhead it creates to generate such
an event on each and every IRQ. Looking at counter_push_event(), I can see it
using a spin-lock, besides quite a bit of code potentially being executed,
depending on user-space. The lock can probably be held by non-IRQ code
also, which can potentially introduce more latency and cause high-frequency
interrupts to be delayed far too long. This particular driver uses
atomic_inc() to increment a counter, which AFAIK on most machines should be a
single instruction. The main application for this driver is to count pulses
_fast_ with minimal CPU load. IMHO we should do better than potentially
blocking on a spin-lock in IRQ context.
I know this is akin to trying to do hard-real-time stuff in the kernel, but
since its main application is for embedded systems that have a known and
controllable interrupt environment most of the time, this can be done if one
is careful to not do certain things in IRQ context, such as using locks.

> Instead, perhaps introducing a "frequency" or "period" Count extension
> would make more sense here. This extension could report the value delta
> between counts, or alternatively the time delta from which you can
> derive frequency. Regarding implementation, you can store the previous
> value in a variable, updating it whenever an interrupt occurs, and
> compute the particular delta every time a read is requested by the user.

The original version of this driver used a circular buffer that stored the
timestamps of the last 'n' interrupts. A user-space read action would copy this
buffer repeatedly (max tries --> fail) until two copies are identical to
ensure integrity avoiding the use of locks. This is of course dead ugly and I
was hoping for a better solution. But to be better IMHO it must avoid locks in
IRQ context at all costs.
Having a sample 'n' consecutive of time-stamps in user-space, made frequency
calculation, filtering and glitch detection quite simple.

> David Lechner is implementing something similar for the TI eQEP driver
> to expose speed, so I'm CCing them here in case this is of interest to
> them.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Lechner]

On 11/24/21 7:58 PM, William Breathitt Gray wrote:
> On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:
>> Hi William,
>>
>> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:
>>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
>>>> Add counter_push_event() to notify user space about new pulses
>>>>
>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>> ---
>>>>   drivers/counter/interrupt-cnt.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
>>>> index 8514a87fcbee..b237137b552b 100644
>>>> --- a/drivers/counter/interrupt-cnt.c
>>>> +++ b/drivers/counter/interrupt-cnt.c
>>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
>>>>   
>>>>   	atomic_inc(&priv->count);
>>>>   
>>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
>>>> +
>>>>   	return IRQ_HANDLED;
>>>>   }
>>>>   
>>>> -- 
>>>> 2.30.2
>>>
>>> Hi Oleksij,
>>>
>>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
>>> an interrupt is handled, which I suspect is not what you want to happen.
>>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
>>> so you'll need to check for a count value overflow before pushing the
>>> event.
>>>
>>> It would be good idea to implement a ceiling extension as well (you can
>>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
>>> particular point where the value overflows.
>>
>> Thank you!
>>
>> What would be the best and resource effective strategy for periodically
>> getting frequency of interrupts/pulses? This is actual information which is
>> needed for my use case.
>>
>> So far, I was pushing every event to the user space, which is working
>> but probably not the most resource effective method of doing it.
>>
>> Regards,
>> Oleskij
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> We could introduce a new Counter change-of-state event type which would
> trigger whenever the count value changes, but I agree with you that this
> is likely not the best way for us to derive the frequency of the
> interrupts due to the indirection of handling and parsing the event
> data.
> 
> Instead, perhaps introducing a "frequency" or "period" Count extension
> would make more sense here. This extension could report the value delta
> between counts, or alternatively the time delta from which you can
> derive frequency. Regarding implementation, you can store the previous
> value in a variable, updating it whenever an interrupt occurs, and
> compute the particular delta every time a read is requested by the user.
> 
> David Lechner is implementing something similar for the TI eQEP driver
> to expose speed, so I'm CCing them here in case this is of interest to
> them.
> 

Based on my experience, I would recommend that counter drivers be as
"thin" as possible. They shouldn't try to provide any information that
the hardware itself doesn't provide. In other words, the kernel should
provide userspace the information needed to calculate the speed/rate
but not try to do the actual calculation in the kernel. Inevitably
there are nuances for specific use cases that can't all possibly be
handled by such an implementation.

I've tried using gpio interrupts to try to calculate speed/rate in
the kernel before and it simply doesn't work reliably. Interrupts
get missed and the calculation will be off.

For really slow counts (i.e. 1 count/second), I can see a use for
generating an event on each count though. For high rates, I would
just read the count every 100ms in usespace and divide the change in
the number of counts by the time period to get the rate.

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Jander]

Hi David,

On Mon, 6 Dec 2021 13:24:18 -0600
David Lechner <david@lechnology.com> wrote:

> On 11/24/21 7:58 PM, William Breathitt Gray wrote:
> > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:  
> >> Hi William,
> >>
> >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:  
> >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:  
> >>>> Add counter_push_event() to notify user space about new pulses
> >>>>
> >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>> ---
> >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> >>>>   1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> >>>> index 8514a87fcbee..b237137b552b 100644
> >>>> --- a/drivers/counter/interrupt-cnt.c
> >>>> +++ b/drivers/counter/interrupt-cnt.c
> >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> >>>>   
> >>>>   	atomic_inc(&priv->count);
> >>>>   
> >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> >>>> +
> >>>>   	return IRQ_HANDLED;
> >>>>   }
> >>>>   
> >>>> -- 
> >>>> 2.30.2  
> >>>
> >>> Hi Oleksij,
> >>>
> >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> >>> an interrupt is handled, which I suspect is not what you want to happen.
> >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> >>> so you'll need to check for a count value overflow before pushing the
> >>> event.
> >>>
> >>> It would be good idea to implement a ceiling extension as well (you can
> >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> >>> particular point where the value overflows.  
> >>
> >> Thank you!
> >>
> >> What would be the best and resource effective strategy for periodically
> >> getting frequency of interrupts/pulses? This is actual information which is
> >> needed for my use case.
> >>
> >> So far, I was pushing every event to the user space, which is working
> >> but probably not the most resource effective method of doing it.
> >>
> >> Regards,
> >> Oleskij
> >> -- 
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |  
> > 
> > We could introduce a new Counter change-of-state event type which would
> > trigger whenever the count value changes, but I agree with you that this
> > is likely not the best way for us to derive the frequency of the
> > interrupts due to the indirection of handling and parsing the event
> > data.
> > 
> > Instead, perhaps introducing a "frequency" or "period" Count extension
> > would make more sense here. This extension could report the value delta
> > between counts, or alternatively the time delta from which you can
> > derive frequency. Regarding implementation, you can store the previous
> > value in a variable, updating it whenever an interrupt occurs, and
> > compute the particular delta every time a read is requested by the user.
> > 
> > David Lechner is implementing something similar for the TI eQEP driver
> > to expose speed, so I'm CCing them here in case this is of interest to
> > them.
> >   
> 
> Based on my experience, I would recommend that counter drivers be as
> "thin" as possible. They shouldn't try to provide any information that
> the hardware itself doesn't provide. In other words, the kernel should
> provide userspace the information needed to calculate the speed/rate
> but not try to do the actual calculation in the kernel. Inevitably
> there are nuances for specific use cases that can't all possibly be
> handled by such an implementation.

I completely agree with this. While interrupts aren't really meant for
measuring frequency, and this being somewhat of a mis-use of something, it is
still possible to do and very useful in many cases. That said, while the
counter framework is AFAIK the best fit for this, the main use-case for this
driver is measuring wheel speed (and similar "speeds"). For this, the minimum
amount of information the driver needs to provide user-space with to do
reliable calculations, is high-resolution time-stamps of GPIO events. A simple
counter is not suited, because there can be glitches that need to be detected.
If user-space gets a buffer full of consecutive time-stamps (don't need to be
all of them, just a sample of n consecutive timestamps), as well as total
count, all needed calculations, glitch filtering, low-pass filtering, etc...
can be done in user-space just fine.

> I've tried using gpio interrupts to try to calculate speed/rate in
> the kernel before and it simply doesn't work reliably. Interrupts
> get missed and the calculation will be off.

Exactly. Been there, done that.
For reliable speed calculations of a mechanical system, the properties of the
mechanical system need to be known, like physical limits of accelerations,
maximum (or minimum) speed, etc. The minimum set of input data needed by a
user-space application to do these calculations is total pulse count in
addition to a buffer of timestamps of n consecutive input events (raising or
falling edges on GPIO). So IMHO this is what the driver should provide, and
in the most resource-efficient way possible. This particular driver will be
used 3 times on the same SoC, with each up to 10-15k pulses per second. That
is a lot of interrupts for an embedded system, so they better consume as
little resources as possible. Filling a ring buffer with timestamps should be
possible, as long as no locking is involved. Locks in IRQ context must be
avoided at all costs, specially in this case.

> For really slow counts (i.e. 1 count/second), I can see a use for
> generating an event on each count though. For high rates, I would
> just read the count every 100ms in usespace and divide the change in
> the number of counts by the time period to get the rate.

For slow counts, I agree, but for high rates, I don't (see above). There can
be glitches and false events that can (and must) be effectively filtered out.
For that user-space needs to know the time of each event during the
measurement period.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7329 bytes --]

Hello David,

On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:
> On Mon, 6 Dec 2021 13:24:18 -0600
> David Lechner <david@lechnology.com> wrote:
> 
> > On 11/24/21 7:58 PM, William Breathitt Gray wrote:
> > > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:  
> > >> Hi William,
> > >>
> > >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:  
> > >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:  
> > >>>> Add counter_push_event() to notify user space about new pulses
> > >>>>
> > >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > >>>> ---
> > >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> > >>>>   1 file changed, 2 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > >>>> index 8514a87fcbee..b237137b552b 100644
> > >>>> --- a/drivers/counter/interrupt-cnt.c
> > >>>> +++ b/drivers/counter/interrupt-cnt.c
> > >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > >>>>   
> > >>>>   	atomic_inc(&priv->count);
> > >>>>   
> > >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > >>>> +
> > >>>>   	return IRQ_HANDLED;
> > >>>>   }
> > >>>>   
> > >>>> -- 
> > >>>> 2.30.2  
> > >>>
> > >>> Hi Oleksij,
> > >>>
> > >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > >>> an interrupt is handled, which I suspect is not what you want to happen.
> > >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > >>> so you'll need to check for a count value overflow before pushing the
> > >>> event.
> > >>>
> > >>> It would be good idea to implement a ceiling extension as well (you can
> > >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > >>> particular point where the value overflows.  
> > >>
> > >> Thank you!
> > >>
> > >> What would be the best and resource effective strategy for periodically
> > >> getting frequency of interrupts/pulses? This is actual information which is
> > >> needed for my use case.
> > >>
> > >> So far, I was pushing every event to the user space, which is working
> > >> but probably not the most resource effective method of doing it.
> > >>
> > >> Regards,
> > >> Oleskij
> > >> -- 
> > >> Pengutronix e.K.                           |                             |
> > >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |  
> > > 
> > > We could introduce a new Counter change-of-state event type which would
> > > trigger whenever the count value changes, but I agree with you that this
> > > is likely not the best way for us to derive the frequency of the
> > > interrupts due to the indirection of handling and parsing the event
> > > data.
> > > 
> > > Instead, perhaps introducing a "frequency" or "period" Count extension
> > > would make more sense here. This extension could report the value delta
> > > between counts, or alternatively the time delta from which you can
> > > derive frequency. Regarding implementation, you can store the previous
> > > value in a variable, updating it whenever an interrupt occurs, and
> > > compute the particular delta every time a read is requested by the user.
> > > 
> > > David Lechner is implementing something similar for the TI eQEP driver
> > > to expose speed, so I'm CCing them here in case this is of interest to
> > > them.
> > >   
> > 
> > Based on my experience, I would recommend that counter drivers be as
> > "thin" as possible. They shouldn't try to provide any information that
> > the hardware itself doesn't provide. In other words, the kernel should
> > provide userspace the information needed to calculate the speed/rate
> > but not try to do the actual calculation in the kernel. Inevitably
> > there are nuances for specific use cases that can't all possibly be
> > handled by such an implementation.
> 
> I completely agree with this. While interrupts aren't really meant for
> measuring frequency, and this being somewhat of a mis-use of something, it is
> still possible to do and very useful in many cases. That said, while the
> counter framework is AFAIK the best fit for this, the main use-case for this
> driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> amount of information the driver needs to provide user-space with to do
> reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> counter is not suited, because there can be glitches that need to be detected.
> If user-space gets a buffer full of consecutive time-stamps (don't need to be
> all of them, just a sample of n consecutive timestamps), as well as total
> count, all needed calculations, glitch filtering, low-pass filtering, etc...
> can be done in user-space just fine.
> 
> > I've tried using gpio interrupts to try to calculate speed/rate in
> > the kernel before and it simply doesn't work reliably. Interrupts
> > get missed and the calculation will be off.
> 
> Exactly. Been there, done that.
> For reliable speed calculations of a mechanical system, the properties of the
> mechanical system need to be known, like physical limits of accelerations,
> maximum (or minimum) speed, etc. The minimum set of input data needed by a
> user-space application to do these calculations is total pulse count in
> addition to a buffer of timestamps of n consecutive input events (raising or
> falling edges on GPIO). So IMHO this is what the driver should provide, and
> in the most resource-efficient way possible. This particular driver will be
> used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> is a lot of interrupts for an embedded system, so they better consume as
> little resources as possible. Filling a ring buffer with timestamps should be
> possible, as long as no locking is involved. Locks in IRQ context must be
> avoided at all costs, specially in this case.
> 
> > For really slow counts (i.e. 1 count/second), I can see a use for
> > generating an event on each count though. For high rates, I would
> > just read the count every 100ms in usespace and divide the change in
> > the number of counts by the time period to get the rate.
> 
> For slow counts, I agree, but for high rates, I don't (see above). There can
> be glitches and false events that can (and must) be effectively filtered out.
> For that user-space needs to know the time of each event during the
> measurement period.

No sure I understood the problem here. If you keep the driver as is and
in userspace just read out the counter value twice and measure the time
between the reads[1], you can calculate the average frequency of the
event in userspace.

Isn't that good enough?

Best regards
Uwe

[1] maybe support this timing by providing a timestamp with the read
    value to reduce timing jitter.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Jander]

Dear Uwe,

On Wed, 8 Dec 2021 14:59:02 +0100
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Hello David,
> 
> On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:
> > On Mon, 6 Dec 2021 13:24:18 -0600
> > David Lechner <david@lechnology.com> wrote:
> >   
> > > On 11/24/21 7:58 PM, William Breathitt Gray wrote:  
> > > > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:    
> > > >> Hi William,
> > > >>
> > > >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:    
> > > >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:    
> > > >>>> Add counter_push_event() to notify user space about new pulses
> > > >>>>
> > > >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > >>>> ---
> > > >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> > > >>>>   1 file changed, 2 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > >>>> index 8514a87fcbee..b237137b552b 100644
> > > >>>> --- a/drivers/counter/interrupt-cnt.c
> > > >>>> +++ b/drivers/counter/interrupt-cnt.c
> > > >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > > >>>>   
> > > >>>>   	atomic_inc(&priv->count);
> > > >>>>   
> > > >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > >>>> +
> > > >>>>   	return IRQ_HANDLED;
> > > >>>>   }
> > > >>>>   
> > > >>>> -- 
> > > >>>> 2.30.2    
> > > >>>
> > > >>> Hi Oleksij,
> > > >>>
> > > >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > > >>> an interrupt is handled, which I suspect is not what you want to happen.
> > > >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > > >>> so you'll need to check for a count value overflow before pushing the
> > > >>> event.
> > > >>>
> > > >>> It would be good idea to implement a ceiling extension as well (you can
> > > >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > > >>> particular point where the value overflows.    
> > > >>
> > > >> Thank you!
> > > >>
> > > >> What would be the best and resource effective strategy for periodically
> > > >> getting frequency of interrupts/pulses? This is actual information which is
> > > >> needed for my use case.
> > > >>
> > > >> So far, I was pushing every event to the user space, which is working
> > > >> but probably not the most resource effective method of doing it.
> > > >>
> > > >> Regards,
> > > >> Oleskij
> > > >> -- 
> > > >> Pengutronix e.K.                           |                             |
> > > >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |    
> > > > 
> > > > We could introduce a new Counter change-of-state event type which would
> > > > trigger whenever the count value changes, but I agree with you that this
> > > > is likely not the best way for us to derive the frequency of the
> > > > interrupts due to the indirection of handling and parsing the event
> > > > data.
> > > > 
> > > > Instead, perhaps introducing a "frequency" or "period" Count extension
> > > > would make more sense here. This extension could report the value delta
> > > > between counts, or alternatively the time delta from which you can
> > > > derive frequency. Regarding implementation, you can store the previous
> > > > value in a variable, updating it whenever an interrupt occurs, and
> > > > compute the particular delta every time a read is requested by the user.
> > > > 
> > > > David Lechner is implementing something similar for the TI eQEP driver
> > > > to expose speed, so I'm CCing them here in case this is of interest to
> > > > them.
> > > >     
> > > 
> > > Based on my experience, I would recommend that counter drivers be as
> > > "thin" as possible. They shouldn't try to provide any information that
> > > the hardware itself doesn't provide. In other words, the kernel should
> > > provide userspace the information needed to calculate the speed/rate
> > > but not try to do the actual calculation in the kernel. Inevitably
> > > there are nuances for specific use cases that can't all possibly be
> > > handled by such an implementation.  
> > 
> > I completely agree with this. While interrupts aren't really meant for
> > measuring frequency, and this being somewhat of a mis-use of something, it is
> > still possible to do and very useful in many cases. That said, while the
> > counter framework is AFAIK the best fit for this, the main use-case for this
> > driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> > amount of information the driver needs to provide user-space with to do
> > reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> > counter is not suited, because there can be glitches that need to be detected.
> > If user-space gets a buffer full of consecutive time-stamps (don't need to be
> > all of them, just a sample of n consecutive timestamps), as well as total
> > count, all needed calculations, glitch filtering, low-pass filtering, etc...
> > can be done in user-space just fine.
> >   
> > > I've tried using gpio interrupts to try to calculate speed/rate in
> > > the kernel before and it simply doesn't work reliably. Interrupts
> > > get missed and the calculation will be off.  
> > 
> > Exactly. Been there, done that.
> > For reliable speed calculations of a mechanical system, the properties of the
> > mechanical system need to be known, like physical limits of accelerations,
> > maximum (or minimum) speed, etc. The minimum set of input data needed by a
> > user-space application to do these calculations is total pulse count in
> > addition to a buffer of timestamps of n consecutive input events (raising or
> > falling edges on GPIO). So IMHO this is what the driver should provide, and
> > in the most resource-efficient way possible. This particular driver will be
> > used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> > is a lot of interrupts for an embedded system, so they better consume as
> > little resources as possible. Filling a ring buffer with timestamps should be
> > possible, as long as no locking is involved. Locks in IRQ context must be
> > avoided at all costs, specially in this case.
> >   
> > > For really slow counts (i.e. 1 count/second), I can see a use for
> > > generating an event on each count though. For high rates, I would
> > > just read the count every 100ms in usespace and divide the change in
> > > the number of counts by the time period to get the rate.  
> > 
> > For slow counts, I agree, but for high rates, I don't (see above). There can
> > be glitches and false events that can (and must) be effectively filtered out.
> > For that user-space needs to know the time of each event during the
> > measurement period.  
> 
> No sure I understood the problem here. If you keep the driver as is and
> in userspace just read out the counter value twice and measure the time
> between the reads[1], you can calculate the average frequency of the
> event in userspace.
> 
> Isn't that good enough?

No, I'm afraid it isn't, for two reasons:

1. These counters are often used in environments, where glitches can and do
happen. So sometimes there are fake events. The only way to tell fake from
real pulses, is to filter them. Unfortunately you need the timestamps of each
event for that.

2. Another reason for having time-stamps is the case when the frequency is low
and one still requires fast accurate measurements. In that case timestamps
provide a way of accurately measuring period time.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 9115 bytes --]

On Wed, Dec 08, 2021 at 05:10:35PM +0100, David Jander wrote:
> 
> Dear Uwe,
> 
> On Wed, 8 Dec 2021 14:59:02 +0100
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Hello David,
> > 
> > On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:
> > > On Mon, 6 Dec 2021 13:24:18 -0600
> > > David Lechner <david@lechnology.com> wrote:
> > >   
> > > > On 11/24/21 7:58 PM, William Breathitt Gray wrote:  
> > > > > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:    
> > > > >> Hi William,
> > > > >>
> > > > >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:    
> > > > >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:    
> > > > >>>> Add counter_push_event() to notify user space about new pulses
> > > > >>>>
> > > > >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > >>>> ---
> > > > >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> > > > >>>>   1 file changed, 2 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > > >>>> index 8514a87fcbee..b237137b552b 100644
> > > > >>>> --- a/drivers/counter/interrupt-cnt.c
> > > > >>>> +++ b/drivers/counter/interrupt-cnt.c
> > > > >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > > > >>>>   
> > > > >>>>   	atomic_inc(&priv->count);
> > > > >>>>   
> > > > >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > > >>>> +
> > > > >>>>   	return IRQ_HANDLED;
> > > > >>>>   }
> > > > >>>>   
> > > > >>>> -- 
> > > > >>>> 2.30.2    
> > > > >>>
> > > > >>> Hi Oleksij,
> > > > >>>
> > > > >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > > > >>> an interrupt is handled, which I suspect is not what you want to happen.
> > > > >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > > > >>> so you'll need to check for a count value overflow before pushing the
> > > > >>> event.
> > > > >>>
> > > > >>> It would be good idea to implement a ceiling extension as well (you can
> > > > >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > > > >>> particular point where the value overflows.    
> > > > >>
> > > > >> Thank you!
> > > > >>
> > > > >> What would be the best and resource effective strategy for periodically
> > > > >> getting frequency of interrupts/pulses? This is actual information which is
> > > > >> needed for my use case.
> > > > >>
> > > > >> So far, I was pushing every event to the user space, which is working
> > > > >> but probably not the most resource effective method of doing it.
> > > > >>
> > > > >> Regards,
> > > > >> Oleskij
> > > > >> -- 
> > > > >> Pengutronix e.K.                           |                             |
> > > > >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > > >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > > >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |    
> > > > > 
> > > > > We could introduce a new Counter change-of-state event type which would
> > > > > trigger whenever the count value changes, but I agree with you that this
> > > > > is likely not the best way for us to derive the frequency of the
> > > > > interrupts due to the indirection of handling and parsing the event
> > > > > data.
> > > > > 
> > > > > Instead, perhaps introducing a "frequency" or "period" Count extension
> > > > > would make more sense here. This extension could report the value delta
> > > > > between counts, or alternatively the time delta from which you can
> > > > > derive frequency. Regarding implementation, you can store the previous
> > > > > value in a variable, updating it whenever an interrupt occurs, and
> > > > > compute the particular delta every time a read is requested by the user.
> > > > > 
> > > > > David Lechner is implementing something similar for the TI eQEP driver
> > > > > to expose speed, so I'm CCing them here in case this is of interest to
> > > > > them.
> > > > >     
> > > > 
> > > > Based on my experience, I would recommend that counter drivers be as
> > > > "thin" as possible. They shouldn't try to provide any information that
> > > > the hardware itself doesn't provide. In other words, the kernel should
> > > > provide userspace the information needed to calculate the speed/rate
> > > > but not try to do the actual calculation in the kernel. Inevitably
> > > > there are nuances for specific use cases that can't all possibly be
> > > > handled by such an implementation.  
> > > 
> > > I completely agree with this. While interrupts aren't really meant for
> > > measuring frequency, and this being somewhat of a mis-use of something, it is
> > > still possible to do and very useful in many cases. That said, while the
> > > counter framework is AFAIK the best fit for this, the main use-case for this
> > > driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> > > amount of information the driver needs to provide user-space with to do
> > > reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> > > counter is not suited, because there can be glitches that need to be detected.
> > > If user-space gets a buffer full of consecutive time-stamps (don't need to be
> > > all of them, just a sample of n consecutive timestamps), as well as total
> > > count, all needed calculations, glitch filtering, low-pass filtering, etc...
> > > can be done in user-space just fine.
> > >   
> > > > I've tried using gpio interrupts to try to calculate speed/rate in
> > > > the kernel before and it simply doesn't work reliably. Interrupts
> > > > get missed and the calculation will be off.  
> > > 
> > > Exactly. Been there, done that.
> > > For reliable speed calculations of a mechanical system, the properties of the
> > > mechanical system need to be known, like physical limits of accelerations,
> > > maximum (or minimum) speed, etc. The minimum set of input data needed by a
> > > user-space application to do these calculations is total pulse count in
> > > addition to a buffer of timestamps of n consecutive input events (raising or
> > > falling edges on GPIO). So IMHO this is what the driver should provide, and
> > > in the most resource-efficient way possible. This particular driver will be
> > > used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> > > is a lot of interrupts for an embedded system, so they better consume as
> > > little resources as possible. Filling a ring buffer with timestamps should be
> > > possible, as long as no locking is involved. Locks in IRQ context must be
> > > avoided at all costs, specially in this case.
> > >   
> > > > For really slow counts (i.e. 1 count/second), I can see a use for
> > > > generating an event on each count though. For high rates, I would
> > > > just read the count every 100ms in usespace and divide the change in
> > > > the number of counts by the time period to get the rate.  
> > > 
> > > For slow counts, I agree, but for high rates, I don't (see above). There can
> > > be glitches and false events that can (and must) be effectively filtered out.
> > > For that user-space needs to know the time of each event during the
> > > measurement period.  
> > 
> > No sure I understood the problem here. If you keep the driver as is and
> > in userspace just read out the counter value twice and measure the time
> > between the reads[1], you can calculate the average frequency of the
> > event in userspace.
> > 
> > Isn't that good enough?
> 
> No, I'm afraid it isn't, for two reasons:
> 
> 1. These counters are often used in environments, where glitches can and do
> happen. So sometimes there are fake events. The only way to tell fake from
> real pulses, is to filter them. Unfortunately you need the timestamps of each
> event for that.
> 
> 2. Another reason for having time-stamps is the case when the frequency is low
> and one still requires fast accurate measurements. In that case timestamps
> provide a way of accurately measuring period time.
> 
> Best regards,
> 
> -- 
> David Jander
> Protonic Holland.

Keeping drivers focused on just exposing the hardware data and
functionality is likely the best path to choose, so my earlier
suggestion of a "frequency" extension would better be left for userspace
to handle.

So in order to enable userspace to derive frequency, you need reliable
timestamps for enough consecutive events to provide an adequate size
sample of data on which to perform filtering and other such operations.

If we add a COUNTER_EVENT_CHANGE_OF_STATE or similar, every count change
will generate an event with a logged timestamp. Is the problem with this
solution primarily that the Counter event queue is currently utilizing
spinlocks for synchronization?

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Jander]

Dear William,

On Wed, 15 Dec 2021 17:48:26 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Wed, Dec 08, 2021 at 05:10:35PM +0100, David Jander wrote:
> > 
> > Dear Uwe,
> > 
> > On Wed, 8 Dec 2021 14:59:02 +0100
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >   
> > > Hello David,
> > > 
> > > On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:  
> > > > On Mon, 6 Dec 2021 13:24:18 -0600
> > > > David Lechner <david@lechnology.com> wrote:
> > > >     
> > > > > On 11/24/21 7:58 PM, William Breathitt Gray wrote:    
> > > > > > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:      
> > > > > >> Hi William,
> > > > > >>
> > > > > >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:      
> > > > > >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:      
> > > > > >>>> Add counter_push_event() to notify user space about new pulses
> > > > > >>>>
> > > > > >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > >>>> ---
> > > > > >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> > > > > >>>>   1 file changed, 2 insertions(+)
> > > > > >>>>
> > > > > >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > > > >>>> index 8514a87fcbee..b237137b552b 100644
> > > > > >>>> --- a/drivers/counter/interrupt-cnt.c
> > > > > >>>> +++ b/drivers/counter/interrupt-cnt.c
> > > > > >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > > > > >>>>   
> > > > > >>>>   	atomic_inc(&priv->count);
> > > > > >>>>   
> > > > > >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > > > >>>> +
> > > > > >>>>   	return IRQ_HANDLED;
> > > > > >>>>   }
> > > > > >>>>   
> > > > > >>>> -- 
> > > > > >>>> 2.30.2      
> > > > > >>>
> > > > > >>> Hi Oleksij,
> > > > > >>>
> > > > > >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > > > > >>> an interrupt is handled, which I suspect is not what you want to happen.
> > > > > >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > > > > >>> so you'll need to check for a count value overflow before pushing the
> > > > > >>> event.
> > > > > >>>
> > > > > >>> It would be good idea to implement a ceiling extension as well (you can
> > > > > >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > > > > >>> particular point where the value overflows.      
> > > > > >>
> > > > > >> Thank you!
> > > > > >>
> > > > > >> What would be the best and resource effective strategy for periodically
> > > > > >> getting frequency of interrupts/pulses? This is actual information which is
> > > > > >> needed for my use case.
> > > > > >>
> > > > > >> So far, I was pushing every event to the user space, which is working
> > > > > >> but probably not the most resource effective method of doing it.
> > > > > >>
> > > > > >> Regards,
> > > > > >> Oleskij
> > > > > >> -- 
> > > > > >> Pengutronix e.K.                           |                             |
> > > > > >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > > > >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > > > >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |      
> > > > > > 
> > > > > > We could introduce a new Counter change-of-state event type which would
> > > > > > trigger whenever the count value changes, but I agree with you that this
> > > > > > is likely not the best way for us to derive the frequency of the
> > > > > > interrupts due to the indirection of handling and parsing the event
> > > > > > data.
> > > > > > 
> > > > > > Instead, perhaps introducing a "frequency" or "period" Count extension
> > > > > > would make more sense here. This extension could report the value delta
> > > > > > between counts, or alternatively the time delta from which you can
> > > > > > derive frequency. Regarding implementation, you can store the previous
> > > > > > value in a variable, updating it whenever an interrupt occurs, and
> > > > > > compute the particular delta every time a read is requested by the user.
> > > > > > 
> > > > > > David Lechner is implementing something similar for the TI eQEP driver
> > > > > > to expose speed, so I'm CCing them here in case this is of interest to
> > > > > > them.
> > > > > >       
> > > > > 
> > > > > Based on my experience, I would recommend that counter drivers be as
> > > > > "thin" as possible. They shouldn't try to provide any information that
> > > > > the hardware itself doesn't provide. In other words, the kernel should
> > > > > provide userspace the information needed to calculate the speed/rate
> > > > > but not try to do the actual calculation in the kernel. Inevitably
> > > > > there are nuances for specific use cases that can't all possibly be
> > > > > handled by such an implementation.    
> > > > 
> > > > I completely agree with this. While interrupts aren't really meant for
> > > > measuring frequency, and this being somewhat of a mis-use of something, it is
> > > > still possible to do and very useful in many cases. That said, while the
> > > > counter framework is AFAIK the best fit for this, the main use-case for this
> > > > driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> > > > amount of information the driver needs to provide user-space with to do
> > > > reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> > > > counter is not suited, because there can be glitches that need to be detected.
> > > > If user-space gets a buffer full of consecutive time-stamps (don't need to be
> > > > all of them, just a sample of n consecutive timestamps), as well as total
> > > > count, all needed calculations, glitch filtering, low-pass filtering, etc...
> > > > can be done in user-space just fine.
> > > >     
> > > > > I've tried using gpio interrupts to try to calculate speed/rate in
> > > > > the kernel before and it simply doesn't work reliably. Interrupts
> > > > > get missed and the calculation will be off.    
> > > > 
> > > > Exactly. Been there, done that.
> > > > For reliable speed calculations of a mechanical system, the properties of the
> > > > mechanical system need to be known, like physical limits of accelerations,
> > > > maximum (or minimum) speed, etc. The minimum set of input data needed by a
> > > > user-space application to do these calculations is total pulse count in
> > > > addition to a buffer of timestamps of n consecutive input events (raising or
> > > > falling edges on GPIO). So IMHO this is what the driver should provide, and
> > > > in the most resource-efficient way possible. This particular driver will be
> > > > used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> > > > is a lot of interrupts for an embedded system, so they better consume as
> > > > little resources as possible. Filling a ring buffer with timestamps should be
> > > > possible, as long as no locking is involved. Locks in IRQ context must be
> > > > avoided at all costs, specially in this case.
> > > >     
> > > > > For really slow counts (i.e. 1 count/second), I can see a use for
> > > > > generating an event on each count though. For high rates, I would
> > > > > just read the count every 100ms in usespace and divide the change in
> > > > > the number of counts by the time period to get the rate.    
> > > > 
> > > > For slow counts, I agree, but for high rates, I don't (see above). There can
> > > > be glitches and false events that can (and must) be effectively filtered out.
> > > > For that user-space needs to know the time of each event during the
> > > > measurement period.    
> > > 
> > > No sure I understood the problem here. If you keep the driver as is and
> > > in userspace just read out the counter value twice and measure the time
> > > between the reads[1], you can calculate the average frequency of the
> > > event in userspace.
> > > 
> > > Isn't that good enough?  
> > 
> > No, I'm afraid it isn't, for two reasons:
> > 
> > 1. These counters are often used in environments, where glitches can and do
> > happen. So sometimes there are fake events. The only way to tell fake from
> > real pulses, is to filter them. Unfortunately you need the timestamps of each
> > event for that.
> > 
> > 2. Another reason for having time-stamps is the case when the frequency is low
> > and one still requires fast accurate measurements. In that case timestamps
> > provide a way of accurately measuring period time.
> > 
> > Best regards,
> > 
> > -- 
> > David Jander
> > Protonic Holland.  
> 
> Keeping drivers focused on just exposing the hardware data and
> functionality is likely the best path to choose, so my earlier
> suggestion of a "frequency" extension would better be left for userspace
> to handle.

I agree.

> So in order to enable userspace to derive frequency, you need reliable
> timestamps for enough consecutive events to provide an adequate size
> sample of data on which to perform filtering and other such operations.

Ack.

> If we add a COUNTER_EVENT_CHANGE_OF_STATE or similar, every count change
> will generate an event with a logged timestamp. Is the problem with this
> solution primarily that the Counter event queue is currently utilizing
> spinlocks for synchronization?

Yes. Basically, since one can expect a very high amount of IRQs, it seems
paramount to eliminate any source of latency (spinlocks, etc...) from
interrupt context as well as to keep CPU load as low as technically possible.

If a spinlock is used, and at 10kHz pulses, on a moderately fast embedded SoC,
it is IMHO quite possible to have user-space cause the spinlock to be held for
more than 100 microseconds, thus causing a pulse to be missed. Not to mention
slight jitter introduced to the timestamps that can cause user-space to falsely
filter out events (a software PLL that doesn't correctly lock).

The ideal ISR in this case would only take a timestamp from a hardware TSC (or
similarly latency-free direct source) and put it into a circular buffer
without using locks, and maybe increase an unsigned long counter value (atomic
operation if MB's are correctly used) and nothing else.
If, for example, such a solution would require user-space access CPU
load (complexity) to increase by a factor of 10 or even more (in order to
avoid locks), this is likely still preferable, since the ISR is executed maybe
1000+ times more often than user-space accessing the driver.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 12680 bytes --]

On Wed, Dec 15, 2021 at 10:08:53AM +0100, David Jander wrote:
> 
> Dear William,
> 
> On Wed, 15 Dec 2021 17:48:26 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Wed, Dec 08, 2021 at 05:10:35PM +0100, David Jander wrote:
> > > 
> > > Dear Uwe,
> > > 
> > > On Wed, 8 Dec 2021 14:59:02 +0100
> > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > >   
> > > > Hello David,
> > > > 
> > > > On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:  
> > > > > On Mon, 6 Dec 2021 13:24:18 -0600
> > > > > David Lechner <david@lechnology.com> wrote:
> > > > >     
> > > > > > On 11/24/21 7:58 PM, William Breathitt Gray wrote:    
> > > > > > > On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:      
> > > > > > >> Hi William,
> > > > > > >>
> > > > > > >> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:      
> > > > > > >>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:      
> > > > > > >>>> Add counter_push_event() to notify user space about new pulses
> > > > > > >>>>
> > > > > > >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > > >>>> ---
> > > > > > >>>>   drivers/counter/interrupt-cnt.c | 2 ++
> > > > > > >>>>   1 file changed, 2 insertions(+)
> > > > > > >>>>
> > > > > > >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > > > > >>>> index 8514a87fcbee..b237137b552b 100644
> > > > > > >>>> --- a/drivers/counter/interrupt-cnt.c
> > > > > > >>>> +++ b/drivers/counter/interrupt-cnt.c
> > > > > > >>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > > > > > >>>>   
> > > > > > >>>>   	atomic_inc(&priv->count);
> > > > > > >>>>   
> > > > > > >>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > > > > >>>> +
> > > > > > >>>>   	return IRQ_HANDLED;
> > > > > > >>>>   }
> > > > > > >>>>   
> > > > > > >>>> -- 
> > > > > > >>>> 2.30.2      
> > > > > > >>>
> > > > > > >>> Hi Oleksij,
> > > > > > >>>
> > > > > > >>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > > > > > >>> an interrupt is handled, which I suspect is not what you want to happen.
> > > > > > >>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > > > > > >>> so you'll need to check for a count value overflow before pushing the
> > > > > > >>> event.
> > > > > > >>>
> > > > > > >>> It would be good idea to implement a ceiling extension as well (you can
> > > > > > >>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > > > > > >>> particular point where the value overflows.      
> > > > > > >>
> > > > > > >> Thank you!
> > > > > > >>
> > > > > > >> What would be the best and resource effective strategy for periodically
> > > > > > >> getting frequency of interrupts/pulses? This is actual information which is
> > > > > > >> needed for my use case.
> > > > > > >>
> > > > > > >> So far, I was pushing every event to the user space, which is working
> > > > > > >> but probably not the most resource effective method of doing it.
> > > > > > >>
> > > > > > >> Regards,
> > > > > > >> Oleskij
> > > > > > >> -- 
> > > > > > >> Pengutronix e.K.                           |                             |
> > > > > > >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > > > > >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > > > > >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |      
> > > > > > > 
> > > > > > > We could introduce a new Counter change-of-state event type which would
> > > > > > > trigger whenever the count value changes, but I agree with you that this
> > > > > > > is likely not the best way for us to derive the frequency of the
> > > > > > > interrupts due to the indirection of handling and parsing the event
> > > > > > > data.
> > > > > > > 
> > > > > > > Instead, perhaps introducing a "frequency" or "period" Count extension
> > > > > > > would make more sense here. This extension could report the value delta
> > > > > > > between counts, or alternatively the time delta from which you can
> > > > > > > derive frequency. Regarding implementation, you can store the previous
> > > > > > > value in a variable, updating it whenever an interrupt occurs, and
> > > > > > > compute the particular delta every time a read is requested by the user.
> > > > > > > 
> > > > > > > David Lechner is implementing something similar for the TI eQEP driver
> > > > > > > to expose speed, so I'm CCing them here in case this is of interest to
> > > > > > > them.
> > > > > > >       
> > > > > > 
> > > > > > Based on my experience, I would recommend that counter drivers be as
> > > > > > "thin" as possible. They shouldn't try to provide any information that
> > > > > > the hardware itself doesn't provide. In other words, the kernel should
> > > > > > provide userspace the information needed to calculate the speed/rate
> > > > > > but not try to do the actual calculation in the kernel. Inevitably
> > > > > > there are nuances for specific use cases that can't all possibly be
> > > > > > handled by such an implementation.    
> > > > > 
> > > > > I completely agree with this. While interrupts aren't really meant for
> > > > > measuring frequency, and this being somewhat of a mis-use of something, it is
> > > > > still possible to do and very useful in many cases. That said, while the
> > > > > counter framework is AFAIK the best fit for this, the main use-case for this
> > > > > driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> > > > > amount of information the driver needs to provide user-space with to do
> > > > > reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> > > > > counter is not suited, because there can be glitches that need to be detected.
> > > > > If user-space gets a buffer full of consecutive time-stamps (don't need to be
> > > > > all of them, just a sample of n consecutive timestamps), as well as total
> > > > > count, all needed calculations, glitch filtering, low-pass filtering, etc...
> > > > > can be done in user-space just fine.
> > > > >     
> > > > > > I've tried using gpio interrupts to try to calculate speed/rate in
> > > > > > the kernel before and it simply doesn't work reliably. Interrupts
> > > > > > get missed and the calculation will be off.    
> > > > > 
> > > > > Exactly. Been there, done that.
> > > > > For reliable speed calculations of a mechanical system, the properties of the
> > > > > mechanical system need to be known, like physical limits of accelerations,
> > > > > maximum (or minimum) speed, etc. The minimum set of input data needed by a
> > > > > user-space application to do these calculations is total pulse count in
> > > > > addition to a buffer of timestamps of n consecutive input events (raising or
> > > > > falling edges on GPIO). So IMHO this is what the driver should provide, and
> > > > > in the most resource-efficient way possible. This particular driver will be
> > > > > used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> > > > > is a lot of interrupts for an embedded system, so they better consume as
> > > > > little resources as possible. Filling a ring buffer with timestamps should be
> > > > > possible, as long as no locking is involved. Locks in IRQ context must be
> > > > > avoided at all costs, specially in this case.
> > > > >     
> > > > > > For really slow counts (i.e. 1 count/second), I can see a use for
> > > > > > generating an event on each count though. For high rates, I would
> > > > > > just read the count every 100ms in usespace and divide the change in
> > > > > > the number of counts by the time period to get the rate.    
> > > > > 
> > > > > For slow counts, I agree, but for high rates, I don't (see above). There can
> > > > > be glitches and false events that can (and must) be effectively filtered out.
> > > > > For that user-space needs to know the time of each event during the
> > > > > measurement period.    
> > > > 
> > > > No sure I understood the problem here. If you keep the driver as is and
> > > > in userspace just read out the counter value twice and measure the time
> > > > between the reads[1], you can calculate the average frequency of the
> > > > event in userspace.
> > > > 
> > > > Isn't that good enough?  
> > > 
> > > No, I'm afraid it isn't, for two reasons:
> > > 
> > > 1. These counters are often used in environments, where glitches can and do
> > > happen. So sometimes there are fake events. The only way to tell fake from
> > > real pulses, is to filter them. Unfortunately you need the timestamps of each
> > > event for that.
> > > 
> > > 2. Another reason for having time-stamps is the case when the frequency is low
> > > and one still requires fast accurate measurements. In that case timestamps
> > > provide a way of accurately measuring period time.
> > > 
> > > Best regards,
> > > 
> > > -- 
> > > David Jander
> > > Protonic Holland.  
> > 
> > Keeping drivers focused on just exposing the hardware data and
> > functionality is likely the best path to choose, so my earlier
> > suggestion of a "frequency" extension would better be left for userspace
> > to handle.
> 
> I agree.
> 
> > So in order to enable userspace to derive frequency, you need reliable
> > timestamps for enough consecutive events to provide an adequate size
> > sample of data on which to perform filtering and other such operations.
> 
> Ack.
> 
> > If we add a COUNTER_EVENT_CHANGE_OF_STATE or similar, every count change
> > will generate an event with a logged timestamp. Is the problem with this
> > solution primarily that the Counter event queue is currently utilizing
> > spinlocks for synchronization?
> 
> Yes. Basically, since one can expect a very high amount of IRQs, it seems
> paramount to eliminate any source of latency (spinlocks, etc...) from
> interrupt context as well as to keep CPU load as low as technically possible.
> 
> If a spinlock is used, and at 10kHz pulses, on a moderately fast embedded SoC,
> it is IMHO quite possible to have user-space cause the spinlock to be held for
> more than 100 microseconds, thus causing a pulse to be missed. Not to mention
> slight jitter introduced to the timestamps that can cause user-space to falsely
> filter out events (a software PLL that doesn't correctly lock).
> 
> The ideal ISR in this case would only take a timestamp from a hardware TSC (or
> similarly latency-free direct source) and put it into a circular buffer
> without using locks, and maybe increase an unsigned long counter value (atomic
> operation if MB's are correctly used) and nothing else.
> If, for example, such a solution would require user-space access CPU
> load (complexity) to increase by a factor of 10 or even more (in order to
> avoid locks), this is likely still preferable, since the ISR is executed maybe
> 1000+ times more often than user-space accessing the driver.
> 
> Best regards,
> 
> -- 
> David Jander
> Protonic Holland.

So the counter_push_event() function interacts with two spinlocks:
events_list_lock and events_in_lock. The events_list_lock spinlock is
necessary because userspace can modify the events_list list via the
counter_enable_events() and counter_disable_events() functions. The
events_in_lock spinlock is necessary because userspace can modify the
events kfifo via the counter_events_queue_size_write() function.

A lockless solution for this might be possible if the driver maintains
its own circular buffer as you suggest. The driver's IRQ handler can
write to this circular buffer without calling the counter_push_event()
function, and then flush the buffer to the Counter character device via
a userspace write to a "flush_events" sysfs attribute or similar; this
eliminates the need for the events_in_lock spinlock. The state of the
events_list list can be captured in the driver's events_configure()
callback and stored locally in the driver for reference, thus
eliminating the need for the events_list_lock; interrupts can be
disabled before the driver's local copy of events_list is modified.

With only one reader and one writer operating on the driver's buffer,
you can use the normal kfifo_in and kfifo_out calls for lockless
operations. Perhaps that is a way forward for this problem.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Lechner]

On 12/24/21 10:07 PM, William Breathitt Gray wrote:
> On Wed, Dec 15, 2021 at 10:08:53AM +0100, David Jander wrote:
>>
>> Dear William,
>>
>> On Wed, 15 Dec 2021 17:48:26 +0900
>> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>>
>>> On Wed, Dec 08, 2021 at 05:10:35PM +0100, David Jander wrote:
>>>>
>>>> Dear Uwe,
>>>>
>>>> On Wed, 8 Dec 2021 14:59:02 +0100
>>>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>>>>    
>>>>> Hello David,
>>>>>
>>>>> On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:
>>>>>> On Mon, 6 Dec 2021 13:24:18 -0600
>>>>>> David Lechner <david@lechnology.com> wrote:
>>>>>>      
>>>>>>> On 11/24/21 7:58 PM, William Breathitt Gray wrote:
>>>>>>>> On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:
>>>>>>>>> Hi William,
>>>>>>>>>
>>>>>>>>> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:
>>>>>>>>>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
>>>>>>>>>>> Add counter_push_event() to notify user space about new pulses
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>>>>>>> ---
>>>>>>>>>>>    drivers/counter/interrupt-cnt.c | 2 ++
>>>>>>>>>>>    1 file changed, 2 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
>>>>>>>>>>> index 8514a87fcbee..b237137b552b 100644
>>>>>>>>>>> --- a/drivers/counter/interrupt-cnt.c
>>>>>>>>>>> +++ b/drivers/counter/interrupt-cnt.c
>>>>>>>>>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
>>>>>>>>>>>    
>>>>>>>>>>>    	atomic_inc(&priv->count);
>>>>>>>>>>>    
>>>>>>>>>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
>>>>>>>>>>> +
>>>>>>>>>>>    	return IRQ_HANDLED;
>>>>>>>>>>>    }
>>>>>>>>>>>    
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.30.2
>>>>>>>>>>
>>>>>>>>>> Hi Oleksij,
>>>>>>>>>>
>>>>>>>>>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
>>>>>>>>>> an interrupt is handled, which I suspect is not what you want to happen.
>>>>>>>>>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
>>>>>>>>>> so you'll need to check for a count value overflow before pushing the
>>>>>>>>>> event.
>>>>>>>>>>
>>>>>>>>>> It would be good idea to implement a ceiling extension as well (you can
>>>>>>>>>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
>>>>>>>>>> particular point where the value overflows.
>>>>>>>>>
>>>>>>>>> Thank you!
>>>>>>>>>
>>>>>>>>> What would be the best and resource effective strategy for periodically
>>>>>>>>> getting frequency of interrupts/pulses? This is actual information which is
>>>>>>>>> needed for my use case.
>>>>>>>>>
>>>>>>>>> So far, I was pushing every event to the user space, which is working
>>>>>>>>> but probably not the most resource effective method of doing it.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Oleskij
>>>>>>>>> -- 
>>>>>>>>> Pengutronix e.K.                           |                             |
>>>>>>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>>>>>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>>>>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>>>>>>>
>>>>>>>> We could introduce a new Counter change-of-state event type which would
>>>>>>>> trigger whenever the count value changes, but I agree with you that this
>>>>>>>> is likely not the best way for us to derive the frequency of the
>>>>>>>> interrupts due to the indirection of handling and parsing the event
>>>>>>>> data.
>>>>>>>>
>>>>>>>> Instead, perhaps introducing a "frequency" or "period" Count extension
>>>>>>>> would make more sense here. This extension could report the value delta
>>>>>>>> between counts, or alternatively the time delta from which you can
>>>>>>>> derive frequency. Regarding implementation, you can store the previous
>>>>>>>> value in a variable, updating it whenever an interrupt occurs, and
>>>>>>>> compute the particular delta every time a read is requested by the user.
>>>>>>>>
>>>>>>>> David Lechner is implementing something similar for the TI eQEP driver
>>>>>>>> to expose speed, so I'm CCing them here in case this is of interest to
>>>>>>>> them.
>>>>>>>>        
>>>>>>>
>>>>>>> Based on my experience, I would recommend that counter drivers be as
>>>>>>> "thin" as possible. They shouldn't try to provide any information that
>>>>>>> the hardware itself doesn't provide. In other words, the kernel should
>>>>>>> provide userspace the information needed to calculate the speed/rate
>>>>>>> but not try to do the actual calculation in the kernel. Inevitably
>>>>>>> there are nuances for specific use cases that can't all possibly be
>>>>>>> handled by such an implementation.
>>>>>>
>>>>>> I completely agree with this. While interrupts aren't really meant for
>>>>>> measuring frequency, and this being somewhat of a mis-use of something, it is
>>>>>> still possible to do and very useful in many cases. That said, while the
>>>>>> counter framework is AFAIK the best fit for this, the main use-case for this
>>>>>> driver is measuring wheel speed (and similar "speeds"). For this, the minimum
>>>>>> amount of information the driver needs to provide user-space with to do
>>>>>> reliable calculations, is high-resolution time-stamps of GPIO events. A simple
>>>>>> counter is not suited, because there can be glitches that need to be detected.
>>>>>> If user-space gets a buffer full of consecutive time-stamps (don't need to be
>>>>>> all of them, just a sample of n consecutive timestamps), as well as total
>>>>>> count, all needed calculations, glitch filtering, low-pass filtering, etc...
>>>>>> can be done in user-space just fine.
>>>>>>      
>>>>>>> I've tried using gpio interrupts to try to calculate speed/rate in
>>>>>>> the kernel before and it simply doesn't work reliably. Interrupts
>>>>>>> get missed and the calculation will be off.
>>>>>>
>>>>>> Exactly. Been there, done that.
>>>>>> For reliable speed calculations of a mechanical system, the properties of the
>>>>>> mechanical system need to be known, like physical limits of accelerations,
>>>>>> maximum (or minimum) speed, etc. The minimum set of input data needed by a
>>>>>> user-space application to do these calculations is total pulse count in
>>>>>> addition to a buffer of timestamps of n consecutive input events (raising or
>>>>>> falling edges on GPIO). So IMHO this is what the driver should provide, and
>>>>>> in the most resource-efficient way possible. This particular driver will be
>>>>>> used 3 times on the same SoC, with each up to 10-15k pulses per second. That
>>>>>> is a lot of interrupts for an embedded system, so they better consume as
>>>>>> little resources as possible. Filling a ring buffer with timestamps should be
>>>>>> possible, as long as no locking is involved. Locks in IRQ context must be
>>>>>> avoided at all costs, specially in this case.
>>>>>>      
>>>>>>> For really slow counts (i.e. 1 count/second), I can see a use for
>>>>>>> generating an event on each count though. For high rates, I would
>>>>>>> just read the count every 100ms in usespace and divide the change in
>>>>>>> the number of counts by the time period to get the rate.
>>>>>>
>>>>>> For slow counts, I agree, but for high rates, I don't (see above). There can
>>>>>> be glitches and false events that can (and must) be effectively filtered out.
>>>>>> For that user-space needs to know the time of each event during the
>>>>>> measurement period.
>>>>>
>>>>> No sure I understood the problem here. If you keep the driver as is and
>>>>> in userspace just read out the counter value twice and measure the time
>>>>> between the reads[1], you can calculate the average frequency of the
>>>>> event in userspace.
>>>>>
>>>>> Isn't that good enough?
>>>>
>>>> No, I'm afraid it isn't, for two reasons:
>>>>
>>>> 1. These counters are often used in environments, where glitches can and do
>>>> happen. So sometimes there are fake events. The only way to tell fake from
>>>> real pulses, is to filter them. Unfortunately you need the timestamps of each
>>>> event for that.
>>>>
>>>> 2. Another reason for having time-stamps is the case when the frequency is low
>>>> and one still requires fast accurate measurements. In that case timestamps
>>>> provide a way of accurately measuring period time.
>>>>
>>>> Best regards,
>>>>
>>>> -- 
>>>> David Jander
>>>> Protonic Holland.
>>>
>>> Keeping drivers focused on just exposing the hardware data and
>>> functionality is likely the best path to choose, so my earlier
>>> suggestion of a "frequency" extension would better be left for userspace
>>> to handle.
>>
>> I agree.
>>
>>> So in order to enable userspace to derive frequency, you need reliable
>>> timestamps for enough consecutive events to provide an adequate size
>>> sample of data on which to perform filtering and other such operations.
>>
>> Ack.
>>
>>> If we add a COUNTER_EVENT_CHANGE_OF_STATE or similar, every count change
>>> will generate an event with a logged timestamp. Is the problem with this
>>> solution primarily that the Counter event queue is currently utilizing
>>> spinlocks for synchronization?
>>
>> Yes. Basically, since one can expect a very high amount of IRQs, it seems
>> paramount to eliminate any source of latency (spinlocks, etc...) from
>> interrupt context as well as to keep CPU load as low as technically possible.
>>
>> If a spinlock is used, and at 10kHz pulses, on a moderately fast embedded SoC,
>> it is IMHO quite possible to have user-space cause the spinlock to be held for
>> more than 100 microseconds, thus causing a pulse to be missed. Not to mention
>> slight jitter introduced to the timestamps that can cause user-space to falsely
>> filter out events (a software PLL that doesn't correctly lock).
>>
>> The ideal ISR in this case would only take a timestamp from a hardware TSC (or
>> similarly latency-free direct source) and put it into a circular buffer
>> without using locks, and maybe increase an unsigned long counter value (atomic
>> operation if MB's are correctly used) and nothing else.
>> If, for example, such a solution would require user-space access CPU
>> load (complexity) to increase by a factor of 10 or even more (in order to
>> avoid locks), this is likely still preferable, since the ISR is executed maybe
>> 1000+ times more often than user-space accessing the driver.
>>
>> Best regards,
>>
>> -- 
>> David Jander
>> Protonic Holland.
> 
> So the counter_push_event() function interacts with two spinlocks:
> events_list_lock and events_in_lock. The events_list_lock spinlock is
> necessary because userspace can modify the events_list list via the
> counter_enable_events() and counter_disable_events() functions. The
> events_in_lock spinlock is necessary because userspace can modify the
> events kfifo via the counter_events_queue_size_write() function.
> 
> A lockless solution for this might be possible if the driver maintains
> its own circular buffer as you suggest. The driver's IRQ handler can
> write to this circular buffer without calling the counter_push_event()
> function, and then flush the buffer to the Counter character device via
> a userspace write to a "flush_events" sysfs attribute or similar; this
> eliminates the need for the events_in_lock spinlock. The state of the
> events_list list can be captured in the driver's events_configure()
> callback and stored locally in the driver for reference, thus
> eliminating the need for the events_list_lock; interrupts can be
> disabled before the driver's local copy of events_list is modified.
> 
> With only one reader and one writer operating on the driver's buffer,
> you can use the normal kfifo_in and kfifo_out calls for lockless
> operations. Perhaps that is a way forward for this problem.
> 

Would it be possible to make it so that userspace can't modify the
events_list when IRQs are enabled? Then we wouldn't have to add asecond event buffer.

IIRC, the only operations that modify events_list are when another
list replaces events_list when events are enabled and when
events_list is cleared when events are disabled. So as long as the
ordering is right with respect to enabling and disabling IRQs, it
seems like the spin lock should not be needed.

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 13516 bytes --]

On Mon, Dec 27, 2021 at 09:16:31AM -0600, David Lechner wrote:
> On 12/24/21 10:07 PM, William Breathitt Gray wrote:
> > On Wed, Dec 15, 2021 at 10:08:53AM +0100, David Jander wrote:
> >>
> >> Dear William,
> >>
> >> On Wed, 15 Dec 2021 17:48:26 +0900
> >> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >>
> >>> On Wed, Dec 08, 2021 at 05:10:35PM +0100, David Jander wrote:
> >>>>
> >>>> Dear Uwe,
> >>>>
> >>>> On Wed, 8 Dec 2021 14:59:02 +0100
> >>>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >>>>    
> >>>>> Hello David,
> >>>>>
> >>>>> On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:
> >>>>>> On Mon, 6 Dec 2021 13:24:18 -0600
> >>>>>> David Lechner <david@lechnology.com> wrote:
> >>>>>>      
> >>>>>>> On 11/24/21 7:58 PM, William Breathitt Gray wrote:
> >>>>>>>> On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:
> >>>>>>>>> Hi William,
> >>>>>>>>>
> >>>>>>>>> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:
> >>>>>>>>>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
> >>>>>>>>>>> Add counter_push_event() to notify user space about new pulses
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>>>>>>>>> ---
> >>>>>>>>>>>    drivers/counter/interrupt-cnt.c | 2 ++
> >>>>>>>>>>>    1 file changed, 2 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> >>>>>>>>>>> index 8514a87fcbee..b237137b552b 100644
> >>>>>>>>>>> --- a/drivers/counter/interrupt-cnt.c
> >>>>>>>>>>> +++ b/drivers/counter/interrupt-cnt.c
> >>>>>>>>>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> >>>>>>>>>>>    
> >>>>>>>>>>>    	atomic_inc(&priv->count);
> >>>>>>>>>>>    
> >>>>>>>>>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> >>>>>>>>>>> +
> >>>>>>>>>>>    	return IRQ_HANDLED;
> >>>>>>>>>>>    }
> >>>>>>>>>>>    
> >>>>>>>>>>> -- 
> >>>>>>>>>>> 2.30.2
> >>>>>>>>>>
> >>>>>>>>>> Hi Oleksij,
> >>>>>>>>>>
> >>>>>>>>>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> >>>>>>>>>> an interrupt is handled, which I suspect is not what you want to happen.
> >>>>>>>>>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> >>>>>>>>>> so you'll need to check for a count value overflow before pushing the
> >>>>>>>>>> event.
> >>>>>>>>>>
> >>>>>>>>>> It would be good idea to implement a ceiling extension as well (you can
> >>>>>>>>>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> >>>>>>>>>> particular point where the value overflows.
> >>>>>>>>>
> >>>>>>>>> Thank you!
> >>>>>>>>>
> >>>>>>>>> What would be the best and resource effective strategy for periodically
> >>>>>>>>> getting frequency of interrupts/pulses? This is actual information which is
> >>>>>>>>> needed for my use case.
> >>>>>>>>>
> >>>>>>>>> So far, I was pushing every event to the user space, which is working
> >>>>>>>>> but probably not the most resource effective method of doing it.
> >>>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Oleskij
> >>>>>>>>> -- 
> >>>>>>>>> Pengutronix e.K.                           |                             |
> >>>>>>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> >>>>>>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >>>>>>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >>>>>>>>
> >>>>>>>> We could introduce a new Counter change-of-state event type which would
> >>>>>>>> trigger whenever the count value changes, but I agree with you that this
> >>>>>>>> is likely not the best way for us to derive the frequency of the
> >>>>>>>> interrupts due to the indirection of handling and parsing the event
> >>>>>>>> data.
> >>>>>>>>
> >>>>>>>> Instead, perhaps introducing a "frequency" or "period" Count extension
> >>>>>>>> would make more sense here. This extension could report the value delta
> >>>>>>>> between counts, or alternatively the time delta from which you can
> >>>>>>>> derive frequency. Regarding implementation, you can store the previous
> >>>>>>>> value in a variable, updating it whenever an interrupt occurs, and
> >>>>>>>> compute the particular delta every time a read is requested by the user.
> >>>>>>>>
> >>>>>>>> David Lechner is implementing something similar for the TI eQEP driver
> >>>>>>>> to expose speed, so I'm CCing them here in case this is of interest to
> >>>>>>>> them.
> >>>>>>>>        
> >>>>>>>
> >>>>>>> Based on my experience, I would recommend that counter drivers be as
> >>>>>>> "thin" as possible. They shouldn't try to provide any information that
> >>>>>>> the hardware itself doesn't provide. In other words, the kernel should
> >>>>>>> provide userspace the information needed to calculate the speed/rate
> >>>>>>> but not try to do the actual calculation in the kernel. Inevitably
> >>>>>>> there are nuances for specific use cases that can't all possibly be
> >>>>>>> handled by such an implementation.
> >>>>>>
> >>>>>> I completely agree with this. While interrupts aren't really meant for
> >>>>>> measuring frequency, and this being somewhat of a mis-use of something, it is
> >>>>>> still possible to do and very useful in many cases. That said, while the
> >>>>>> counter framework is AFAIK the best fit for this, the main use-case for this
> >>>>>> driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> >>>>>> amount of information the driver needs to provide user-space with to do
> >>>>>> reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> >>>>>> counter is not suited, because there can be glitches that need to be detected.
> >>>>>> If user-space gets a buffer full of consecutive time-stamps (don't need to be
> >>>>>> all of them, just a sample of n consecutive timestamps), as well as total
> >>>>>> count, all needed calculations, glitch filtering, low-pass filtering, etc...
> >>>>>> can be done in user-space just fine.
> >>>>>>      
> >>>>>>> I've tried using gpio interrupts to try to calculate speed/rate in
> >>>>>>> the kernel before and it simply doesn't work reliably. Interrupts
> >>>>>>> get missed and the calculation will be off.
> >>>>>>
> >>>>>> Exactly. Been there, done that.
> >>>>>> For reliable speed calculations of a mechanical system, the properties of the
> >>>>>> mechanical system need to be known, like physical limits of accelerations,
> >>>>>> maximum (or minimum) speed, etc. The minimum set of input data needed by a
> >>>>>> user-space application to do these calculations is total pulse count in
> >>>>>> addition to a buffer of timestamps of n consecutive input events (raising or
> >>>>>> falling edges on GPIO). So IMHO this is what the driver should provide, and
> >>>>>> in the most resource-efficient way possible. This particular driver will be
> >>>>>> used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> >>>>>> is a lot of interrupts for an embedded system, so they better consume as
> >>>>>> little resources as possible. Filling a ring buffer with timestamps should be
> >>>>>> possible, as long as no locking is involved. Locks in IRQ context must be
> >>>>>> avoided at all costs, specially in this case.
> >>>>>>      
> >>>>>>> For really slow counts (i.e. 1 count/second), I can see a use for
> >>>>>>> generating an event on each count though. For high rates, I would
> >>>>>>> just read the count every 100ms in usespace and divide the change in
> >>>>>>> the number of counts by the time period to get the rate.
> >>>>>>
> >>>>>> For slow counts, I agree, but for high rates, I don't (see above). There can
> >>>>>> be glitches and false events that can (and must) be effectively filtered out.
> >>>>>> For that user-space needs to know the time of each event during the
> >>>>>> measurement period.
> >>>>>
> >>>>> No sure I understood the problem here. If you keep the driver as is and
> >>>>> in userspace just read out the counter value twice and measure the time
> >>>>> between the reads[1], you can calculate the average frequency of the
> >>>>> event in userspace.
> >>>>>
> >>>>> Isn't that good enough?
> >>>>
> >>>> No, I'm afraid it isn't, for two reasons:
> >>>>
> >>>> 1. These counters are often used in environments, where glitches can and do
> >>>> happen. So sometimes there are fake events. The only way to tell fake from
> >>>> real pulses, is to filter them. Unfortunately you need the timestamps of each
> >>>> event for that.
> >>>>
> >>>> 2. Another reason for having time-stamps is the case when the frequency is low
> >>>> and one still requires fast accurate measurements. In that case timestamps
> >>>> provide a way of accurately measuring period time.
> >>>>
> >>>> Best regards,
> >>>>
> >>>> -- 
> >>>> David Jander
> >>>> Protonic Holland.
> >>>
> >>> Keeping drivers focused on just exposing the hardware data and
> >>> functionality is likely the best path to choose, so my earlier
> >>> suggestion of a "frequency" extension would better be left for userspace
> >>> to handle.
> >>
> >> I agree.
> >>
> >>> So in order to enable userspace to derive frequency, you need reliable
> >>> timestamps for enough consecutive events to provide an adequate size
> >>> sample of data on which to perform filtering and other such operations.
> >>
> >> Ack.
> >>
> >>> If we add a COUNTER_EVENT_CHANGE_OF_STATE or similar, every count change
> >>> will generate an event with a logged timestamp. Is the problem with this
> >>> solution primarily that the Counter event queue is currently utilizing
> >>> spinlocks for synchronization?
> >>
> >> Yes. Basically, since one can expect a very high amount of IRQs, it seems
> >> paramount to eliminate any source of latency (spinlocks, etc...) from
> >> interrupt context as well as to keep CPU load as low as technically possible.
> >>
> >> If a spinlock is used, and at 10kHz pulses, on a moderately fast embedded SoC,
> >> it is IMHO quite possible to have user-space cause the spinlock to be held for
> >> more than 100 microseconds, thus causing a pulse to be missed. Not to mention
> >> slight jitter introduced to the timestamps that can cause user-space to falsely
> >> filter out events (a software PLL that doesn't correctly lock).
> >>
> >> The ideal ISR in this case would only take a timestamp from a hardware TSC (or
> >> similarly latency-free direct source) and put it into a circular buffer
> >> without using locks, and maybe increase an unsigned long counter value (atomic
> >> operation if MB's are correctly used) and nothing else.
> >> If, for example, such a solution would require user-space access CPU
> >> load (complexity) to increase by a factor of 10 or even more (in order to
> >> avoid locks), this is likely still preferable, since the ISR is executed maybe
> >> 1000+ times more often than user-space accessing the driver.
> >>
> >> Best regards,
> >>
> >> -- 
> >> David Jander
> >> Protonic Holland.
> > 
> > So the counter_push_event() function interacts with two spinlocks:
> > events_list_lock and events_in_lock. The events_list_lock spinlock is
> > necessary because userspace can modify the events_list list via the
> > counter_enable_events() and counter_disable_events() functions. The
> > events_in_lock spinlock is necessary because userspace can modify the
> > events kfifo via the counter_events_queue_size_write() function.
> > 
> > A lockless solution for this might be possible if the driver maintains
> > its own circular buffer as you suggest. The driver's IRQ handler can
> > write to this circular buffer without calling the counter_push_event()
> > function, and then flush the buffer to the Counter character device via
> > a userspace write to a "flush_events" sysfs attribute or similar; this
> > eliminates the need for the events_in_lock spinlock. The state of the
> > events_list list can be captured in the driver's events_configure()
> > callback and stored locally in the driver for reference, thus
> > eliminating the need for the events_list_lock; interrupts can be
> > disabled before the driver's local copy of events_list is modified.
> > 
> > With only one reader and one writer operating on the driver's buffer,
> > you can use the normal kfifo_in and kfifo_out calls for lockless
> > operations. Perhaps that is a way forward for this problem.
> > 
> 
> Would it be possible to make it so that userspace can't modify the
> events_list when IRQs are enabled? Then we wouldn't have to add asecond event buffer.
> 
> IIRC, the only operations that modify events_list are when another
> list replaces events_list when events are enabled and when
> events_list is cleared when events are disabled. So as long as the
> ordering is right with respect to enabling and disabling IRQs, it
> seems like the spin lock should not be needed.

I think that could work. If IRQs are always disabled before events_list
is modified, then there is never a risk of interacting with an invalid
events_list and thus counter_push_events() won't need spinlocks. When
IRQs are disabled we miss any possible events, but we are missing those
already anyway when the events_list is modified.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Jonathan Cameron]

On Wed, 29 Dec 2021 18:26:06 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Mon, Dec 27, 2021 at 09:16:31AM -0600, David Lechner wrote:
> > On 12/24/21 10:07 PM, William Breathitt Gray wrote:  
> > > On Wed, Dec 15, 2021 at 10:08:53AM +0100, David Jander wrote:  
> > >>
> > >> Dear William,
> > >>
> > >> On Wed, 15 Dec 2021 17:48:26 +0900
> > >> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > >>  
> > >>> On Wed, Dec 08, 2021 at 05:10:35PM +0100, David Jander wrote:  
> > >>>>
> > >>>> Dear Uwe,
> > >>>>
> > >>>> On Wed, 8 Dec 2021 14:59:02 +0100
> > >>>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > >>>>      
> > >>>>> Hello David,
> > >>>>>
> > >>>>> On Tue, Dec 07, 2021 at 08:16:02AM +0100, David Jander wrote:  
> > >>>>>> On Mon, 6 Dec 2021 13:24:18 -0600
> > >>>>>> David Lechner <david@lechnology.com> wrote:
> > >>>>>>        
> > >>>>>>> On 11/24/21 7:58 PM, William Breathitt Gray wrote:  
> > >>>>>>>> On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:  
> > >>>>>>>>> Hi William,
> > >>>>>>>>>
> > >>>>>>>>> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:  
> > >>>>>>>>>> On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:  
> > >>>>>>>>>>> Add counter_push_event() to notify user space about new pulses
> > >>>>>>>>>>>
> > >>>>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>    drivers/counter/interrupt-cnt.c | 2 ++
> > >>>>>>>>>>>    1 file changed, 2 insertions(+)
> > >>>>>>>>>>>
> > >>>>>>>>>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > >>>>>>>>>>> index 8514a87fcbee..b237137b552b 100644
> > >>>>>>>>>>> --- a/drivers/counter/interrupt-cnt.c
> > >>>>>>>>>>> +++ b/drivers/counter/interrupt-cnt.c
> > >>>>>>>>>>> @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > >>>>>>>>>>>    
> > >>>>>>>>>>>    	atomic_inc(&priv->count);
> > >>>>>>>>>>>    
> > >>>>>>>>>>> +	counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > >>>>>>>>>>> +
> > >>>>>>>>>>>    	return IRQ_HANDLED;
> > >>>>>>>>>>>    }
> > >>>>>>>>>>>    
> > >>>>>>>>>>> -- 
> > >>>>>>>>>>> 2.30.2  
> > >>>>>>>>>>
> > >>>>>>>>>> Hi Oleksij,
> > >>>>>>>>>>
> > >>>>>>>>>> It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > >>>>>>>>>> an interrupt is handled, which I suspect is not what you want to happen.
> > >>>>>>>>>> The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > >>>>>>>>>> so you'll need to check for a count value overflow before pushing the
> > >>>>>>>>>> event.
> > >>>>>>>>>>
> > >>>>>>>>>> It would be good idea to implement a ceiling extension as well (you can
> > >>>>>>>>>> use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > >>>>>>>>>> particular point where the value overflows.  
> > >>>>>>>>>
> > >>>>>>>>> Thank you!
> > >>>>>>>>>
> > >>>>>>>>> What would be the best and resource effective strategy for periodically
> > >>>>>>>>> getting frequency of interrupts/pulses? This is actual information which is
> > >>>>>>>>> needed for my use case.
> > >>>>>>>>>
> > >>>>>>>>> So far, I was pushing every event to the user space, which is working
> > >>>>>>>>> but probably not the most resource effective method of doing it.
> > >>>>>>>>>
> > >>>>>>>>> Regards,
> > >>>>>>>>> Oleskij
> > >>>>>>>>> -- 
> > >>>>>>>>> Pengutronix e.K.                           |                             |
> > >>>>>>>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > >>>>>>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > >>>>>>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |  
> > >>>>>>>>
> > >>>>>>>> We could introduce a new Counter change-of-state event type which would
> > >>>>>>>> trigger whenever the count value changes, but I agree with you that this
> > >>>>>>>> is likely not the best way for us to derive the frequency of the
> > >>>>>>>> interrupts due to the indirection of handling and parsing the event
> > >>>>>>>> data.
> > >>>>>>>>
> > >>>>>>>> Instead, perhaps introducing a "frequency" or "period" Count extension
> > >>>>>>>> would make more sense here. This extension could report the value delta
> > >>>>>>>> between counts, or alternatively the time delta from which you can
> > >>>>>>>> derive frequency. Regarding implementation, you can store the previous
> > >>>>>>>> value in a variable, updating it whenever an interrupt occurs, and
> > >>>>>>>> compute the particular delta every time a read is requested by the user.
> > >>>>>>>>
> > >>>>>>>> David Lechner is implementing something similar for the TI eQEP driver
> > >>>>>>>> to expose speed, so I'm CCing them here in case this is of interest to
> > >>>>>>>> them.
> > >>>>>>>>          
> > >>>>>>>
> > >>>>>>> Based on my experience, I would recommend that counter drivers be as
> > >>>>>>> "thin" as possible. They shouldn't try to provide any information that
> > >>>>>>> the hardware itself doesn't provide. In other words, the kernel should
> > >>>>>>> provide userspace the information needed to calculate the speed/rate
> > >>>>>>> but not try to do the actual calculation in the kernel. Inevitably
> > >>>>>>> there are nuances for specific use cases that can't all possibly be
> > >>>>>>> handled by such an implementation.  
> > >>>>>>
> > >>>>>> I completely agree with this. While interrupts aren't really meant for
> > >>>>>> measuring frequency, and this being somewhat of a mis-use of something, it is
> > >>>>>> still possible to do and very useful in many cases. That said, while the
> > >>>>>> counter framework is AFAIK the best fit for this, the main use-case for this
> > >>>>>> driver is measuring wheel speed (and similar "speeds"). For this, the minimum
> > >>>>>> amount of information the driver needs to provide user-space with to do
> > >>>>>> reliable calculations, is high-resolution time-stamps of GPIO events. A simple
> > >>>>>> counter is not suited, because there can be glitches that need to be detected.
> > >>>>>> If user-space gets a buffer full of consecutive time-stamps (don't need to be
> > >>>>>> all of them, just a sample of n consecutive timestamps), as well as total
> > >>>>>> count, all needed calculations, glitch filtering, low-pass filtering, etc...
> > >>>>>> can be done in user-space just fine.
> > >>>>>>        
> > >>>>>>> I've tried using gpio interrupts to try to calculate speed/rate in
> > >>>>>>> the kernel before and it simply doesn't work reliably. Interrupts
> > >>>>>>> get missed and the calculation will be off.  
> > >>>>>>
> > >>>>>> Exactly. Been there, done that.
> > >>>>>> For reliable speed calculations of a mechanical system, the properties of the
> > >>>>>> mechanical system need to be known, like physical limits of accelerations,
> > >>>>>> maximum (or minimum) speed, etc. The minimum set of input data needed by a
> > >>>>>> user-space application to do these calculations is total pulse count in
> > >>>>>> addition to a buffer of timestamps of n consecutive input events (raising or
> > >>>>>> falling edges on GPIO). So IMHO this is what the driver should provide, and
> > >>>>>> in the most resource-efficient way possible. This particular driver will be
> > >>>>>> used 3 times on the same SoC, with each up to 10-15k pulses per second. That
> > >>>>>> is a lot of interrupts for an embedded system, so they better consume as
> > >>>>>> little resources as possible. Filling a ring buffer with timestamps should be
> > >>>>>> possible, as long as no locking is involved. Locks in IRQ context must be
> > >>>>>> avoided at all costs, specially in this case.
> > >>>>>>        
> > >>>>>>> For really slow counts (i.e. 1 count/second), I can see a use for
> > >>>>>>> generating an event on each count though. For high rates, I would
> > >>>>>>> just read the count every 100ms in usespace and divide the change in
> > >>>>>>> the number of counts by the time period to get the rate.  
> > >>>>>>
> > >>>>>> For slow counts, I agree, but for high rates, I don't (see above). There can
> > >>>>>> be glitches and false events that can (and must) be effectively filtered out.
> > >>>>>> For that user-space needs to know the time of each event during the
> > >>>>>> measurement period.  
> > >>>>>
> > >>>>> No sure I understood the problem here. If you keep the driver as is and
> > >>>>> in userspace just read out the counter value twice and measure the time
> > >>>>> between the reads[1], you can calculate the average frequency of the
> > >>>>> event in userspace.
> > >>>>>
> > >>>>> Isn't that good enough?  
> > >>>>
> > >>>> No, I'm afraid it isn't, for two reasons:
> > >>>>
> > >>>> 1. These counters are often used in environments, where glitches can and do
> > >>>> happen. So sometimes there are fake events. The only way to tell fake from
> > >>>> real pulses, is to filter them. Unfortunately you need the timestamps of each
> > >>>> event for that.
> > >>>>
> > >>>> 2. Another reason for having time-stamps is the case when the frequency is low
> > >>>> and one still requires fast accurate measurements. In that case timestamps
> > >>>> provide a way of accurately measuring period time.
> > >>>>
> > >>>> Best regards,
> > >>>>
> > >>>> -- 
> > >>>> David Jander
> > >>>> Protonic Holland.  
> > >>>
> > >>> Keeping drivers focused on just exposing the hardware data and
> > >>> functionality is likely the best path to choose, so my earlier
> > >>> suggestion of a "frequency" extension would better be left for userspace
> > >>> to handle.  
> > >>
> > >> I agree.
> > >>  
> > >>> So in order to enable userspace to derive frequency, you need reliable
> > >>> timestamps for enough consecutive events to provide an adequate size
> > >>> sample of data on which to perform filtering and other such operations.  
> > >>
> > >> Ack.
> > >>  
> > >>> If we add a COUNTER_EVENT_CHANGE_OF_STATE or similar, every count change
> > >>> will generate an event with a logged timestamp. Is the problem with this
> > >>> solution primarily that the Counter event queue is currently utilizing
> > >>> spinlocks for synchronization?  
> > >>
> > >> Yes. Basically, since one can expect a very high amount of IRQs, it seems
> > >> paramount to eliminate any source of latency (spinlocks, etc...) from
> > >> interrupt context as well as to keep CPU load as low as technically possible.
> > >>
> > >> If a spinlock is used, and at 10kHz pulses, on a moderately fast embedded SoC,
> > >> it is IMHO quite possible to have user-space cause the spinlock to be held for
> > >> more than 100 microseconds, thus causing a pulse to be missed. Not to mention
> > >> slight jitter introduced to the timestamps that can cause user-space to falsely
> > >> filter out events (a software PLL that doesn't correctly lock).
> > >>
> > >> The ideal ISR in this case would only take a timestamp from a hardware TSC (or
> > >> similarly latency-free direct source) and put it into a circular buffer
> > >> without using locks, and maybe increase an unsigned long counter value (atomic
> > >> operation if MB's are correctly used) and nothing else.
> > >> If, for example, such a solution would require user-space access CPU
> > >> load (complexity) to increase by a factor of 10 or even more (in order to
> > >> avoid locks), this is likely still preferable, since the ISR is executed maybe
> > >> 1000+ times more often than user-space accessing the driver.
> > >>
> > >> Best regards,
> > >>
> > >> -- 
> > >> David Jander
> > >> Protonic Holland.  
> > > 
> > > So the counter_push_event() function interacts with two spinlocks:
> > > events_list_lock and events_in_lock. The events_list_lock spinlock is
> > > necessary because userspace can modify the events_list list via the
> > > counter_enable_events() and counter_disable_events() functions. The
> > > events_in_lock spinlock is necessary because userspace can modify the
> > > events kfifo via the counter_events_queue_size_write() function.
> > > 
> > > A lockless solution for this might be possible if the driver maintains
> > > its own circular buffer as you suggest. The driver's IRQ handler can
> > > write to this circular buffer without calling the counter_push_event()
> > > function, and then flush the buffer to the Counter character device via
> > > a userspace write to a "flush_events" sysfs attribute or similar; this
> > > eliminates the need for the events_in_lock spinlock. The state of the
> > > events_list list can be captured in the driver's events_configure()
> > > callback and stored locally in the driver for reference, thus
> > > eliminating the need for the events_list_lock; interrupts can be
> > > disabled before the driver's local copy of events_list is modified.
> > > 
> > > With only one reader and one writer operating on the driver's buffer,
> > > you can use the normal kfifo_in and kfifo_out calls for lockless
> > > operations. Perhaps that is a way forward for this problem.
> > >   
> > 
> > Would it be possible to make it so that userspace can't modify the
> > events_list when IRQs are enabled? Then we wouldn't have to add asecond event buffer.
> > 
> > IIRC, the only operations that modify events_list are when another
> > list replaces events_list when events are enabled and when
> > events_list is cleared when events are disabled. So as long as the
> > ordering is right with respect to enabling and disabling IRQs, it
> > seems like the spin lock should not be needed.  
> 
> I think that could work. If IRQs are always disabled before events_list
> is modified, then there is never a risk of interacting with an invalid
> events_list and thus counter_push_events() won't need spinlocks. When
> IRQs are disabled we miss any possible events, but we are missing those
> already anyway when the events_list is modified.

I wonder if an RCU approach would be cleaner and perhaps give the performance
necessary?
https://www.kernel.org/doc/html/latest/RCU/listRCU.html

> 
> William Breathitt Gray

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Oleksij Rempel]

Hi William,

On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
... 
> So the counter_push_event() function interacts with two spinlocks:
> events_list_lock and events_in_lock. The events_list_lock spinlock is
> necessary because userspace can modify the events_list list via the
> counter_enable_events() and counter_disable_events() functions. The
> events_in_lock spinlock is necessary because userspace can modify the
> events kfifo via the counter_events_queue_size_write() function.
> 
> A lockless solution for this might be possible if the driver maintains
> its own circular buffer as you suggest. The driver's IRQ handler can
> write to this circular buffer without calling the counter_push_event()
> function, and then flush the buffer to the Counter character device via
> a userspace write to a "flush_events" sysfs attribute or similar; this
> eliminates the need for the events_in_lock spinlock. The state of the
> events_list list can be captured in the driver's events_configure()
> callback and stored locally in the driver for reference, thus
> eliminating the need for the events_list_lock; interrupts can be
> disabled before the driver's local copy of events_list is modified.
> 
> With only one reader and one writer operating on the driver's buffer,
> you can use the normal kfifo_in and kfifo_out calls for lockless
> operations. Perhaps that is a way forward for this problem.

As proof of concept, I implemented the double buffered version with the
sysfs flush_events interface. Currently it feels kind of wired, I use
poll and wait until it timeouts to run the sysfs_flush_counter() to
trigger new data.

Here is example:
int main(void)
{
	ret = sysfs_enable_counter();
	...

	fd = open("/dev/counter0", O_RDWR);
	...

	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
	...

	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
	...

	for (;;) {
		struct pollfd fds[] = {
			{
				.fd = fd,
				.events = POLLIN,
			},
		};
		ssize_t i;

		/* wait for 10 sec */
		ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
		if (ret == -EINTR)
			continue;
		else if (ret < 0)
			return -errno;
		else if (ret == 0) {
			sysfs_flush_counter(); <---- request to flush queued events from the driver
			continue;
		}

		ret = read(fd, event_data, sizeof(event_data));
		...

		for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
			/* process event */
			....
		}
	}

	return ret;
}

If it is still the only way to go, I'll send kernel patches.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[David Lechner]

On 2/2/22 6:32 AM, Oleksij Rempel wrote:
> Hi William,
> 
> On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
> ...
>> So the counter_push_event() function interacts with two spinlocks:
>> events_list_lock and events_in_lock. The events_list_lock spinlock is
>> necessary because userspace can modify the events_list list via the
>> counter_enable_events() and counter_disable_events() functions. The
>> events_in_lock spinlock is necessary because userspace can modify the
>> events kfifo via the counter_events_queue_size_write() function.
>>
>> A lockless solution for this might be possible if the driver maintains
>> its own circular buffer as you suggest. The driver's IRQ handler can
>> write to this circular buffer without calling the counter_push_event()
>> function, and then flush the buffer to the Counter character device via
>> a userspace write to a "flush_events" sysfs attribute or similar; this
>> eliminates the need for the events_in_lock spinlock. The state of the
>> events_list list can be captured in the driver's events_configure()
>> callback and stored locally in the driver for reference, thus
>> eliminating the need for the events_list_lock; interrupts can be
>> disabled before the driver's local copy of events_list is modified.
>>
>> With only one reader and one writer operating on the driver's buffer,
>> you can use the normal kfifo_in and kfifo_out calls for lockless
>> operations. Perhaps that is a way forward for this problem.
> 
> As proof of concept, I implemented the double buffered version with the
> sysfs flush_events interface. Currently it feels kind of wired, I use
> poll and wait until it timeouts to run the sysfs_flush_counter() to
> trigger new data.
> 
> Here is example:
> int main(void)
> {
> 	ret = sysfs_enable_counter();
> 	...
> 
> 	fd = open("/dev/counter0", O_RDWR);
> 	...
> 
> 	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> 	...
> 
> 	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> 	...
> 
> 	for (;;) {
> 		struct pollfd fds[] = {
> 			{
> 				.fd = fd,
> 				.events = POLLIN,
> 			},
> 		};
> 		ssize_t i;
> 
> 		/* wait for 10 sec */
> 		ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
> 		if (ret == -EINTR)
> 			continue;
> 		else if (ret < 0)
> 			return -errno;
> 		else if (ret == 0) {
> 			sysfs_flush_counter(); <---- request to flush queued events from the driver
> 			continue;
> 		}
> 
> 		ret = read(fd, event_data, sizeof(event_data));
> 		...
> 
> 		for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
> 			/* process event */
> 			....
> 		}
> 	}
> 
> 	return ret;
> }
> 
> If it is still the only way to go, I'll send kernel patches.
> 
> Regards,
> Oleksij
> 

Couldn't the flush be implicit in the `read()` implementation
instead of requiring a separate sysfs attribute to trigger it?

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Oleksij Rempel]

On Wed, Feb 02, 2022 at 09:17:57AM -0600, David Lechner wrote:
> On 2/2/22 6:32 AM, Oleksij Rempel wrote:
> > Hi William,
> > 
> > On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
> > ...
> > > So the counter_push_event() function interacts with two spinlocks:
> > > events_list_lock and events_in_lock. The events_list_lock spinlock is
> > > necessary because userspace can modify the events_list list via the
> > > counter_enable_events() and counter_disable_events() functions. The
> > > events_in_lock spinlock is necessary because userspace can modify the
> > > events kfifo via the counter_events_queue_size_write() function.
> > > 
> > > A lockless solution for this might be possible if the driver maintains
> > > its own circular buffer as you suggest. The driver's IRQ handler can
> > > write to this circular buffer without calling the counter_push_event()
> > > function, and then flush the buffer to the Counter character device via
> > > a userspace write to a "flush_events" sysfs attribute or similar; this
> > > eliminates the need for the events_in_lock spinlock. The state of the
> > > events_list list can be captured in the driver's events_configure()
> > > callback and stored locally in the driver for reference, thus
> > > eliminating the need for the events_list_lock; interrupts can be
> > > disabled before the driver's local copy of events_list is modified.
> > > 
> > > With only one reader and one writer operating on the driver's buffer,
> > > you can use the normal kfifo_in and kfifo_out calls for lockless
> > > operations. Perhaps that is a way forward for this problem.
> > 
> > As proof of concept, I implemented the double buffered version with the
> > sysfs flush_events interface. Currently it feels kind of wired, I use
> > poll and wait until it timeouts to run the sysfs_flush_counter() to
> > trigger new data.
> > 
> > Here is example:
> > int main(void)
> > {
> > 	ret = sysfs_enable_counter();
> > 	...
> > 
> > 	fd = open("/dev/counter0", O_RDWR);
> > 	...
> > 
> > 	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> > 	...
> > 
> > 	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> > 	...
> > 
> > 	for (;;) {
> > 		struct pollfd fds[] = {
> > 			{
> > 				.fd = fd,
> > 				.events = POLLIN,
> > 			},
> > 		};
> > 		ssize_t i;
> > 
> > 		/* wait for 10 sec */
> > 		ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
> > 		if (ret == -EINTR)
> > 			continue;
> > 		else if (ret < 0)
> > 			return -errno;
> > 		else if (ret == 0) {
> > 			sysfs_flush_counter(); <---- request to flush queued events from the driver
> > 			continue;
> > 		}
> > 
> > 		ret = read(fd, event_data, sizeof(event_data));
> > 		...
> > 
> > 		for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
> > 			/* process event */
> > 			....
> > 		}
> > 	}
> > 
> > 	return ret;
> > }
> > 
> > If it is still the only way to go, I'll send kernel patches.
> > 
> > Regards,
> > Oleksij
> > 
> 
> Couldn't the flush be implicit in the `read()` implementation
> instead of requiring a separate sysfs attribute to trigger it?

Hm...

To detect pulse frequency, I need a burst of sequential time-stamps
without drops. In case the pulse frequency is higher then the use space
is able to get it out of FIFO, we will get high number of drops. 
So, we do not need all time stamps. Only bunch of them without drops in
the middle.

I know, at some frequency we wont be able to collect all pulses any way.
Internal FIFO is just increasing the max detectable frequency. So, it is
sort of optimization.

My current driver version has own FIFO which is filled directly by the
IRQ handler and user space trigger flush_cb to push all collected
time stamps. The main question is: how the flush procedure should be
controlled. We have following options:

- Attach it to the read(). The disadvantage: at high frequencies, we
  wont be able to get a burst with time stamps without drops in the
  middle
- Trigger flush from user space. In this case, we make user space a bit
  more complicated and cant really get all advantages of poll().
- kernel driver is using own timer to trigger flush. The timer can be
  configured from user space. The advantage of it, the user space is
  simple and has full advantage of using poll()

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 5064 bytes --]

On Thu, Feb 03, 2022 at 08:24:11AM +0100, Oleksij Rempel wrote:
> On Wed, Feb 02, 2022 at 09:17:57AM -0600, David Lechner wrote:
> > On 2/2/22 6:32 AM, Oleksij Rempel wrote:
> > > Hi William,
> > > 
> > > On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
> > > ...
> > > > So the counter_push_event() function interacts with two spinlocks:
> > > > events_list_lock and events_in_lock. The events_list_lock spinlock is
> > > > necessary because userspace can modify the events_list list via the
> > > > counter_enable_events() and counter_disable_events() functions. The
> > > > events_in_lock spinlock is necessary because userspace can modify the
> > > > events kfifo via the counter_events_queue_size_write() function.
> > > > 
> > > > A lockless solution for this might be possible if the driver maintains
> > > > its own circular buffer as you suggest. The driver's IRQ handler can
> > > > write to this circular buffer without calling the counter_push_event()
> > > > function, and then flush the buffer to the Counter character device via
> > > > a userspace write to a "flush_events" sysfs attribute or similar; this
> > > > eliminates the need for the events_in_lock spinlock. The state of the
> > > > events_list list can be captured in the driver's events_configure()
> > > > callback and stored locally in the driver for reference, thus
> > > > eliminating the need for the events_list_lock; interrupts can be
> > > > disabled before the driver's local copy of events_list is modified.
> > > > 
> > > > With only one reader and one writer operating on the driver's buffer,
> > > > you can use the normal kfifo_in and kfifo_out calls for lockless
> > > > operations. Perhaps that is a way forward for this problem.
> > > 
> > > As proof of concept, I implemented the double buffered version with the
> > > sysfs flush_events interface. Currently it feels kind of wired, I use
> > > poll and wait until it timeouts to run the sysfs_flush_counter() to
> > > trigger new data.
> > > 
> > > Here is example:
> > > int main(void)
> > > {
> > > 	ret = sysfs_enable_counter();
> > > 	...
> > > 
> > > 	fd = open("/dev/counter0", O_RDWR);
> > > 	...
> > > 
> > > 	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> > > 	...
> > > 
> > > 	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> > > 	...
> > > 
> > > 	for (;;) {
> > > 		struct pollfd fds[] = {
> > > 			{
> > > 				.fd = fd,
> > > 				.events = POLLIN,
> > > 			},
> > > 		};
> > > 		ssize_t i;
> > > 
> > > 		/* wait for 10 sec */
> > > 		ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
> > > 		if (ret == -EINTR)
> > > 			continue;
> > > 		else if (ret < 0)
> > > 			return -errno;
> > > 		else if (ret == 0) {
> > > 			sysfs_flush_counter(); <---- request to flush queued events from the driver
> > > 			continue;
> > > 		}
> > > 
> > > 		ret = read(fd, event_data, sizeof(event_data));
> > > 		...
> > > 
> > > 		for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
> > > 			/* process event */
> > > 			....
> > > 		}
> > > 	}
> > > 
> > > 	return ret;
> > > }
> > > 
> > > If it is still the only way to go, I'll send kernel patches.
> > > 
> > > Regards,
> > > Oleksij
> > > 
> > 
> > Couldn't the flush be implicit in the `read()` implementation
> > instead of requiring a separate sysfs attribute to trigger it?
> 
> Hm...
> 
> To detect pulse frequency, I need a burst of sequential time-stamps
> without drops. In case the pulse frequency is higher then the use space
> is able to get it out of FIFO, we will get high number of drops. 
> So, we do not need all time stamps. Only bunch of them without drops in
> the middle.
> 
> I know, at some frequency we wont be able to collect all pulses any way.
> Internal FIFO is just increasing the max detectable frequency. So, it is
> sort of optimization.
> 
> My current driver version has own FIFO which is filled directly by the
> IRQ handler and user space trigger flush_cb to push all collected
> time stamps. The main question is: how the flush procedure should be
> controlled. We have following options:
> 
> - Attach it to the read(). The disadvantage: at high frequencies, we
>   wont be able to get a burst with time stamps without drops in the
>   middle
> - Trigger flush from user space. In this case, we make user space a bit
>   more complicated and cant really get all advantages of poll().
> - kernel driver is using own timer to trigger flush. The timer can be
>   configured from user space. The advantage of it, the user space is
>   simple and has full advantage of using poll()
> 
> Regards,
> Oleksij

Hi Oleksij,

Earlier in this thread, Jonathan Cameron suggested using the RCU macros
to protect access to the events. Taking an RCU approach would eliminate
the need for spinlocks because the memory barriers are built-in to the
macros, so I assume flushing would no longer be necessary. Would RCU be
a viable solution for your needs?

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

[Oleksij Rempel]

Hi William,

On Thu, Feb 03, 2022 at 04:50:54PM +0900, William Breathitt Gray wrote:
> > Hm...
> > 
> > To detect pulse frequency, I need a burst of sequential time-stamps
> > without drops. In case the pulse frequency is higher then the use space
> > is able to get it out of FIFO, we will get high number of drops. 
> > So, we do not need all time stamps. Only bunch of them without drops in
> > the middle.
> > 
> > I know, at some frequency we wont be able to collect all pulses any way.
> > Internal FIFO is just increasing the max detectable frequency. So, it is
> > sort of optimization.
> > 
> > My current driver version has own FIFO which is filled directly by the
> > IRQ handler and user space trigger flush_cb to push all collected
> > time stamps. The main question is: how the flush procedure should be
> > controlled. We have following options:
> > 
> > - Attach it to the read(). The disadvantage: at high frequencies, we
> >   wont be able to get a burst with time stamps without drops in the
> >   middle
> > - Trigger flush from user space. In this case, we make user space a bit
> >   more complicated and cant really get all advantages of poll().
> > - kernel driver is using own timer to trigger flush. The timer can be
> >   configured from user space. The advantage of it, the user space is
> >   simple and has full advantage of using poll()
> > 
> > Regards,
> > Oleksij
> 
> Hi Oleksij,
> 
> Earlier in this thread, Jonathan Cameron suggested using the RCU macros
> to protect access to the events. Taking an RCU approach would eliminate
> the need for spinlocks because the memory barriers are built-in to the
> macros, so I assume flushing would no longer be necessary. Would RCU be
> a viable solution for your needs?

IMO, RCU is the wrong word in this context. It provide an advantage
where we need to reuse/read less frequently changed data. In this use
case we need to move data ASAP, so KFIFO seems to work just fine here.

In any case, after implementi double FIFO and more testing I would
prefer to stay with my initial patch. On a single core system, with have
no waiting time at all. No concurrent access. And on the SMP system
(iMX6Q), currently I can measure higher frequency with initial not
optimized driver:
- with counter_push_event() directly from IRQ: max freq 30-35kHz
- with double FIFO, i have max freq of ~25kHz

Your suggestion was to add COUNTER_EVENT_CHANGE_OF_STATE and use it for
my use case. Correct?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |