Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> Remove devm_* allocation of memory of 'struct device' objects.
> The devm_* lifetime is incompatible with device->release() lifetime.
> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> functions for each component in order to free the allocated memory at
> the appropriate time. Each component such as wq, engine, and group now
> needs to be allocated individually in order to setup the lifetime properly.

I really don't understand why idxd has so many struct device objects.

Typically I expect a simple driver to have exactly one, usually
provided by its subsystem.

What is the purpose?

And it is still messed up because it has:

struct idxd_device {
        enum idxd_type type;
        struct device conf_dev; <-- This is a kref

        struct dma_device dma_dev; <-- And this is also a kref
}

The first kref does kfree() and the second does
idxd_conf_device_release() which does nothing - this is obviously
wrong too.

> +static int idxd_allocate_wqs(struct idxd_device *idxd)
> +{
> +	struct device *dev = &idxd->pdev->dev;
> +	struct idxd_wq *wq;
> +	int i, rc;
> +
> +	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq *),
> +				 GFP_KERNEL);

Memory stored in the idxd_device should be freed by the release
function, not by devm.

And since the sub objects already have a pointer to the idxd_device,
I'd keep all the types the same but have the sub structs carry a kref
on the idxd_device, so their release function is just kref_put

Would be much less code

But even better would be to get rid of the struct device embedded in
the sub objects

Jason