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

[Jason Gunthorpe <jgg@nvidia.com>]

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@nvidia.com>
> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 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