Re: [PATCH 3/4] cxl/mem: Do not rely on device_add() side effects for dev_set_name() failures

[Jason Gunthorpe]

On Wed, Mar 24, 2021 at 02:02:01PM -0700, Dan Williams wrote:
> While device_add() will happen to catch dev_set_name() failures it is a
> broken pattern to follow given that the core may try to fall back to a
> different name.
> 
> Add explicit checking for dev_set_name() failures to be cleaned up by
> put_device(). Skip cdev_device_add() and proceed directly to
> put_device() if the name set failure.
> 
> Fixes: b39cb1052a5c ("cxl/mem: Register CXL memX devices")
> Reported-by: Jason Gunthorpe <j...@nvidia.com>
> Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
>  drivers/cxl/mem.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index e53d573ae4ab..d615f183520c 100644
> +++ b/drivers/cxl/mem.c
> @@ -1204,12 +1204,14 @@ static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
>       dev->bus = &cxl_bus_type;
>       dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>       dev->type = &cxl_memdev_type;
> -     dev_set_name(dev, "mem%d", cxlmd->id);
>  
>       cdev = &cxlmd->cdev;
>       cdev_init(cdev, &cxl_memdev_fops);
>  
> -     rc = cdev_device_add(cdev, dev);
> +     rc = dev_set_name(dev, "mem%d", cxlmd->id);
> +     if (rc == 0)
> +             rc = cdev_device_add(cdev, dev);

Success oriented flow please

This is much nicer if you split the allocation, then this flow is
clean and simple. cxl_alloc_memdev() is undone by cxl_memdev_release()

and I would reorder the code so they are above/below each other, it is
easy to check and understand when logically paired functions are on
the same screen.

static struct cxl_memdev *cxl_alloc_memdev(struct cxl_mem *cxlm)
{
        struct cxl_memdev *cxlmd;
        struct device *dev;

        cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL);
        if (!cxlmd)
                return PTR_ERR(-ENOMEM);

        init_completion(&cxlmd->ops_dead);
        cxlmd->cxlm = cxlm;

        rc = ida_alloc_range(&cxl_memdev_ida, 0, CXL_MEM_MAX_DEVS, GFP_KERNEL);
        if (rc < 0)
                goto err_free;
        cxlmd->id = rc;

        /*
         * @cxlm is deallocated when the driver unbinds so operations
         * that are using it need to hold a live reference.
         */
        rc = percpu_ref_init(&cxlmd->ops_active, cxlmdev_ops_active_release, 0,
                             GFP_KERNEL);
        if (rc)
                goto err_id;

        dev = &cxlmd->dev;
        dev->parent = &pdev->dev;
        dev->bus = &cxl_bus_type;
        dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
        dev->type = &cxl_memdev_type;
        device_initialize(dev);
        return cxlmd;

err_id:
        ida_free(&cxl_memdev_ida, cxlmd->id);
err_free:
        kfree(cxlmd);
        return PTR_ERR(rc);
}

static int cxl_mem_add_memdev(struct cxl_mem *cxlm)
{
        struct pci_dev *pdev = cxlm->pdev;
        struct cdev *cdev;
        int rc;

        cxlmd = cxl_alloc_memdev(cxlm);
        if (IS_ERR(cxlmd))
                return ERR_PTR(cxlmd)

        rc = dev_set_name(dev, "mem%d", cxlmd->id);
        if (rc)
                goto err;

        cdev = &cxlmd->cdev;
        cdev_init(cdev, &cxl_memdev_fops);

        // Must be last
        rc = cdev_device_add(cdev, dev);
        if (rc)
                goto err;

        return devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd);
err:
        put_device(&cxlmd->dev);
        return rc;
}