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

[Jiri Pirko]

Wed, Oct 21, 2026 at 03:25:05AM +0200, jic23@xxxxxxxxxx wrote:
>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.

Agreed, easier to read. Will fix.


>
>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.

Will do.


>
>> +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");
>

Thanks!