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
Jason
