A small collection of fixes mostly inspired by Jason's recognition of
dev_set_name() error handling mistakes on other driver review.
dev_set_name() can fail and although device_add() might catch it that's
not a reliable assumption. While fixing that I noticed that the unwind
handling for cdev_device_add() failures leaked the device name.
The sysfs_emit() fixup and unpublishing of device power management files
are just sanity cleanups.
---
Dan Williams (4):
cxl/mem: Use sysfs_emit() for attribute show routines
cxl/mem: Fix cdev_device_add() error handling
cxl/mem: Do not rely on device_add() side effects for dev_set_name() failures
cxl/mem: Disable cxl device power management
drivers/cxl/mem.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
base-commit: a38fd8748464831584a19438cbb3082b5a2dab15
There is no power management of cxl virtual devices, disable
device-power-management and runtime-power-management to prevent
userspace from growing expectations of those attributes appearing. They
can be added back in the future if needed.
Reviewed-by: Ben Widawsky <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/cxl/mem.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index d615f183520c..338923a6b2ef 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -1204,6 +1204,7 @@ 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;
+ device_set_pm_not_required(dev);
cdev = &cxlmd->cdev;
cdev_init(cdev, &cxl_memdev_fops);
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 <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
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
--- a/drivers/cxl/mem.c
+++ 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);
+
if (rc) {
percpu_ref_kill(&cxlmd->ops_active);
put_device(dev);
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 <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> 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;
}