On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
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?
When we initially designed this, Dan suggested to tie the device and
workqueue enabling to the Linux device model so that the
enabling/disabling can be done via bind/unbind of the sub drivers. So
that's how we ended up with all the 'struct device' under idxd and one
for each configurable component of the hardware device.
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.
Yes I need to fix this. I forgot about the dma_dev.
+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.
These memory are not tied to 'struct device' and should be able to
managed by devm right?
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
I think I see what you are saying. Let me have another go at it.
But even better would be to get rid of the struct device embedded in
the sub objects
Jason
