On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
The current implementation gets device lifetime tracking wrong. The
problem is that allocation of struct counter_device is controlled by the
individual drivers but this structure contains a struct device that
might have to live longer than a driver is bound. As a result a command
sequence like:
{ sleep 5; echo bang; } > /dev/counter0 &
sleep 1;
echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind
can keep a reference to the struct device and unbinding results in
freeing the memory occupied by this device resulting in an oops.
This commit provides two new functions (plus some helpers):
- counter_alloc() to allocate a struct counter_device that is
automatically freed once the embedded struct device is released
- counter_add() to register such a device.
Note that this commit doesn't fix any issues, all drivers have to be
converted to these new functions to correct the lifetime problems.
Nice work!
[...]
@@ -24,6 +25,11 @@
/* Provides a unique ID for each counter device */
static DEFINE_IDA(counter_ida);
+struct counter_device_allochelper {
+ struct counter_device counter;
+ unsigned long privdata[];
The normal k*alloc functions guarantee that the allocation is cacheline
aligned. Without that for example the ____cacheline_aligned attribute
will not work correctly. And while it is unlikely that a counter driver
will need for example DMA memory I think it is still good practice to
guarantee this here to avoid bad surprises. There are two ways of doing
this.
Make privdata ____cacheline_aligned like in the memstick framework[1].
The downside is that this will reserve padding for the allocation, even
if no private data is allocated.
The alternative is to do something similar to IIO[2] which only
allocates the padding if there actually is any private data requested.
The disadvantage of that is that the code is a bit more cumbersome. And
most drivers will want to have some private data anyway so it might not
be worth it.
[1]
https://elixir.bootlin.com/linux/v5.16-rc7/source/include/linux/memstick.h#L292
[2]
https://elixir.bootlin.com/linux/v5.16-rc7/source/drivers/iio/industrialio-core.c#L1638
[...]
+struct counter_device *counter_alloc(size_t sizeof_priv)
I'd add a parent parameter here since with the devm_ variant we have to
pass it anyway and this allows to slightly reduce the boilerplate code
in the driver where the parent field of the counter has to be initialized.
+{
+ struct counter_device_allochelper *ch;
+ struct counter_device *counter;
+ struct device *dev;
+ int id, err;
+
+ ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL);
+ if (!ch) {
+ err = -ENOMEM;
+ goto err_alloc_ch;
+ }
+
+ counter = &ch->counter;
+ dev = &counter->dev;
+
+ /* Acquire unique ID */
+ err = ida_alloc(&counter_ida, GFP_KERNEL);
+ if (err < 0) {
+ goto err_ida_alloc;
+ }
There is a inconsistency whether braces are used for single statement
`if`s in this patch.
+ dev->id = err;
+
+ err = counter_chrdev_add(counter);
+ if (err < 0)
+ goto err_chrdev_add;
+
+ device_initialize(dev);
+ /* Configure device structure for Counter */
+ dev->type = &counter_device_type;
+ dev->bus = &counter_bus_type;
+ dev->devt = MKDEV(MAJOR(counter_devt), id);
+
+ mutex_init(&counter->ops_exist_lock);
+
+ return counter;
+
+err_chrdev_add:
+
+ ida_free(&counter_ida, dev->id);
+err_ida_alloc:
+
+ kfree(ch);
+err_alloc_ch:
+
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(counter_alloc);
+
+void counter_put(struct counter_device *counter)
+{
+ put_device(&counter->dev);
+}
+
+/**
+ * counter_add - complete registration of a counter
+ * @counter: the counter to add
+ *
+ * This is part two of counter registration.
+ *
+ * If this succeeds, call counter_unregister() to get rid of the counter_device again.
+ */
+int counter_add(struct counter_device *counter)
+{
+ int err;
+ struct device *dev = &counter->dev;
+
+ get_device(&counter->dev);
This get_device() is not needed. device_add() will also add a reference,
so you increment the reference count by 2 when calling counter_add().
It is only needed to balance the put_device() in counter_unregister().
I'd add a `counter->legacy_device` around that put_device() and then
remove it in the last patch.
The extra get_device() here is also leaked on the error paths, so
removing it will allow to keep the errors paths as they are.
+
+ if (counter->parent) {
+ dev->parent = counter->parent;
+ dev->of_node = counter->parent->of_node;
+ }
+
+ err = counter_sysfs_add(counter);
+ if (err < 0)
+ return err;
+
+ /* implies device_add(dev) */
+ err = cdev_device_add(&counter->chrdev, dev);
+
+ return err;
return cdev_device_add(...).
+struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv)
API Documentation?
+{
+ struct counter_device *counter;
+ int err;
+
+ counter = counter_alloc(sizeof_priv);
+ if (IS_ERR(counter))
+ return counter;
+
+ err = devm_add_action_or_reset(dev, devm_counter_put, counter);
+ if (err < 0)
+ return ERR_PTR(err);
+
+ return counter;
+}
+EXPORT_SYMBOL_GPL(devm_counter_alloc);
+
+int devm_counter_add(struct device *dev,
+ struct counter_device *const counter)
+{
API Documentation?
+ int err;
+
+ err = counter_add(counter);
+ if (err < 0)
+ return err;
+
+ return devm_add_action_or_reset(dev, devm_counter_release, counter);
+}
+EXPORT_SYMBOL_GPL(devm_counter_add);
+
#define COUNTER_DEV_MAX 256
