Hello Jonathan,
On Wed, Dec 29, 2021 at 05:06:12PM +0000, Jonathan Cameron wrote:
> 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>
Thanks.
> I'd have liked to have seen a change log here. Quite a few comments on this
> one and not all had 'obvious' resolutions.
yeah, should have been a bit less lazy and comment a bit more than the
range diff in the cover letter.
To catch up:
- privdata is now cache line aligned
- consistent bracing for oneline if blocks
- fixed the warning by the 0day bot which explained that only one
device can be bound
- reordered the body of counter_add() to better match
counter_register()
- Add an EXPORT_SYMBOL_GPL(counter_put);
- Drop one pair of get_device/put_device
- kernel doc for devm_counter_a{lloc,dd}
> > 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.
Yeah, this was a wim of the moment after I saw that this usually only
results in a 32 byte alignment.
> > +};
> > +
>
> ...
>
> >
> > +/**
> > + * 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.
I'll consider it if it comes to a v4.
> > + }
> > +
> > + 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.
I know this is unusual, but I like this approach. The error label is
named after what failed (instead of the more usual what has to be undone
first) and then it's grouped to the matching undo-function.
See
https://lore.kernel.org/linux-pwm/20201106093435.4mlr6ujivvkzkd5z@xxxxxxxxxxxxxx
for a more verbose reasoning (which however failed to convince my fellow
pwm maintainers :-\).
> > + ida_free(&counter_ida, dev->id);
> > +err_ida_alloc:
> > +
> > + kfree(ch);
> > +err_alloc_ch:
> > +
> > + return ERR_PTR(err);
> > +}
Best regards and thanks for your feedback,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
