Re: [PATCH RFC 12/20] ctlv: Add device and operation dispatch

[Jonathan Cameron]

On Thu, 15 Oct 2026 16:50:45 +0200
Jiri Pirko <jiri@xxxxxxxxxxx> wrote:

> From: Jiri Pirko <jiri@xxxxxxxxxx>
> 
> Register CTLV devices, resolve family chains and dispatch operations
> through validated messages. Wire CONFIG_CTLV and the generated root family
> after all module objects are present.
> 
> Assisted-by: Cursor: GPT-5.6 Sol
> Assisted-by: Cursor: Claude Opus 5.5
> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxx>
> ---
>  drivers/Kconfig             |    2 +
>  drivers/Makefile            |    1 +
>  drivers/ctlv/Makefile       |    4 +-
>  drivers/ctlv/core.c         | 1517 +++++++++++++++++++++++++++++++++++
Normally I'd push back hard on too big to read, but it is an rfc
and other patches in the series are much large.

I'm out of time for today and not sure when I'll get back to this,
so just a few quick comments inline on the bit I did look at.

>  drivers/ctlv/device.h       |   17 +
>  include/linux/ctlv.h        |   55 ++
>  include/linux/ctlv_family.h |   22 +
>  7 files changed, 1616 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/ctlv/core.c

> diff --git a/drivers/ctlv/core.c b/drivers/ctlv/core.c
> new file mode 100644
> index 000000000000..aeab29fe2fdd
> --- /dev/null
> +++ b/drivers/ctlv/core.c

...

> +static int ctlv_file_open(struct inode *inode, struct file *filp)
> +{
> +	struct ctlv_device *ctlvdev = container_of(inode->i_cdev, struct ctlv_device, cdev);
> +	struct ctlv_file *cfile;
> +	int ret;
> +
> +	/*
> +	 * A failed create can withdraw the cdev while an open is waiting for
> +	 * the lifecycle lock, so admission itself first pins core storage.
> +	 */
> +	get_device(&ctlvdev->dev);
> +	ctlv_file_open_pinned(ctlvdev);
> +	scoped_guard(rwsem_read, &ctlvdev->registration_lock) {
> +		if (!ctlv_file_open_allowed(ctlvdev)) {
> +			ret = -ENODEV;
> +			break;
Personally I still dislike the hidden for loop in scoped_guard()
and using break with that enough I'd do a 
	do {
		guard(rwsem_read)(&ctlvdev->registration_lock);

	
	} while (0);

Just so it is obvious what those breaks are from.

Maybe I just need to get this one into my code review head...
> +		}
> +		if (!(filp->f_mode & FMODE_READ)) {
> +			ret = -EACCES;
> +			break;
> +		}
> +
> +		cfile = kzalloc_obj(*cfile, GFP_KERNEL_ACCOUNT);
> +		if (!cfile) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +		cfile->ctlvdev = ctlvdev;
> +		/*
> +		 * Commits this description to the device. An open that failed
> +		 * after this would have to take it back.
> +		 */
> +		ret = ctlv_event_file_init(cfile);
> +		if (ret) {
> +			kfree(cfile);
> +			break;
> +		}
> +		/* This description keeps the reference taken on entry. */
> +		filp->private_data = cfile;
> +		return 0;
> +	}
> +
> +	put_device(&ctlvdev->dev);
> +	return ret;
> +}

> +struct ctlv_device *ctlv_create(struct device *parent_dev, const struct ctlv_family *family,
> +				const void *init_info)
> +{
> +	struct ctlv_device *ctlvdev;
> +	int devnum, ret;
> +
> +	if (!parent_dev || !family)
> +		return ERR_PTR(-EINVAL);
> +
> +	ctlvdev = kzalloc_obj(*ctlvdev, GFP_KERNEL);
> +	if (!ctlvdev)
> +		return ERR_PTR(-ENOMEM);
> +	ctlvdev->family = family;
> +	init_rwsem(&ctlvdev->registration_lock);
> +	spin_lock_init(&ctlvdev->transition_lock);
> +	ctlv_event_device_init(ctlvdev);
> +
> +	/* Before anything reads the schema through this device. */
> +	ret = ctlv_op_resolution_init(ctlvdev, family);
> +	if (ret)
> +		goto err_free;
> +	/* What this family's events need of every queue opened on it. */
> +	ret = ctlv_event_sizes(ctlvdev);
> +	if (ret)
> +		goto err_free;
> +	/* Before a minor is spent on a device that could not be announced. */
> +	ret = ctlv_device_uevent_check(family);
> +	if (ret)
> +		goto err_free;
> +	if (ctlvdev->dynamic_ops_count) {
> +		ctlvdev->disabled = bitmap_zalloc(ctlvdev->dynamic_ops_count, GFP_KERNEL);
> +		if (!ctlvdev->disabled) {
> +			ret = -ENOMEM;
> +			goto err_free;
> +		}
> +	}
> +	devnum = ida_alloc_max(&ctlv_device_ids, CTLV_MAX_DEVICES - 1, GFP_KERNEL);
> +	if (devnum < 0) {
> +		ret = devnum;
> +		goto err_free;
> +	}
> +
> +	ctlvdev->dev.devt = ctlv_device_number + devnum;
> +	ctlvdev->dev.class = &ctlv_device_class;
> +	ctlvdev->dev.parent = parent_dev;
> +	device_initialize(&ctlvdev->dev);
> +	/*
> +	 * The core owns the file operations, so an open holds the core module
> +	 * and not the family's, which may unload while readers remain.
> +	 */
> +	cdev_init(&ctlvdev->cdev, &ctlv_file_operations);
> +	ctlvdev->cdev.owner = THIS_MODULE;
> +
> +	ret = dev_set_name(&ctlvdev->dev, "ctlv%d", devnum);
> +	if (ret)
> +		goto err_put;
> +	ret = ctlv_family_claim_add(family->schema);
> +	if (ret)
> +		goto err_put;
> +	ret = ctlv_family_privs_pre_init(ctlvdev, init_info);
> +	if (ret)
> +		goto err_put;
> +	ret = ctlv_family_privs_init(ctlvdev);
> +	if (ret)
> +		goto err_put;
> +	ctlv_op_publish_all(ctlvdev);
> +	ctlv_event_contexts_start(ctlvdev);
> +	ret = ctlv_family_privs_post_init(ctlvdev);
> +	if (ret)
> +		goto err_fini;
> +	scoped_guard(rwsem_write, &ctlvdev->registration_lock) {
> +		ret = ctlv_device_add(ctlvdev);
> +		if (!ret)
> +			ctlvdev->registered = true;
> +	}
> +	if (ret)
> +		goto err_fini;
> +	return ctlvdev;
> +
> +err_fini:
> +	ctlv_device_fini(ctlvdev);
> +err_put:
> +	put_device(&ctlvdev->dev);
> +	return ERR_PTR(ret);

Jason did some nice handling in the fwctl stuff for
device alloc and hand over after device_initialize()
(I've copied it into a few places recently :)

I'd like to see something similar here as it made
it easier to follow lifetimes etc.

> +err_free:
> +	bitmap_free(ctlvdev->disabled);
> +	ctlv_family_privs_free(ctlvdev);
> +	kfree(ctlvdev->ops);
> +	kfree(ctlvdev);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_NS_GPL(ctlv_create, "CTLV");