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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 15 Oct 2026 16:50:43 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> From: Jiri Pirko <jiri@nvidia.com>
> 
> 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@nvidia.com>
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.

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

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

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

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

> +	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;
> +}