Hello,
On Mon, Dec 27, 2021 at 01:16:56PM +0100, Lars-Peter Clausen wrote:
> 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!
\o/
> > [...]
> > @@ -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.
Yeah, I thought to add more alignment once the need arises. My
preference would be to got for the ____cacheline_aligned approach.
> > [...]
> > +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.
I don't feel strong here, can do that.
> > +{
> > + 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.
Oh, indeed.
> > + 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.
Ah indeed. I added that to balance the call of put_device() by devm_counter_alloc()'s
release function (devm_counter_put()). But on a quick check you're
right. I will think about it a bit more and test.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
