| Message ID | 013b2b8682ddc3c85038083e6d5567696b6254b3.1613131238.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
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;
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; >
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
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
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?
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
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
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 --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;
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(+)