| Message ID | e298043c880b350a42bdc40452376a3708bf533b.1625471640.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jonathan Cameron |
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
Hi William,
url: https://github.com/0day-ci/linux/commits/William-Breathitt-Gray/Introduce-the-Counter-character-device-interface/20210705-162324
base: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
config: parisc-randconfig-m031-20210705 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/counter/counter-sysfs.c:815 counter_events_queue_size_write() warn: inconsistent returns '&counter->chrdev_lock'.
vim +815 drivers/counter/counter-sysfs.c
d95f8a2e83a813 William Breathitt Gray 2021-07-05 794 static int counter_events_queue_size_write(struct counter_device *counter,
d95f8a2e83a813 William Breathitt Gray 2021-07-05 795 u64 val)
d95f8a2e83a813 William Breathitt Gray 2021-07-05 796 {
d95f8a2e83a813 William Breathitt Gray 2021-07-05 797 int err;
d95f8a2e83a813 William Breathitt Gray 2021-07-05 798 DECLARE_KFIFO_PTR(events, struct counter_event);
d95f8a2e83a813 William Breathitt Gray 2021-07-05 799
d95f8a2e83a813 William Breathitt Gray 2021-07-05 800 /* Verify chrdev is not currently being used */
d95f8a2e83a813 William Breathitt Gray 2021-07-05 801 if (!mutex_trylock(&counter->chrdev_lock))
d95f8a2e83a813 William Breathitt Gray 2021-07-05 802 return -EBUSY;
d95f8a2e83a813 William Breathitt Gray 2021-07-05 803
d95f8a2e83a813 William Breathitt Gray 2021-07-05 804 /* Allocate new events queue */
d95f8a2e83a813 William Breathitt Gray 2021-07-05 805 err = kfifo_alloc(&events, val, GFP_ATOMIC);
d95f8a2e83a813 William Breathitt Gray 2021-07-05 806 if (err)
d95f8a2e83a813 William Breathitt Gray 2021-07-05 807 return err;
Drop the lock. With a mutex the alloc() doesn't necessarily have to
ATOMIC by the way.
d95f8a2e83a813 William Breathitt Gray 2021-07-05 808
d95f8a2e83a813 William Breathitt Gray 2021-07-05 809 /* Swap in new events queue */
d95f8a2e83a813 William Breathitt Gray 2021-07-05 810 kfifo_free(&counter->events);
d95f8a2e83a813 William Breathitt Gray 2021-07-05 811 counter->events.kfifo = events.kfifo;
d95f8a2e83a813 William Breathitt Gray 2021-07-05 812
d95f8a2e83a813 William Breathitt Gray 2021-07-05 813 mutex_unlock(&counter->chrdev_lock);
d95f8a2e83a813 William Breathitt Gray 2021-07-05 814
d95f8a2e83a813 William Breathitt Gray 2021-07-05 @815 return 0;
d95f8a2e83a813 William Breathitt Gray 2021-07-05 816 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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(+)