Hello,
On Tue, Dec 28, 2021 at 06:00:17PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Dec 2021 10:45:16 +0100
> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> > +struct counter_device_allochelper {
> > + struct counter_device counter;
> > + unsigned long privdata[];
> Nice to keep this opaque. We danced around how to do this in IIO for
> a while and never got to a solution we all liked. Slight disadvantage
> is this might matter in hot paths where the compiler doesn't have visibility
> to know it can very cheaply access the privdata.
>
> I guess that can be looked at if anyone ever measures it as an important
> element (they probably never will!)
*nod*
> > +};
> > +
> > static void counter_device_release(struct device *dev)
> > {
> > struct counter_device *const counter =
> > @@ -31,6 +37,9 @@ static void counter_device_release(struct device *dev)
> >
> > counter_chrdev_remove(counter);
> > ida_free(&counter_ida, dev->id);
> > +
> > + if (!counter->legacy_device)
> > + kfree(container_of(counter, struct counter_device_allochelper, counter));
> > }
> >
> > static struct device_type counter_device_type = {
> > @@ -53,7 +62,14 @@ static dev_t counter_devt;
> > */
> > void *counter_priv(const struct counter_device *const counter)
> > {
> > - return counter->priv;
> > + if (counter->legacy_device) {
> > + return counter->priv;
> > + } else {
> > + struct counter_device_allochelper *ch =
> > + container_of(counter, struct counter_device_allochelper, counter);
> > +
> > + return &ch->privdata;
> > + }
> > }
> > EXPORT_SYMBOL_GPL(counter_priv);
> >
> > @@ -74,6 +90,8 @@ int counter_register(struct counter_device *const counter)
> > int id;
> > int err;
> >
> > + counter->legacy_device = true;
> > +
> > /* Acquire unique ID */
> > id = ida_alloc(&counter_ida, GFP_KERNEL);
> > if (id < 0)
> > @@ -114,6 +132,100 @@ int counter_register(struct counter_device *const counter)
> > }
> > EXPORT_SYMBOL_GPL(counter_register);
> >
> > +/**
> > + * 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 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;
> > + }
> > + dev->id = err;
> > +
> > + err = counter_chrdev_add(counter);
>
> Should think about renaming counter_chrdev_add() given it
> does init and allocation stuff but no adding.
This is orthogonal to this series though. So I won't address this in the
context of the lifetime fixes here.
> Personal inclination here would be to keep the elements in here
> in the same order as in counter_register() where it doesn't matter
> in the interests of slightly easier comparison of the code.
I reordered a bit because counter_register fails to undo ida_alloc() in
the error path. However I might have missed that some initialisation has
to be done before calling counter_chrdev_add().
Will check in detail for v3.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
