Re: [PATCH RFC 10/20] ctlv: Prepare event queue support

[Jiri Pirko]

Wed, Oct 21, 2026 at 03:12:08AM +0200, jic23@xxxxxxxxxx wrote:
>On Thu, 15 Oct 2026 16:50:43 +0200
>Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
>
>> From: Jiri Pirko <jiri@xxxxxxxxxx>
>> 
>> Add per-open subscriptions, bounded event queues and event creation for
>> CTLV families. Add the generated root family kernel bindings now that their
>> message and event helpers are available.
>> 
>> The event and generated root objects can be compile-checked here.
>> CONFIG_CTLV remains unwired until device dispatch completes the module.
>> 
>> Assisted-by: Cursor: GPT-5.6 Sol
>> Assisted-by: Cursor: Claude Opus 5.5
>> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxx>
>Nothing fundamental - just a few passing comments.
>
>I'm running low on time, so will take a closer look at
>this in a future revision.
>
>> diff --git a/drivers/ctlv/event.c b/drivers/ctlv/event.c
>> new file mode 100644
>> index 000000000000..0f68d33e031c
>> --- /dev/null
>> +++ b/drivers/ctlv/event.c
>> @@ -0,0 +1,713 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES
>> + *
>> + * Per-open event queues and loss reporting.
>> + */
>> +
>> +#include <linux/bitmap.h>
>> +#include <linux/build_bug.h>
>
>cleanup.h given you are making quite a bit of use
>of guard() in here.

Okay.


>
>> +#include <linux/ctlv.h>
>> +#include <linux/ctlv_gen.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/list.h>
>> +#include <linux/minmax.h>
>> +#include <linux/overflow.h>
>> +#include <linux/poll.h>
>> +#include <linux/sizes.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/string.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/wait.h>
>> +
>> +#include "builder.h"
>> +#include "device.h"
>> +#include "event.h"
>> +#include "internal.h"
>> +#include "message.h"
>> +#include "schema.h"
>
>> +
>> +struct ctlv_op_ctx *ctlv_event_create(struct ctlv_device *ctlvdev, u64 op_id,
>> +				      struct ctlv_cursor **reply, gfp_t gfp)
>> +{
>> +	const struct ctlv_op_entry *op;
>> +	struct ctlv_op_ctx *ctx;
>> +	void *out;
>> +	u32 room;
>> +	int ret;
>> +
>> +	ctx = kzalloc_obj(*ctx, gfp);
>> +	if (!ctx)
>> +		return ERR_PTR(-ENOMEM);
>> +	scoped_guard(spinlock, &ctlvdev->events_lock) {
>
>Generally mixing anything cleanup.h with gotos is
>a bad idea. Tends to get Linus saying your code needs
>restructuring even if it isn't an actual bug.

Okay. Will rework.


>
>> +		const struct ctlv_family *family;
>> +
>> +		if (!ctlvdev->events_live) {
>> +			ret = -ENODEV;
>> +			goto err_free_ctx;
>> +		}
>> +		if (WARN_ON_ONCE(ctlvdev->active_event_contexts == U32_MAX)) {
>> +			ret = -EOVERFLOW;
>> +			goto err_free_ctx;
>> +		}
>> +		/* Admission pins the op until emit or abort. */
>> +		family = READ_ONCE(ctlvdev->family);
>> +		if (!family) {
>> +			ret = -ENODEV;
>> +			goto err_free_ctx;
>> +		}
>> +		get_device(&ctlvdev->dev);
>> +		ctlvdev->active_event_contexts++;
>> +		ctx->ctlvdev = ctlvdev;
>> +		ctx->family = family;
>> +	}
>> +
>> +	op = ctlv_op_find(ctlvdev, op_id);
>> +	if (WARN_ON_ONCE(!op) || WARN_ON_ONCE(op->schema->type != CTLV_OP_TYPE_EVENT)) {
>> +		ret = -EINVAL;
>> +		goto err_put_ctx;
>> +	}
>> +	room = ctlv_event_len(op) - sizeof(struct ctlv_event_hdr);
>> +	out = kmalloc(room, gfp);
>> +	if (!out) {
>> +		ret = -ENOMEM;
>> +		goto err_put_ctx;
>> +	}
>> +
>> +	ctx->gfp = gfp;
>> +	ctx->op = op;
>> +	ctlv_message_output_init(&ctx->output, ctx, out, room, &op->schema->reply);
>> +	ctlv_message_root(&ctx->output_root, &ctx->output);
>> +	ctx->deepest = &ctx->output_root;
>> +	*reply = &ctx->output_root;
>> +	return ctx;
>> +
>> +err_put_ctx:
>> +	ctlv_event_free(ctx);
>> +	return ERR_PTR(ret);
>> +err_free_ctx:
>> +	kfree(ctx);
>> +	return ERR_PTR(ret);
>
>Two different error paths also tend to indicate code needs
>a rethink and maybe splitting into a couple of functions for the
>different stages.

Okay, will do.


>
>> +}
>> +EXPORT_SYMBOL_NS_GPL(ctlv_event_create, "CTLV");
>
>
>
>> +
>> +ssize_t ctlv_event_file_read(struct ctlv_file *cfile, char __user *buf, size_t len, bool blocking)
>> +{
>> +	struct ctlv_device *ctlvdev = cfile->ctlvdev;
>> +	struct ctlv_event_record *rec;
>> +	ssize_t ret;
>> +
>> +	/* dup() and SCM_RIGHTS share one serialized queue. */
>> +	if (mutex_lock_interruptible(&cfile->reading))
>> +		return -ERESTARTSYS;
>
>	ACQUIRE() magic should work here and let you do direct returns on error.

Okay.


>
>> +
>> +	while (!(rec = ctlv_event_queue_take(cfile))) {
>> +		/* Drained, and the device is gone. */
>> +		if (READ_ONCE(ctlvdev->dead_announced)) {
>> +			ret = -ENODEV;
>> +			goto out_unlock;
>> +		}
>> +		if (!blocking) {
>> +			ret = -EAGAIN;
>> +			goto out_unlock;
>> +		}
>> +		ret = wait_event_interruptible(cfile->readers, ctlv_event_waiting(cfile));
>> +		if (ret) {
>> +			ret = -ERESTARTSYS;
>> +			goto out_unlock;
>> +		}
>> +	}
>> +
>> +	if (rec->len > len) {
>> +		ctlv_event_queue_untake(cfile, rec);
>> +		ret = -EMSGSIZE;
>> +		goto out_unlock;
>> +	}
>> +	if (copy_to_user(buf, &rec->hdr, rec->len)) {
>> +		ctlv_event_queue_untake(cfile, rec);
>> +		ret = -EFAULT;
>> +		goto out_unlock;
>> +	}
>> +	ret = rec->len;
>> +	ctlv_event_queue_consumed(cfile, rec);
>> +
>> +out_unlock:
>> +	mutex_unlock(&cfile->reading);
>> +	return ret;
>> +}
>
>> +static int ctlv_event_subscriptions_set(struct ctlv_op_ctx *ctx, const struct ctlv_cursor *request,
>> +					struct ctlv_input_ref ref, bool subscribe)
>> +{
>> +	struct ctlv_device *ctlvdev = ctx->ctlvdev;
>> +	struct ctlv_cursor attr;
>> +
>> +	for (ctlv_input_attr(&attr, request, ref); ctlv_attr_ok(&attr); ctlv_attr_next(&attr)) {
>> +		u64 op_id = ctlv_attr_u64(&attr);
>> +
>> +		if (!ctlv_event_subscription_find(ctlvdev, op_id))
>> +			return ctlv_op_errorf(ctx, -EOPNOTSUPP,
>> +					      "op %#llx is not an event this device emits",
>> +					      (unsigned long long)op_id);
>> +	}
>> +
>> +	guard(spinlock)
>> +		(&ctlvdev->events_lock);
>
>That looks odd. 
>	guard(spinlock)(&ctlvdev->events_lock);
>is more familiar form to me anyway.

True. Will fix.


>
>> +	for (ctlv_input_attr(&attr, request, ref); ctlv_attr_ok(&attr); ctlv_attr_next(&attr)) {
>> +		u64 op_id = ctlv_attr_u64(&attr);
>> +		const struct ctlv_op_entry *op = ctlv_event_subscription_find(ctlvdev, op_id);
>> +
>> +		if (subscribe)
>> +			set_bit(op->event_index, ctx->file->subscribed);
>> +		else
>> +			clear_bit(op->event_index, ctx->file->subscribed);
>
>		assign_bit()
>
>> +	}
>> +	return 0;
>> +}
>