+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct counter_device *const counter = filp->private_data;
+ raw_spinlock_t *const events_lock = &counter->events_lock;
+ unsigned long flags;
+ struct list_head *const events_list = &counter->events_list;
+ struct list_head *const next_events_list = &counter->next_events_list;
+
+ switch (cmd) {
+ case COUNTER_CLEAR_WATCHES_IOCTL:
+ raw_spin_lock_irqsave(events_lock, flags);
+ counter_events_list_free(events_list);
+ raw_spin_unlock_irqrestore(events_lock, flags);
+ counter_events_list_free(next_events_list);
I think this ioctl is doing too much. If we have to use it for both
stopping events and clearing the list accumulated by
COUNTER_SET_WATCH_IOCTL, then we have a race condition of no events
after clearing watches during the time we are adding new ones and
until we load the new ones.
It would probably make more sense to call this ioctl
COUNTER_STOP_WATCHES_IOCTL and move counter_events_list_free(
next_events_list) to the end of COUNTER_LOAD_WATCHES_IOCTL.
I don't think we will necessarily have a race condition here.
COUNTER_CLEAR_WATCHES_IOCTL is intended to just clear the watches; e.g.
bring us back to a clear state when some sort of job has completely
finished and the user is no longer going to watch events for a while
(maybe they're adjusting the conveyor for the next job or some similar
operation).
I think the scenario you're concerned about is when you need to swap
watches in the middle of a job without losing events. In this case, you
wouldn't need to use COUNTER_CLEAR_WATCHES_IOCTL at all. Instead, you
would just set up the watches via COUNTER_SET_WATCH_IOCTL, and then use
COUNTER_LOAD_WATCHES_IOCTL to perform the swap; after
COUNTER_LOAD_WATCHES_IOCTL completes, next_events_list is empty (thanks
to list_replace_init()) and you're ready for the next set of watches.
Got it. I think I missed the fact that list_replace_init() clears
next_events_list.
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ unsigned long flags;
+
+ put_device(&counter->dev);
put_device() should be at the end of the function in case it is the last
reference.
put_device() shouldn't affect the counter_device events members, so I
don't think there's a difference in this case if it's called at the
beginning or end of the counter_chrdev_release function.
It isn't possible the some memory allocated with devm_kalloc() could be
be referenced after calling put_device() now or in the future?
+}
+EXPORT_SYMBOL_GPL(counter_push_event);
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index e66ed99dd5ea..cefef61f170d 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
Not sure why sysfs changes are in the chrdev patch. Are these
changes related somehow?
Sorry, I forgot to explain this in the cover letter. The changes here
are only useful for the character device interface. These changes
introduce the extensionZ_name and extensionZ_width sysfs attributes.
In the character device interface, extensions are selected by their id
number, and the value returned depends on the type of data. The new
sysfs attributes introduced here allow users to match the id of an
extension with its name, as well as the bit width of the value returned
so that the user knows whether to use the value_u8 or value_u64 union
member in struct counter_event.
Are we sure that all value types will always be CPU-endian unsigned
integers? Or should we make an enum to describe the data type instead
of just the width?
