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
