lkml.org 
[lkml]   [2021]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 13/23] counter: Provide alternative counter registration functions
From
Date
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
>

\
 
 \ /
  Last update: 2021-12-27 13:18    [W:0.322 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site