On 3/12/2021 7:41 AM, Jason Gunthorpe wrote:
On Fri, Mar 05, 2021 at 09:36:02AM -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.
In the process also fix up issues from the fallout of the changes.
Reported-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
v6:
- Fix char dev initialization issues (Jason)
- Fix other 'struct device' initialization issues.
v5:
- Rebased against 5.12-rc dmaengine/fixes
v4:
- fix up the life time of cdev creation/destruction (Jason)
- Tested with KASAN and other memory allocation leak detections. (Jason)
v3:
- Remove devm_* for irq request and cleanup related bits (Jason)
v2:
- Remove all devm_* alloc for idxd_device (Jason)
- Add kref dep for dma_dev (Jason)
drivers/dma/idxd/cdev.c | 44 ++++----
drivers/dma/idxd/device.c | 20 ++-
drivers/dma/idxd/dma.c | 13 ++
drivers/dma/idxd/idxd.h | 43 +++++++
drivers/dma/idxd/init.c | 261 +++++++++++++++++++++++++++++++++------------
drivers/dma/idxd/irq.c | 6 +
drivers/dma/idxd/sysfs.c | 225 ++++++++++++++++++++-------------------
7 files changed, 393 insertions(+), 219 deletions(-)
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 0db9b82ed8cf..56143336e88b 100644
+++ b/drivers/dma/idxd/cdev.c
@@ -259,34 +259,29 @@ static int idxd_wq_cdev_dev_setup(struct idxd_wq *wq)
return -ENOMEM;
dev = idxd_cdev->dev;
+ device_initialize(dev);
dev->parent = &idxd->pdev->dev;
- dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd),
- idxd->id, wq->id);
dev->bus = idxd_get_bus_type(idxd);
+ dev->type = &idxd_cdev_device_type;
+ rc = dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd),
+ idxd->id, wq->id);
+ if (rc < 0)
+ goto dev_set_err;
cdev_ctx = &ictx[wq->idxd->type];
minor = ida_simple_get(&cdev_ctx->minor_ida, 0, MINORMASK, GFP_KERNEL);
if (minor < 0) {
rc = minor;
- kfree(dev);
- goto ida_err;
+ goto dev_set_err;
}
dev->devt = MKDEV(MAJOR(cdev_ctx->devt), minor);
- dev->type = &idxd_cdev_device_type;
- rc = device_register(dev);
- if (rc < 0) {
- dev_err(&idxd->pdev->dev, "device register failed\n");
- goto dev_reg_err;
- }
idxd_cdev->minor = minor;
The error unwind after this is wrong:
int idxd_wq_add_cdev(struct idxd_wq *wq)
{
rc = idxd_wq_cdev_dev_setup(wq);
if (rc < 0)
return rc;
// At this point we have done device_initialize() only
rc = cdev_device_add(cdev, dev);
if (rc) {
idxd_wq_cdev_cleanup(wq, CDEV_FAILED);
static void idxd_wq_cdev_cleanup(struct idxd_wq *wq,
enum idxd_cdev_cleanup cdev_state)
{
if (cdev_state == CDEV_NORMAL) {
} else {
device_unregister(dev); // But nobody called device_register!
The 'enum idxd_cdev_cleanup' is really gross, you should avoid that.
This feels like an error that crept in from splitting dev_setup and
add_cdev wrongly
There should be two functions 'allocate' which brings things to the
point that 'put_device()' is the "undo"
And then "add" which does the eventual device add.
To get to that model here you want to move the ida_simple_remove into
the release function
And you need to split this patch up
Looks like I can split this out as an independent cleanup patch. I'll do
that and open code the setup and cleanup as well as they don't need to
have their own functions. I have the new patch and it looks much cleaner.
