On Wed, 29 Dec 2021 16:44:31 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Basically fine - a few trivial comments inline that I'm not that fussed
about whether you take notice of or not. As such
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
I'd have liked to have seen a change log here. Quite a few comments on this
one and not all had 'obvious' resolutions.
> drivers/counter/counter-core.c | 168 ++++++++++++++++++++++++++++++++-
> include/linux/counter.h | 15 +++
> 2 files changed, 181 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 00c41f28c101..b3fa15bbcbdb 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -15,6 +15,7 @@
> #include <linux/kdev_t.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/wait.h>
>
> @@ -24,6 +25,16 @@
> /* Provides a unique ID for each counter device */
> static DEFINE_IDA(counter_ida);
>
> +struct counter_device_allochelper {
> + struct counter_device counter;
> +
> + /*
> + * This is cache line aligned to ensure private data behaves like if it
> + * were kmalloced separately.
> + */
> + unsigned long privdata[] ____cacheline_aligned;
Change log for the patch would have made it easier to see you decided
to make this change after the discussion in v2.
> +};
> +
...
>
> +/**
> + * counter_alloc - allocate a counter_device
> + * @sizeof_priv: size of the driver private data
> + *
> + * This is part one of counter registration. The structure is allocated
> + * dynamically to ensure the right lifetime for the embedded struct device.
> + *
> + * If this succeeds, call counter_put() to get rid of the counter_device again.
> + */
> +struct counter_device *counter_alloc(size_t sizeof_priv)
> +{
> + struct counter_device_allochelper *ch;
> + struct counter_device *counter;
> + struct device *dev;
> + int err;
> +
> + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL);
> + if (!ch) {
> + err = -ENOMEM;
> + goto err_alloc_ch;
Slight preference for a direct return here even though it means
replicating the ERR_PTR() statement. Makes for one less error
path where a reviewer has to go see what is being done.
> + }
> +
> + counter = &ch->counter;
> + dev = &counter->dev;
> +
> + /* Acquire unique ID */
> + err = ida_alloc(&counter_ida, GFP_KERNEL);
> + if (err < 0)
> + goto err_ida_alloc;
> + dev->id = err;
> +
> + mutex_init(&counter->ops_exist_lock);
> + dev->type = &counter_device_type;
> + dev->bus = &counter_bus_type;
> + dev->devt = MKDEV(MAJOR(counter_devt), dev->id);
> +
> + err = counter_chrdev_add(counter);
> + if (err < 0)
> + goto err_chrdev_add;
> +
> + device_initialize(dev);
> +
> + return counter;
> +
> +err_chrdev_add:
> +
Nitpick: Unusual spacing (to my eye anyway). I wouldn't expect to see a blank line after a label
as the label indentation makes a visual separation anyway.
> + ida_free(&counter_ida, dev->id);
> +err_ida_alloc:
> +
> + kfree(ch);
> +err_alloc_ch:
> +
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(counter_alloc);
