Re: [PATCH 1/8] fwctl: Add basic structure for a class subsystem with a cdev

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jun 04, 2024 at 06:05:55PM +0100, Jonathan Cameron wrote:

> Trick for this is often to define a small function that allocates both the
> ida and the device. With in that micro function handle the one error path
> or if you only have two things to do, you can use __free() for the allocation.

This style is already followed here, the _alloc_device() is the
function that does everything before starting reference counting (IMHO
it is the best pattern to use). If we move the ida allocation to that
function then the if inside release is not needed.

Like this:

diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c
index d25b5eb3aee73c..a26697326e6ced 100644
--- a/drivers/fwctl/main.c
+++ b/drivers/fwctl/main.c
@@ -267,8 +267,7 @@ static void fwctl_device_release(struct device *device)
 	struct fwctl_device *fwctl =
 		container_of(device, struct fwctl_device, dev);
 
-	if (fwctl->dev.devt)
-		ida_free(&fwctl_ida, fwctl->dev.devt - fwctl_dev);
+	ida_free(&fwctl_ida, fwctl->dev.devt - fwctl_dev);
 	mutex_destroy(&fwctl->uctx_list_lock);
 	kfree(fwctl);
 }
@@ -288,6 +287,7 @@ static struct fwctl_device *
 _alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
 {
 	struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL);
+	int devnum;
 
 	if (!fwctl)
 		return NULL;
@@ -296,6 +296,12 @@ _alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
 	init_rwsem(&fwctl->registration_lock);
 	mutex_init(&fwctl->uctx_list_lock);
 	INIT_LIST_HEAD(&fwctl->uctx_list);
+
+	devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL);
+	if (devnum < 0)
+		return NULL;
+	fwctl->dev.devt = fwctl_dev + devnum;
+
 	device_initialize(&fwctl->dev);
 	return_ptr(fwctl);
 }
@@ -307,16 +313,10 @@ struct fwctl_device *_fwctl_alloc_device(struct device *parent,
 {
 	struct fwctl_device *fwctl __free(fwctl) =
 		_alloc_device(parent, ops, size);
-	int devnum;
 
 	if (!fwctl)
 		return NULL;
 
-	devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL);
-	if (devnum < 0)
-		return NULL;
-	fwctl->dev.devt = fwctl_dev + devnum;
-
 	cdev_init(&fwctl->cdev, &fwctl_fops);
 	fwctl->cdev.owner = THIS_MODULE;
 
Jason