| Message ID | e298043c880b350a42bdc40452376a3708bf533b.1625471640.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
Hi On 7/5/21 11:19 AM, William Breathitt Gray 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. > ... > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c > index 92805b1f65b8..13644c87d02a 100644 > --- a/drivers/counter/counter-chrdev.c > +++ b/drivers/counter/counter-chrdev.c > @@ -323,6 +323,9 @@ static int counter_chrdev_open(struct inode *inode, struct file *filp) > typeof(*counter), > chrdev); > > + if (!mutex_trylock(&counter->chrdev_lock)) > + return -EBUSY; > + > get_device(&counter->dev); > filp->private_data = counter; > > @@ -339,6 +342,7 @@ static int counter_chrdev_release(struct inode *inode, struct file *filp) > return err; > > put_device(&counter->dev); > + mutex_unlock(&counter->chrdev_lock); > > return 0; > } I got two separate mutex warnings from counter_chrdev_open() by doing blind "cat /dev/counter0". First one due mutex being uninitialized: [ 441.057342] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 441.057355] WARNING: CPU: 2 PID: 366 at kernel/locking/mutex.c:1416 mutex_trylock+0xf2/0x130 ... [ 441.217331] Call Trace: [ 441.220062] counter_chrdev_open+0x21/0x60 [counter] ... which I fixed trivially by (please be free to use it) --- a/drivers/counter/counter-chrdev.c +++ b/drivers/counter/counter-chrdev.c @@ -364,6 +364,7 @@ int counter_chrdev_add(struct counter_device *const counter) spin_lock_init(&counter->events_list_lock); init_waitqueue_head(&counter->events_wait); mutex_init(&counter->events_lock); + mutex_init(&counter->chrdev_lock); /* Initialize character device */ cdev_init(&counter->chrdev, &counter_fops); and after that [ 16.564403] ================================================ [ 16.570725] WARNING: lock held when returning to user space! [ 16.577044] 5.13.0-next-20210706+ #4 Not tainted [ 16.582198] ------------------------------------------------ [ 16.588507] cat/331 is leaving the kernel with locks still held! [ 16.595214] 1 lock held by cat/331: [ 16.599103] #0: ffff888102bb3630 (&counter->chrdev_lock){+.+.}-{3:3}, at: counter_chrdev_open+0x21/0x60 [counter] Jarkko
On Tue, Jul 06, 2021 at 02:40:13PM +0300, Jarkko Nikula wrote: > Hi > > On 7/5/21 11:19 AM, William Breathitt Gray 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. > > > ... > > diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c > > index 92805b1f65b8..13644c87d02a 100644 > > --- a/drivers/counter/counter-chrdev.c > > +++ b/drivers/counter/counter-chrdev.c > > @@ -323,6 +323,9 @@ static int counter_chrdev_open(struct inode *inode, struct file *filp) > > typeof(*counter), > > chrdev); > > > > + if (!mutex_trylock(&counter->chrdev_lock)) > > + return -EBUSY; > > + > > get_device(&counter->dev); > > filp->private_data = counter; > > > > @@ -339,6 +342,7 @@ static int counter_chrdev_release(struct inode *inode, struct file *filp) > > return err; > > > > put_device(&counter->dev); > > + mutex_unlock(&counter->chrdev_lock); > > > > return 0; > > } > > I got two separate mutex warnings from counter_chrdev_open() by doing > blind "cat /dev/counter0". First one due mutex being uninitialized: > > [ 441.057342] DEBUG_LOCKS_WARN_ON(lock->magic != lock) > [ 441.057355] WARNING: CPU: 2 PID: 366 at kernel/locking/mutex.c:1416 > mutex_trylock+0xf2/0x130 > ... > [ 441.217331] Call Trace: > [ 441.220062] counter_chrdev_open+0x21/0x60 [counter] > ... > > which I fixed trivially by (please be free to use it) > > --- a/drivers/counter/counter-chrdev.c > +++ b/drivers/counter/counter-chrdev.c > @@ -364,6 +364,7 @@ int counter_chrdev_add(struct counter_device *const > counter) > spin_lock_init(&counter->events_list_lock); > init_waitqueue_head(&counter->events_wait); > mutex_init(&counter->events_lock); > + mutex_init(&counter->chrdev_lock); > > /* Initialize character device */ > cdev_init(&counter->chrdev, &counter_fops); Thanks, I'll add this line in. > and after that > > [ 16.564403] ================================================ > [ 16.570725] WARNING: lock held when returning to user space! > [ 16.577044] 5.13.0-next-20210706+ #4 Not tainted > [ 16.582198] ------------------------------------------------ > [ 16.588507] cat/331 is leaving the kernel with locks still held! > [ 16.595214] 1 lock held by cat/331: > [ 16.599103] #0: ffff888102bb3630 > (&counter->chrdev_lock){+.+.}-{3:3}, at: counter_chrdev_open+0x21/0x60 > [counter] > > Jarkko I'm not sure how to resolve this warning. The purpose of this lock is to limit chrdev to a single open at a time. To accomplish this I grab this lock in counter_chrdev_open() and hold it until counter_chrdev_release() is called. Is there a better way to accomplish this? William Breathitt Gray
On 7/10/21 5:25 AM, William Breathitt Gray wrote: >> and after that >> >> [ 16.564403] ================================================ >> [ 16.570725] WARNING: lock held when returning to user space! >> [ 16.577044] 5.13.0-next-20210706+ #4 Not tainted >> [ 16.582198] ------------------------------------------------ >> [ 16.588507] cat/331 is leaving the kernel with locks still held! >> [ 16.595214] 1 lock held by cat/331: >> [ 16.599103] #0: ffff888102bb3630 >> (&counter->chrdev_lock){+.+.}-{3:3}, at: counter_chrdev_open+0x21/0x60 >> [counter] >> >> Jarkko > I'm not sure how to resolve this warning. The purpose of this lock is to > limit chrdev to a single open at a time. To accomplish this I grab this > lock in counter_chrdev_open() and hold it until counter_chrdev_release() > is called. Is there a better way to accomplish this? How about using an atomic flag, e.g test_and_set_bit()?
On 7/10/21 10:43 AM, David Lechner wrote: > On 7/10/21 5:25 AM, William Breathitt Gray wrote: >>> and after that >>> >>> [ 16.564403] ================================================ >>> [ 16.570725] WARNING: lock held when returning to user space! >>> [ 16.577044] 5.13.0-next-20210706+ #4 Not tainted >>> [ 16.582198] ------------------------------------------------ >>> [ 16.588507] cat/331 is leaving the kernel with locks still held! >>> [ 16.595214] 1 lock held by cat/331: >>> [ 16.599103] #0: ffff888102bb3630 >>> (&counter->chrdev_lock){+.+.}-{3:3}, at: counter_chrdev_open+0x21/0x60 >>> [counter] >>> >>> Jarkko >> I'm not sure how to resolve this warning. The purpose of this lock is to >> limit chrdev to a single open at a time. To accomplish this I grab this >> lock in counter_chrdev_open() and hold it until counter_chrdev_release() >> is called. Is there a better way to accomplish this? > > How about using an atomic flag, e.g test_and_set_bit()? Another option could be to rethink it at a higher level and avoid the need for a lock (and sysfs attribute) altogether. For example, would it work to (re)allocate the kfifo buffer in the COUNTER_ENABLE_EVENTS_IOCTL callback and add a parameter to that ioctl to specify the buffer size (with units of events rather than bytes)?
On 7/10/21 11:08 AM, David Lechner wrote: > On 7/10/21 10:43 AM, David Lechner wrote: >> On 7/10/21 5:25 AM, William Breathitt Gray wrote: >>>> and after that >>>> >>>> [ 16.564403] ================================================ >>>> [ 16.570725] WARNING: lock held when returning to user space! >>>> [ 16.577044] 5.13.0-next-20210706+ #4 Not tainted >>>> [ 16.582198] ------------------------------------------------ >>>> [ 16.588507] cat/331 is leaving the kernel with locks still held! >>>> [ 16.595214] 1 lock held by cat/331: >>>> [ 16.599103] #0: ffff888102bb3630 >>>> (&counter->chrdev_lock){+.+.}-{3:3}, at: counter_chrdev_open+0x21/0x60 >>>> [counter] >>>> >>>> Jarkko >>> I'm not sure how to resolve this warning. The purpose of this lock is to >>> limit chrdev to a single open at a time. To accomplish this I grab this >>> lock in counter_chrdev_open() and hold it until counter_chrdev_release() >>> is called. Is there a better way to accomplish this? >> >> How about using an atomic flag, e.g test_and_set_bit()? > > Another option could be to rethink it at a higher level and avoid the > need for a lock (and sysfs attribute) altogether. For example, would it > work to (re)allocate the kfifo buffer in the COUNTER_ENABLE_EVENTS_IOCTL > callback and add a parameter to that ioctl to specify the buffer size > (with units of events rather than bytes)? > Thinking about it a bit more, this probably isn't a good suggestion. It would create issues with being able to start reading from the chrdev before enabling events.
On Sat, Jul 10, 2021 at 10:43:22AM -0500, David Lechner wrote: > On 7/10/21 5:25 AM, William Breathitt Gray wrote: > >> and after that > >> > >> [ 16.564403] ================================================ > >> [ 16.570725] WARNING: lock held when returning to user space! > >> [ 16.577044] 5.13.0-next-20210706+ #4 Not tainted > >> [ 16.582198] ------------------------------------------------ > >> [ 16.588507] cat/331 is leaving the kernel with locks still held! > >> [ 16.595214] 1 lock held by cat/331: > >> [ 16.599103] #0: ffff888102bb3630 > >> (&counter->chrdev_lock){+.+.}-{3:3}, at: counter_chrdev_open+0x21/0x60 > >> [counter] > >> > >> Jarkko > > I'm not sure how to resolve this warning. The purpose of this lock is to > > limit chrdev to a single open at a time. To accomplish this I grab this > > lock in counter_chrdev_open() and hold it until counter_chrdev_release() > > is called. Is there a better way to accomplish this? > > How about using an atomic flag, e.g test_and_set_bit()? Yes, I think this might work: atomically test and set the bit to lock and atomically clear it to unlock. I'll replace the mutex with an atomic flag. By the way, what is the difference between test_and_set_bit() and test_and_set_bit_lock()? William Breathitt Gray
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter index e0e99adb0ecc..84ebb1ed28ed 100644 --- a/Documentation/ABI/testing/sysfs-bus-counter +++ b/Documentation/ABI/testing/sysfs-bus-counter @@ -233,6 +233,14 @@ Description: shorter or equal to configured value are ignored. Value 0 means filter is disabled. +What: /sys/bus/counter/devices/counterX/events_queue_size +KernelVersion: 5.15 +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 92805b1f65b8..13644c87d02a 100644 --- a/drivers/counter/counter-chrdev.c +++ b/drivers/counter/counter-chrdev.c @@ -323,6 +323,9 @@ static int counter_chrdev_open(struct inode *inode, struct file *filp) typeof(*counter), chrdev); + if (!mutex_trylock(&counter->chrdev_lock)) + return -EBUSY; + get_device(&counter->dev); filp->private_data = counter; @@ -339,6 +342,7 @@ static int counter_chrdev_release(struct inode *inode, struct file *filp) return err; put_device(&counter->dev); + mutex_unlock(&counter->chrdev_lock); return 0; } diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c index eb1505bfbd89..bf038eff4587 100644 --- a/drivers/counter/counter-sysfs.c +++ b/drivers/counter/counter-sysfs.c @@ -8,7 +8,9 @@ #include <linux/err.h> #include <linux/gfp.h> #include <linux/kernel.h> +#include <linux/kfifo.h> #include <linux/list.h> +#include <linux/mutex.h> #include <linux/string.h> #include <linux/sysfs.h> #include <linux/types.h> @@ -782,12 +784,48 @@ 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 = kfifo_size(&counter->events); + return 0; +} + +static int counter_events_queue_size_write(struct counter_device *counter, + u64 val) +{ + int err; + DECLARE_KFIFO_PTR(events, struct counter_event); + + /* Verify chrdev is not currently being used */ + if (!mutex_trylock(&counter->chrdev_lock)) + return -EBUSY; + + /* Allocate new events queue */ + err = kfifo_alloc(&events, val, GFP_ATOMIC); + if (err) + return err; + + /* Swap in new events queue */ + kfifo_free(&counter->events); + counter->events.kfifo = events.kfifo; + + mutex_unlock(&counter->chrdev_lock); + + return 0; +} + 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) { @@ -826,6 +864,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; diff --git a/include/linux/counter.h b/include/linux/counter.h index 3f0bbe4ff702..854fcaf49c32 100644 --- a/include/linux/counter.h +++ b/include/linux/counter.h @@ -289,6 +289,7 @@ struct counter_ops { * @priv: optional private data supplied by driver * @dev: internal device structure * @chrdev: internal character device structure + * @chrdev_lock: lock to limit chrdev to a single open at a time * @events_list: list of current watching Counter events * @events_list_lock: lock to protect Counter events list operations * @next_events_list: list of next watching Counter events @@ -314,6 +315,7 @@ struct counter_device { struct device dev; struct cdev chrdev; + struct mutex chrdev_lock; struct list_head events_list; spinlock_t events_list_lock; struct list_head next_events_list;
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 | 4 ++ drivers/counter/counter-sysfs.c | 44 +++++++++++++++++++++ include/linux/counter.h | 2 + 4 files changed, 58 insertions(+)