diff mbox series

[v8,20/22] counter: Implement events_queue_size sysfs attribute

Message ID 013b2b8682ddc3c85038083e6d5567696b6254b3.1613131238.git.vilhelm.gray@gmail.com (mailing list archive)
State New
Headers show
Series Introduce the Counter character device interface | expand

Commit Message

William Breathitt Gray Feb. 12, 2021, 12:13 p.m. UTC
The events_queue_size sysfs attribute provides a way for users to
dynamically configure the Counter events queue size for the Counter
character device interface. The size is in number of struct
counter_event data structures. The number of elements will be rounded-up
to a power of 2 due to a requirement of the kfifo_alloc function called
during reallocation of the queue.

Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
 drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
 drivers/counter/counter-chrdev.h            |  2 ++
 drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
 4 files changed, 58 insertions(+)

Comments

Jonathan Cameron Feb. 14, 2021, 6:11 p.m. UTC | #1
On Fri, 12 Feb 2021 21:13:44 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> The events_queue_size sysfs attribute provides a way for users to
> dynamically configure the Counter events queue size for the Counter
> character device interface. The size is in number of struct
> counter_event data structures. The number of elements will be rounded-up
> to a power of 2 due to a requirement of the kfifo_alloc function called
> during reallocation of the queue.
> 
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
>  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
>  drivers/counter/counter-chrdev.h            |  2 ++
>  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
>  4 files changed, 58 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 847e96f19d19..f6cb2a8b08a7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -212,6 +212,14 @@ Description:
>  		both edges:
>  			Any state transition.
>  
> +What:		/sys/bus/counter/devices/counterX/events_queue_size
> +KernelVersion:	5.13
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Size of the Counter events queue in number of struct
> +		counter_event data structures. The number of elements will be
> +		rounded-up to a power of 2.
> +
>  What:		/sys/bus/counter/devices/counterX/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> index 16f02df7f73d..53eea894e13f 100644
> --- a/drivers/counter/counter-chrdev.c
> +++ b/drivers/counter/counter-chrdev.c
> @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
>  	cdev_del(&counter->chrdev);
>  }
>  
> +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> +				 size_t queue_size)
> +{
> +	int err;
> +	DECLARE_KFIFO_PTR(events, struct counter_event);
> +	unsigned long flags;
> +
> +	/* Allocate new events queue */
> +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);

Is there any potential for losing events?

> +	if (err)
> +		return err;
> +
> +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> +	/* Swap in new events queue */
> +	kfifo_free(&counter->events);
> +	counter->events.kfifo = events.kfifo;
> +
> +	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +
> +	return 0;
> +}
> +
>  static int counter_get_data(struct counter_device *const counter,
>  			    const struct counter_comp_node *const comp_node,
>  			    u64 *const value)
> diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
> index cf5a318fe540..ff7fb0191852 100644
> --- a/drivers/counter/counter-chrdev.h
> +++ b/drivers/counter/counter-chrdev.h
> @@ -12,5 +12,7 @@
>  int counter_chrdev_add(struct counter_device *const counter,
>  		       const dev_t counter_devt);
>  void counter_chrdev_remove(struct counter_device *const counter);
> +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> +				 size_t queue_size);
>  
>  #endif /* _COUNTER_CHRDEV_H_ */
> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> index 0cb3dba950bc..9abc821a3871 100644
> --- a/drivers/counter/counter-sysfs.c
> +++ b/drivers/counter/counter-sysfs.c
> @@ -13,6 +13,7 @@
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
>  
> +#include "counter-chrdev.h"
>  #include "counter-sysfs.h"
>  
>  /**
> @@ -737,12 +738,30 @@ static int counter_num_counts_read(struct counter_device *counter, u8 *val)
>  	return 0;
>  }
>  
> +static int counter_events_queue_size_read(struct counter_device *counter,
> +					  u64 *val)
> +{
> +	*val = counter->events.kfifo.mask + 1;

kfifo_size() rather than open coding it?

> +	return 0;
> +}
> +
> +static int counter_events_queue_size_write(struct counter_device *counter,
> +					   u64 val)
> +{
> +	return counter_chrdev_realloc_queue(counter, val);
> +}
> +
>  static struct counter_comp counter_num_signals_comp =
>  	COUNTER_COMP_DEVICE_U8("num_signals", counter_num_signals_read, NULL);
>  
>  static struct counter_comp counter_num_counts_comp =
>  	COUNTER_COMP_DEVICE_U8("num_counts", counter_num_counts_read, NULL);
>  
> +static struct counter_comp counter_events_queue_size_comp =
> +	COUNTER_COMP_DEVICE_U64("events_queue_size",
> +				counter_events_queue_size_read,
> +				counter_events_queue_size_write);
> +
>  static int counter_sysfs_attr_add(struct counter_device *const counter,
>  				  struct counter_attribute_group *group)
>  {
> @@ -781,6 +800,12 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  	if (err < 0)
>  		return err;
>  
> +	/* Create num_counts attribute */
> +	err = counter_attr_create(dev, group, &counter_events_queue_size_comp,
> +				  scope, NULL);
> +	if (err < 0)
> +		return err;
> +
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < counter->num_ext; i++) {
>  		ext = counter->ext + i;
William Breathitt Gray Feb. 18, 2021, 10:32 a.m. UTC | #2
On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
> On Fri, 12 Feb 2021 21:13:44 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > The events_queue_size sysfs attribute provides a way for users to
> > dynamically configure the Counter events queue size for the Counter
> > character device interface. The size is in number of struct
> > counter_event data structures. The number of elements will be rounded-up
> > to a power of 2 due to a requirement of the kfifo_alloc function called
> > during reallocation of the queue.
> > 
> > Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> >  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> >  drivers/counter/counter-chrdev.h            |  2 ++
> >  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> >  4 files changed, 58 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > index 847e96f19d19..f6cb2a8b08a7 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > @@ -212,6 +212,14 @@ Description:
> >  		both edges:
> >  			Any state transition.
> >  
> > +What:		/sys/bus/counter/devices/counterX/events_queue_size
> > +KernelVersion:	5.13
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Size of the Counter events queue in number of struct
> > +		counter_event data structures. The number of elements will be
> > +		rounded-up to a power of 2.
> > +
> >  What:		/sys/bus/counter/devices/counterX/name
> >  KernelVersion:	5.2
> >  Contact:	linux-iio@vger.kernel.org
> > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> > index 16f02df7f73d..53eea894e13f 100644
> > --- a/drivers/counter/counter-chrdev.c
> > +++ b/drivers/counter/counter-chrdev.c
> > @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> >  	cdev_del(&counter->chrdev);
> >  }
> >  
> > +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> > +				 size_t queue_size)
> > +{
> > +	int err;
> > +	DECLARE_KFIFO_PTR(events, struct counter_event);
> > +	unsigned long flags;
> > +
> > +	/* Allocate new events queue */
> > +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);
> 
> Is there any potential for losing events?

We take the events_list_lock down below so we're safe against missing an
event, but past events currently unread in the queue will be lost.

Shortening the size of the queue is inherently a destructive process if
we have more events in the current queue than can fit in the new queue.
Because we a liable to lose some events in such a case, I think it's
best to keep the behavior of this reallocation consistent and have it
provide a fresh empty queue every time, as opposed to sometimes dropping
events and sometimes not.

I also suspect an actual user would be setting the size of their queue
to the required amount before they begin watching events, rather than
adjusting it sporadically during a live operation.

> > +	if (err)
> > +		return err;
> > +
> > +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> > +
> > +	/* Swap in new events queue */
> > +	kfifo_free(&counter->events);
> > +	counter->events.kfifo = events.kfifo;
> > +
> > +	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> >  static int counter_get_data(struct counter_device *const counter,
> >  			    const struct counter_comp_node *const comp_node,
> >  			    u64 *const value)
> > diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
> > index cf5a318fe540..ff7fb0191852 100644
> > --- a/drivers/counter/counter-chrdev.h
> > +++ b/drivers/counter/counter-chrdev.h
> > @@ -12,5 +12,7 @@
> >  int counter_chrdev_add(struct counter_device *const counter,
> >  		       const dev_t counter_devt);
> >  void counter_chrdev_remove(struct counter_device *const counter);
> > +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> > +				 size_t queue_size);
> >  
> >  #endif /* _COUNTER_CHRDEV_H_ */
> > diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> > index 0cb3dba950bc..9abc821a3871 100644
> > --- a/drivers/counter/counter-sysfs.c
> > +++ b/drivers/counter/counter-sysfs.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/sysfs.h>
> >  #include <linux/types.h>
> >  
> > +#include "counter-chrdev.h"
> >  #include "counter-sysfs.h"
> >  
> >  /**
> > @@ -737,12 +738,30 @@ static int counter_num_counts_read(struct counter_device *counter, u8 *val)
> >  	return 0;
> >  }
> >  
> > +static int counter_events_queue_size_read(struct counter_device *counter,
> > +					  u64 *val)
> > +{
> > +	*val = counter->events.kfifo.mask + 1;
> 
> kfifo_size() rather than open coding it?

Ack.

William Breathitt Gray

> > +	return 0;
> > +}
> > +
> > +static int counter_events_queue_size_write(struct counter_device *counter,
> > +					   u64 val)
> > +{
> > +	return counter_chrdev_realloc_queue(counter, val);
> > +}
> > +
> >  static struct counter_comp counter_num_signals_comp =
> >  	COUNTER_COMP_DEVICE_U8("num_signals", counter_num_signals_read, NULL);
> >  
> >  static struct counter_comp counter_num_counts_comp =
> >  	COUNTER_COMP_DEVICE_U8("num_counts", counter_num_counts_read, NULL);
> >  
> > +static struct counter_comp counter_events_queue_size_comp =
> > +	COUNTER_COMP_DEVICE_U64("events_queue_size",
> > +				counter_events_queue_size_read,
> > +				counter_events_queue_size_write);
> > +
> >  static int counter_sysfs_attr_add(struct counter_device *const counter,
> >  				  struct counter_attribute_group *group)
> >  {
> > @@ -781,6 +800,12 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	/* Create num_counts attribute */
> > +	err = counter_attr_create(dev, group, &counter_events_queue_size_comp,
> > +				  scope, NULL);
> > +	if (err < 0)
> > +		return err;
> > +
> >  	/* Create an attribute for each extension */
> >  	for (i = 0; i < counter->num_ext; i++) {
> >  		ext = counter->ext + i;
>
Jonathan Cameron Feb. 21, 2021, 3:51 p.m. UTC | #3
On Thu, 18 Feb 2021 19:32:16 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
> > On Fri, 12 Feb 2021 21:13:44 +0900
> > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >   
> > > The events_queue_size sysfs attribute provides a way for users to
> > > dynamically configure the Counter events queue size for the Counter
> > > character device interface. The size is in number of struct
> > > counter_event data structures. The number of elements will be rounded-up
> > > to a power of 2 due to a requirement of the kfifo_alloc function called
> > > during reallocation of the queue.
> > > 
> > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> > >  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> > >  drivers/counter/counter-chrdev.h            |  2 ++
> > >  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> > >  4 files changed, 58 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > index 847e96f19d19..f6cb2a8b08a7 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > @@ -212,6 +212,14 @@ Description:
> > >  		both edges:
> > >  			Any state transition.
> > >  
> > > +What:		/sys/bus/counter/devices/counterX/events_queue_size
> > > +KernelVersion:	5.13
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Size of the Counter events queue in number of struct
> > > +		counter_event data structures. The number of elements will be
> > > +		rounded-up to a power of 2.
> > > +
> > >  What:		/sys/bus/counter/devices/counterX/name
> > >  KernelVersion:	5.2
> > >  Contact:	linux-iio@vger.kernel.org
> > > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> > > index 16f02df7f73d..53eea894e13f 100644
> > > --- a/drivers/counter/counter-chrdev.c
> > > +++ b/drivers/counter/counter-chrdev.c
> > > @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> > >  	cdev_del(&counter->chrdev);
> > >  }
> > >  
> > > +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> > > +				 size_t queue_size)
> > > +{
> > > +	int err;
> > > +	DECLARE_KFIFO_PTR(events, struct counter_event);
> > > +	unsigned long flags;
> > > +
> > > +	/* Allocate new events queue */
> > > +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);  
> > 
> > Is there any potential for losing events?  
> 
> We take the events_list_lock down below so we're safe against missing an
> event, but past events currently unread in the queue will be lost.
> 
> Shortening the size of the queue is inherently a destructive process if
> we have more events in the current queue than can fit in the new queue.
> Because we a liable to lose some events in such a case, I think it's
> best to keep the behavior of this reallocation consistent and have it
> provide a fresh empty queue every time, as opposed to sometimes dropping
> events and sometimes not.
> 
> I also suspect an actual user would be setting the size of their queue
> to the required amount before they begin watching events, rather than
> adjusting it sporadically during a live operation.
>

Absolutely agree.   As such I wonder if you are better off enforcing this
behaviour?  If the cdev is open for reading, don't allow the fifo to be
resized. 

Jonathan
William Breathitt Gray Feb. 26, 2021, 12:03 a.m. UTC | #4
On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
> On Thu, 18 Feb 2021 19:32:16 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
> > > On Fri, 12 Feb 2021 21:13:44 +0900
> > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > >   
> > > > The events_queue_size sysfs attribute provides a way for users to
> > > > dynamically configure the Counter events queue size for the Counter
> > > > character device interface. The size is in number of struct
> > > > counter_event data structures. The number of elements will be rounded-up
> > > > to a power of 2 due to a requirement of the kfifo_alloc function called
> > > > during reallocation of the queue.
> > > > 
> > > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> > > >  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> > > >  drivers/counter/counter-chrdev.h            |  2 ++
> > > >  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> > > >  4 files changed, 58 insertions(+)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > > index 847e96f19d19..f6cb2a8b08a7 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > > @@ -212,6 +212,14 @@ Description:
> > > >  		both edges:
> > > >  			Any state transition.
> > > >  
> > > > +What:		/sys/bus/counter/devices/counterX/events_queue_size
> > > > +KernelVersion:	5.13
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Size of the Counter events queue in number of struct
> > > > +		counter_event data structures. The number of elements will be
> > > > +		rounded-up to a power of 2.
> > > > +
> > > >  What:		/sys/bus/counter/devices/counterX/name
> > > >  KernelVersion:	5.2
> > > >  Contact:	linux-iio@vger.kernel.org
> > > > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> > > > index 16f02df7f73d..53eea894e13f 100644
> > > > --- a/drivers/counter/counter-chrdev.c
> > > > +++ b/drivers/counter/counter-chrdev.c
> > > > @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> > > >  	cdev_del(&counter->chrdev);
> > > >  }
> > > >  
> > > > +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> > > > +				 size_t queue_size)
> > > > +{
> > > > +	int err;
> > > > +	DECLARE_KFIFO_PTR(events, struct counter_event);
> > > > +	unsigned long flags;
> > > > +
> > > > +	/* Allocate new events queue */
> > > > +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);  
> > > 
> > > Is there any potential for losing events?  
> > 
> > We take the events_list_lock down below so we're safe against missing an
> > event, but past events currently unread in the queue will be lost.
> > 
> > Shortening the size of the queue is inherently a destructive process if
> > we have more events in the current queue than can fit in the new queue.
> > Because we a liable to lose some events in such a case, I think it's
> > best to keep the behavior of this reallocation consistent and have it
> > provide a fresh empty queue every time, as opposed to sometimes dropping
> > events and sometimes not.
> > 
> > I also suspect an actual user would be setting the size of their queue
> > to the required amount before they begin watching events, rather than
> > adjusting it sporadically during a live operation.
> >
> 
> Absolutely agree.   As such I wonder if you are better off enforcing this
> behaviour?  If the cdev is open for reading, don't allow the fifo to be
> resized. 
> 
> Jonathan

I can't really think of a good reason not to, so let's enforce it: if
the cdev is open, then we'll return an EINVAL if the user attempts to
resize the queue.

What is a good way to check for this condition? Should I just call
kref_read() and see if it's greater than 1? For example, in
counter_chrdev_realloc_queue():

	if (kref_read(&counter->dev.kobj.kref) > 1)
		return -EINVAL;

William Breathitt Gray
David Lechner Feb. 27, 2021, 12:14 a.m. UTC | #5
On 2/25/21 6:03 PM, William Breathitt Gray wrote:
> On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
>> On Thu, 18 Feb 2021 19:32:16 +0900
>> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>>
>>> On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
>>>> On Fri, 12 Feb 2021 21:13:44 +0900
>>>> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>>>>    
>>>>> The events_queue_size sysfs attribute provides a way for users to
>>>>> dynamically configure the Counter events queue size for the Counter
>>>>> character device interface. The size is in number of struct
>>>>> counter_event data structures. The number of elements will be rounded-up
>>>>> to a power of 2 due to a requirement of the kfifo_alloc function called
>>>>> during reallocation of the queue.
>>>>>
>>>>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>>>>> ---
>>>>>   Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
>>>>>   drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
>>>>>   drivers/counter/counter-chrdev.h            |  2 ++
>>>>>   drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
>>>>>   4 files changed, 58 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
>>>>> index 847e96f19d19..f6cb2a8b08a7 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-counter
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-counter
>>>>> @@ -212,6 +212,14 @@ Description:
>>>>>   		both edges:
>>>>>   			Any state transition.
>>>>>   
>>>>> +What:		/sys/bus/counter/devices/counterX/events_queue_size
>>>>> +KernelVersion:	5.13
>>>>> +Contact:	linux-iio@vger.kernel.org
>>>>> +Description:
>>>>> +		Size of the Counter events queue in number of struct
>>>>> +		counter_event data structures. The number of elements will be
>>>>> +		rounded-up to a power of 2.
>>>>> +
>>>>>   What:		/sys/bus/counter/devices/counterX/name
>>>>>   KernelVersion:	5.2
>>>>>   Contact:	linux-iio@vger.kernel.org
>>>>> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
>>>>> index 16f02df7f73d..53eea894e13f 100644
>>>>> --- a/drivers/counter/counter-chrdev.c
>>>>> +++ b/drivers/counter/counter-chrdev.c
>>>>> @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
>>>>>   	cdev_del(&counter->chrdev);
>>>>>   }
>>>>>   
>>>>> +int counter_chrdev_realloc_queue(struct counter_device *const counter,
>>>>> +				 size_t queue_size)
>>>>> +{
>>>>> +	int err;
>>>>> +	DECLARE_KFIFO_PTR(events, struct counter_event);
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	/* Allocate new events queue */
>>>>> +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);
>>>>
>>>> Is there any potential for losing events?
>>>
>>> We take the events_list_lock down below so we're safe against missing an
>>> event, but past events currently unread in the queue will be lost.
>>>
>>> Shortening the size of the queue is inherently a destructive process if
>>> we have more events in the current queue than can fit in the new queue.
>>> Because we a liable to lose some events in such a case, I think it's
>>> best to keep the behavior of this reallocation consistent and have it
>>> provide a fresh empty queue every time, as opposed to sometimes dropping
>>> events and sometimes not.
>>>
>>> I also suspect an actual user would be setting the size of their queue
>>> to the required amount before they begin watching events, rather than
>>> adjusting it sporadically during a live operation.
>>>
>>
>> Absolutely agree.   As such I wonder if you are better off enforcing this
>> behaviour?  If the cdev is open for reading, don't allow the fifo to be
>> resized.
>>
>> Jonathan
> 
> I can't really think of a good reason not to, so let's enforce it: if
> the cdev is open, then we'll return an EINVAL if the user attempts to
> resize the queue.
> 
> What is a good way to check for this condition? Should I just call
> kref_read() and see if it's greater than 1? For example, in
> counter_chrdev_realloc_queue():
> 
> 	if (kref_read(&counter->dev.kobj.kref) > 1)
> 		return -EINVAL;
> 
> William Breathitt Gray
> 

Wouldn't EBUSY make more sense?
William Breathitt Gray Feb. 27, 2021, 12:20 a.m. UTC | #6
On Fri, Feb 26, 2021 at 06:14:12PM -0600, David Lechner wrote:
> On 2/25/21 6:03 PM, William Breathitt Gray wrote:
> > On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
> >> On Thu, 18 Feb 2021 19:32:16 +0900
> >> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >>
> >>> On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
> >>>> On Fri, 12 Feb 2021 21:13:44 +0900
> >>>> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >>>>    
> >>>>> The events_queue_size sysfs attribute provides a way for users to
> >>>>> dynamically configure the Counter events queue size for the Counter
> >>>>> character device interface. The size is in number of struct
> >>>>> counter_event data structures. The number of elements will be rounded-up
> >>>>> to a power of 2 due to a requirement of the kfifo_alloc function called
> >>>>> during reallocation of the queue.
> >>>>>
> >>>>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> >>>>> ---
> >>>>>   Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> >>>>>   drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> >>>>>   drivers/counter/counter-chrdev.h            |  2 ++
> >>>>>   drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> >>>>>   4 files changed, 58 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> >>>>> index 847e96f19d19..f6cb2a8b08a7 100644
> >>>>> --- a/Documentation/ABI/testing/sysfs-bus-counter
> >>>>> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> >>>>> @@ -212,6 +212,14 @@ Description:
> >>>>>   		both edges:
> >>>>>   			Any state transition.
> >>>>>   
> >>>>> +What:		/sys/bus/counter/devices/counterX/events_queue_size
> >>>>> +KernelVersion:	5.13
> >>>>> +Contact:	linux-iio@vger.kernel.org
> >>>>> +Description:
> >>>>> +		Size of the Counter events queue in number of struct
> >>>>> +		counter_event data structures. The number of elements will be
> >>>>> +		rounded-up to a power of 2.
> >>>>> +
> >>>>>   What:		/sys/bus/counter/devices/counterX/name
> >>>>>   KernelVersion:	5.2
> >>>>>   Contact:	linux-iio@vger.kernel.org
> >>>>> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> >>>>> index 16f02df7f73d..53eea894e13f 100644
> >>>>> --- a/drivers/counter/counter-chrdev.c
> >>>>> +++ b/drivers/counter/counter-chrdev.c
> >>>>> @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> >>>>>   	cdev_del(&counter->chrdev);
> >>>>>   }
> >>>>>   
> >>>>> +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> >>>>> +				 size_t queue_size)
> >>>>> +{
> >>>>> +	int err;
> >>>>> +	DECLARE_KFIFO_PTR(events, struct counter_event);
> >>>>> +	unsigned long flags;
> >>>>> +
> >>>>> +	/* Allocate new events queue */
> >>>>> +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);
> >>>>
> >>>> Is there any potential for losing events?
> >>>
> >>> We take the events_list_lock down below so we're safe against missing an
> >>> event, but past events currently unread in the queue will be lost.
> >>>
> >>> Shortening the size of the queue is inherently a destructive process if
> >>> we have more events in the current queue than can fit in the new queue.
> >>> Because we a liable to lose some events in such a case, I think it's
> >>> best to keep the behavior of this reallocation consistent and have it
> >>> provide a fresh empty queue every time, as opposed to sometimes dropping
> >>> events and sometimes not.
> >>>
> >>> I also suspect an actual user would be setting the size of their queue
> >>> to the required amount before they begin watching events, rather than
> >>> adjusting it sporadically during a live operation.
> >>>
> >>
> >> Absolutely agree.   As such I wonder if you are better off enforcing this
> >> behaviour?  If the cdev is open for reading, don't allow the fifo to be
> >> resized.
> >>
> >> Jonathan
> > 
> > I can't really think of a good reason not to, so let's enforce it: if
> > the cdev is open, then we'll return an EINVAL if the user attempts to
> > resize the queue.
> > 
> > What is a good way to check for this condition? Should I just call
> > kref_read() and see if it's greater than 1? For example, in
> > counter_chrdev_realloc_queue():
> > 
> > 	if (kref_read(&counter->dev.kobj.kref) > 1)
> > 		return -EINVAL;
> > 
> > William Breathitt Gray
> > 
> 
> Wouldn't EBUSY make more sense?

Yes, EBUSY would be better here because the operation isn't necessarily
invalid, just unavailable because someone else has the cdev open.

William Breathitt Gray
Jonathan Cameron Feb. 27, 2021, 3:18 p.m. UTC | #7
On Fri, 26 Feb 2021 09:03:48 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
> > On Thu, 18 Feb 2021 19:32:16 +0900
> > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >   
> > > On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:  
> > > > On Fri, 12 Feb 2021 21:13:44 +0900
> > > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > > >     
> > > > > The events_queue_size sysfs attribute provides a way for users to
> > > > > dynamically configure the Counter events queue size for the Counter
> > > > > character device interface. The size is in number of struct
> > > > > counter_event data structures. The number of elements will be rounded-up
> > > > > to a power of 2 due to a requirement of the kfifo_alloc function called
> > > > > during reallocation of the queue.
> > > > > 
> > > > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> > > > >  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> > > > >  drivers/counter/counter-chrdev.h            |  2 ++
> > > > >  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> > > > >  4 files changed, 58 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > > > index 847e96f19d19..f6cb2a8b08a7 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > > > @@ -212,6 +212,14 @@ Description:
> > > > >  		both edges:
> > > > >  			Any state transition.
> > > > >  
> > > > > +What:		/sys/bus/counter/devices/counterX/events_queue_size
> > > > > +KernelVersion:	5.13
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Size of the Counter events queue in number of struct
> > > > > +		counter_event data structures. The number of elements will be
> > > > > +		rounded-up to a power of 2.
> > > > > +
> > > > >  What:		/sys/bus/counter/devices/counterX/name
> > > > >  KernelVersion:	5.2
> > > > >  Contact:	linux-iio@vger.kernel.org
> > > > > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> > > > > index 16f02df7f73d..53eea894e13f 100644
> > > > > --- a/drivers/counter/counter-chrdev.c
> > > > > +++ b/drivers/counter/counter-chrdev.c
> > > > > @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> > > > >  	cdev_del(&counter->chrdev);
> > > > >  }
> > > > >  
> > > > > +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> > > > > +				 size_t queue_size)
> > > > > +{
> > > > > +	int err;
> > > > > +	DECLARE_KFIFO_PTR(events, struct counter_event);
> > > > > +	unsigned long flags;
> > > > > +
> > > > > +	/* Allocate new events queue */
> > > > > +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);    
> > > > 
> > > > Is there any potential for losing events?    
> > > 
> > > We take the events_list_lock down below so we're safe against missing an
> > > event, but past events currently unread in the queue will be lost.
> > > 
> > > Shortening the size of the queue is inherently a destructive process if
> > > we have more events in the current queue than can fit in the new queue.
> > > Because we a liable to lose some events in such a case, I think it's
> > > best to keep the behavior of this reallocation consistent and have it
> > > provide a fresh empty queue every time, as opposed to sometimes dropping
> > > events and sometimes not.
> > > 
> > > I also suspect an actual user would be setting the size of their queue
> > > to the required amount before they begin watching events, rather than
> > > adjusting it sporadically during a live operation.
> > >  
> > 
> > Absolutely agree.   As such I wonder if you are better off enforcing this
> > behaviour?  If the cdev is open for reading, don't allow the fifo to be
> > resized. 
> > 
> > Jonathan  
> 
> I can't really think of a good reason not to, so let's enforce it: if
> the cdev is open, then we'll return an EINVAL if the user attempts to
> resize the queue.
> 
> What is a good way to check for this condition? Should I just call
> kref_read() and see if it's greater than 1? For example, in
> counter_chrdev_realloc_queue():
> 
> 	if (kref_read(&counter->dev.kobj.kref) > 1)
> 		return -EINVAL;
In theory at least you might want the kobj.kref to be incremented
for other reasons than just open.   So to keep different concepts
separate, perhaps it's worth a separate variable somewhere to
track whether the file is open currently.

However, it's reasonable (I think) to assume the kref will have a
minimum value if open, so perhaps what you suggest works fine.

Jonathan

> 
> William Breathitt Gray
William Breathitt Gray Feb. 28, 2021, 2:46 a.m. UTC | #8
On Sat, Feb 27, 2021 at 03:18:47PM +0000, Jonathan Cameron wrote:
> On Fri, 26 Feb 2021 09:03:48 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
> > > On Thu, 18 Feb 2021 19:32:16 +0900
> > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > >   
> > > > On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:  
> > > > > On Fri, 12 Feb 2021 21:13:44 +0900
> > > > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > > > >     
> > > > > > The events_queue_size sysfs attribute provides a way for users to
> > > > > > dynamically configure the Counter events queue size for the Counter
> > > > > > character device interface. The size is in number of struct
> > > > > > counter_event data structures. The number of elements will be rounded-up
> > > > > > to a power of 2 due to a requirement of the kfifo_alloc function called
> > > > > > during reallocation of the queue.
> > > > > > 
> > > > > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > > > > ---
> > > > > >  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
> > > > > >  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
> > > > > >  drivers/counter/counter-chrdev.h            |  2 ++
> > > > > >  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
> > > > > >  4 files changed, 58 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > > > > index 847e96f19d19..f6cb2a8b08a7 100644
> > > > > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > > > > @@ -212,6 +212,14 @@ Description:
> > > > > >  		both edges:
> > > > > >  			Any state transition.
> > > > > >  
> > > > > > +What:		/sys/bus/counter/devices/counterX/events_queue_size
> > > > > > +KernelVersion:	5.13
> > > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > > +Description:
> > > > > > +		Size of the Counter events queue in number of struct
> > > > > > +		counter_event data structures. The number of elements will be
> > > > > > +		rounded-up to a power of 2.
> > > > > > +
> > > > > >  What:		/sys/bus/counter/devices/counterX/name
> > > > > >  KernelVersion:	5.2
> > > > > >  Contact:	linux-iio@vger.kernel.org
> > > > > > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> > > > > > index 16f02df7f73d..53eea894e13f 100644
> > > > > > --- a/drivers/counter/counter-chrdev.c
> > > > > > +++ b/drivers/counter/counter-chrdev.c
> > > > > > @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
> > > > > >  	cdev_del(&counter->chrdev);
> > > > > >  }
> > > > > >  
> > > > > > +int counter_chrdev_realloc_queue(struct counter_device *const counter,
> > > > > > +				 size_t queue_size)
> > > > > > +{
> > > > > > +	int err;
> > > > > > +	DECLARE_KFIFO_PTR(events, struct counter_event);
> > > > > > +	unsigned long flags;
> > > > > > +
> > > > > > +	/* Allocate new events queue */
> > > > > > +	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);    
> > > > > 
> > > > > Is there any potential for losing events?    
> > > > 
> > > > We take the events_list_lock down below so we're safe against missing an
> > > > event, but past events currently unread in the queue will be lost.
> > > > 
> > > > Shortening the size of the queue is inherently a destructive process if
> > > > we have more events in the current queue than can fit in the new queue.
> > > > Because we a liable to lose some events in such a case, I think it's
> > > > best to keep the behavior of this reallocation consistent and have it
> > > > provide a fresh empty queue every time, as opposed to sometimes dropping
> > > > events and sometimes not.
> > > > 
> > > > I also suspect an actual user would be setting the size of their queue
> > > > to the required amount before they begin watching events, rather than
> > > > adjusting it sporadically during a live operation.
> > > >  
> > > 
> > > Absolutely agree.   As such I wonder if you are better off enforcing this
> > > behaviour?  If the cdev is open for reading, don't allow the fifo to be
> > > resized. 
> > > 
> > > Jonathan  
> > 
> > I can't really think of a good reason not to, so let's enforce it: if
> > the cdev is open, then we'll return an EINVAL if the user attempts to
> > resize the queue.
> > 
> > What is a good way to check for this condition? Should I just call
> > kref_read() and see if it's greater than 1? For example, in
> > counter_chrdev_realloc_queue():
> > 
> > 	if (kref_read(&counter->dev.kobj.kref) > 1)
> > 		return -EINVAL;
> In theory at least you might want the kobj.kref to be incremented
> for other reasons than just open.   So to keep different concepts
> separate, perhaps it's worth a separate variable somewhere to
> track whether the file is open currently.
> 
> However, it's reasonable (I think) to assume the kref will have a
> minimum value if open, so perhaps what you suggest works fine.
> 
> Jonathan

I noticed an open() operation could occur right after this check, so
we'll need a mutex here to ensure the the queue size is not modified
during use. Because of that, I'll create a separate variable to track
this and use a mutex_trylock() instead of the kref to test whether to
return -EBUSY.

William Breathitt Gray
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 847e96f19d19..f6cb2a8b08a7 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -212,6 +212,14 @@  Description:
 		both edges:
 			Any state transition.
 
+What:		/sys/bus/counter/devices/counterX/events_queue_size
+KernelVersion:	5.13
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Size of the Counter events queue in number of struct
+		counter_event data structures. The number of elements will be
+		rounded-up to a power of 2.
+
 What:		/sys/bus/counter/devices/counterX/name
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
index 16f02df7f73d..53eea894e13f 100644
--- a/drivers/counter/counter-chrdev.c
+++ b/drivers/counter/counter-chrdev.c
@@ -375,6 +375,29 @@  void counter_chrdev_remove(struct counter_device *const counter)
 	cdev_del(&counter->chrdev);
 }
 
+int counter_chrdev_realloc_queue(struct counter_device *const counter,
+				 size_t queue_size)
+{
+	int err;
+	DECLARE_KFIFO_PTR(events, struct counter_event);
+	unsigned long flags;
+
+	/* Allocate new events queue */
+	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);
+	if (err)
+		return err;
+
+	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	/* Swap in new events queue */
+	kfifo_free(&counter->events);
+	counter->events.kfifo = events.kfifo;
+
+	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+
+	return 0;
+}
+
 static int counter_get_data(struct counter_device *const counter,
 			    const struct counter_comp_node *const comp_node,
 			    u64 *const value)
diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
index cf5a318fe540..ff7fb0191852 100644
--- a/drivers/counter/counter-chrdev.h
+++ b/drivers/counter/counter-chrdev.h
@@ -12,5 +12,7 @@ 
 int counter_chrdev_add(struct counter_device *const counter,
 		       const dev_t counter_devt);
 void counter_chrdev_remove(struct counter_device *const counter);
+int counter_chrdev_realloc_queue(struct counter_device *const counter,
+				 size_t queue_size);
 
 #endif /* _COUNTER_CHRDEV_H_ */
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 0cb3dba950bc..9abc821a3871 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -13,6 +13,7 @@ 
 #include <linux/sysfs.h>
 #include <linux/types.h>
 
+#include "counter-chrdev.h"
 #include "counter-sysfs.h"
 
 /**
@@ -737,12 +738,30 @@  static int counter_num_counts_read(struct counter_device *counter, u8 *val)
 	return 0;
 }
 
+static int counter_events_queue_size_read(struct counter_device *counter,
+					  u64 *val)
+{
+	*val = counter->events.kfifo.mask + 1;
+	return 0;
+}
+
+static int counter_events_queue_size_write(struct counter_device *counter,
+					   u64 val)
+{
+	return counter_chrdev_realloc_queue(counter, val);
+}
+
 static struct counter_comp counter_num_signals_comp =
 	COUNTER_COMP_DEVICE_U8("num_signals", counter_num_signals_read, NULL);
 
 static struct counter_comp counter_num_counts_comp =
 	COUNTER_COMP_DEVICE_U8("num_counts", counter_num_counts_read, NULL);
 
+static struct counter_comp counter_events_queue_size_comp =
+	COUNTER_COMP_DEVICE_U64("events_queue_size",
+				counter_events_queue_size_read,
+				counter_events_queue_size_write);
+
 static int counter_sysfs_attr_add(struct counter_device *const counter,
 				  struct counter_attribute_group *group)
 {
@@ -781,6 +800,12 @@  static int counter_sysfs_attr_add(struct counter_device *const counter,
 	if (err < 0)
 		return err;
 
+	/* Create num_counts attribute */
+	err = counter_attr_create(dev, group, &counter_events_queue_size_comp,
+				  scope, NULL);
+	if (err < 0)
+		return err;
+
 	/* Create an attribute for each extension */
 	for (i = 0; i < counter->num_ext; i++) {
 		ext = counter->ext + i;