Re: [PATCH v2 13/23] counter: Provide alternative counter registration functions

[Uwe Kleine-König]

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