Messages in this thread | | | Subject | Re: [PATCH v5 3/5] counter: Add character device interface | From | David Lechner <> | Date | Tue, 20 Oct 2020 10:53:32 -0500 |
| |
>>> +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?
| |