On Tue, Jun 04, 2024 at 12:32:19PM +0300, Leon Romanovsky wrote:
> > +static struct fwctl_device *
> > +_alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
> > +{
> > + struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL);
> > +
> > + if (!fwctl)
> > + return NULL;
>
> <...>
>
> > +/* Drivers use the fwctl_alloc_device() wrapper */
> > +struct fwctl_device *_fwctl_alloc_device(struct device *parent,
> > + const struct fwctl_ops *ops,
> > + size_t size)
> > +{
> > + struct fwctl_device *fwctl __free(fwctl) =
> > + _alloc_device(parent, ops, size);
>
> I'm not a big fan of cleanup.h pattern as it hides important to me
> information about memory object lifetime and by "solving" one class of
> problems it creates another one.
I'm trying it here. One of the most common bugs I end up fixing is
error unwind and cleanup.h has successfully removed all of it. Let's
find out, others thought it was a good idea to add the infrastructure.
One thing that seems clear in my work here is that you should not use
cleanup.h if you don't have simple memory lifetime, like the above
case where the memory is freed if the function fails.
> You didn't check if fwctl is NULL before using it.
Oops, yes
> > + int devnum;
> > +
> > + devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL);
> > + if (devnum < 0)
> > + return NULL;
> > + fwctl->dev.devt = fwctl_dev + devnum;
> > +
> > + cdev_init(&fwctl->cdev, &fwctl_fops);
> > + fwctl->cdev.owner = THIS_MODULE;
> > +
> > + if (dev_set_name(&fwctl->dev, "fwctl%d", fwctl->dev.devt - fwctl_dev))
>
> Did you miss ida_free() here?
No, the put_device() does it in the release function. The __free
always calls fwctl_put()/put_device() on failure, and within all
functions except _alloc_device() the put_device() is the correct way
to free this memory.
Thanks,
Jason
