> From b28a15e4805941e1545f45068688823b9ea0f4ff Mon Sep 17 00:00:00 2001 > From: Jiri Pirko > Date: Thu, 15 Oct 2026 16:50:43 +0200 > Subject: [PATCH 10/20] ctlv: Prepare event queue support > > Add per-open subscriptions, bounded event queues and event creation for > CTLV families. Add the generated root family kernel bindings now that their The diffstat shows no generated root-family files. `ctlv_gen.c` and `include/linux/ctlv_gen.h` were added in the root family patch (217219cc), and `ctlv_gen`.o is not added to the Makefile by this patch (it only appears in the final dispatch patch). So "Add the generated root family kernel bindings" and "the event and generated root objects can be compile-checked here" are both false: only event.o is built-able here, and nothing builds `ctlv_gen.c` until the last patch. Related bisectability wart: the already-merged `include/linux/ctlv_gen.h` uses `CTLV_EVENT_CREATE()`, which is only defined by the `ctlv_family.h` hunk of this patch, so that header is not self-contained in patches 5-9. Either move the macro into the patch that introduces `ctlv_gen.h` or fix the commit message. > 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 > --- > drivers/ctlv/Makefile | 2 +- > drivers/ctlv/device.h | 38 ++ > drivers/ctlv/event.c | 713 ++++++++++++++++++++++++++++++++++++ > drivers/ctlv/event.h | 69 ++++ > drivers/ctlv/internal.h | 7 + > include/linux/ctlv.h | 27 ++ > include/linux/ctlv_family.h | 11 + > 7 files changed, 866 insertions(+), 1 deletion(-) > create mode 100644 drivers/ctlv/device.h > create mode 100644 drivers/ctlv/event.c > create mode 100644 drivers/ctlv/event.h > > diff --git a/drivers/ctlv/Makefile b/drivers/ctlv/Makefile > index 4455062f83d7..e631ccd68994 100644 > --- a/drivers/ctlv/Makefile > +++ b/drivers/ctlv/Makefile > @@ -1,4 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_CTLV) += ctlv.o > > -ctlv-y += blob.o blob_desc.o builder.o message.o schema.o > +ctlv-y += blob.o blob_desc.o builder.o event.o message.o schema.o Consistent with the above: `ctlv_gen`.o is missing, so the "compile-checked" claim cannot hold for the generated root bindings. > diff --git a/drivers/ctlv/device.h b/drivers/ctlv/device.h > new file mode 100644 > index 000000000000..916a346c30ed > --- /dev/null > +++ b/drivers/ctlv/device.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef _CTLV_DEVICE_H > +#define _CTLV_DEVICE_H > + > +#include > +#include > +#include > +#include > +#include > + > +struct ctlv_event_record; > +struct ctlv_family; > +struct ctlv_op_entry; > + > +struct ctlv_device { > + struct device dev; This creates `struct ctlv_device` with only the subset of fields this patch needs. Patch 11 (`introspection.c`) dereferences `ctlvdev->dynamic_ops_count`, `->transition_lock`, `->state_generation` and ->disabled, which only appear in the dispatch patch. Patch 11 therefore cannot be compile-checked as its commit message claims (see there). Fields should be added in the patch that first uses them, and `device.h` should be created by whichever patch actually needs it. > + > + const struct ctlv_family *family; > + > + struct ctlv_op_entry *ops; > + u32 ops_count; > + > + /* Everything about the queues, and nothing family code runs under. */ > + spinlock_t events_lock; > + struct list_head files; > + bool dead_announced; > + wait_queue_head_t event_context_wait; > + u32 active_event_contexts; > + bool events_live; > + u32 events_count; > + const struct ctlv_op_entry *overflow_op; `overflow_op`/`removed_op` are pointers into `ctlvdev->ops[]`. They are set by `ctlv_event_sizes()` at create time (before `ctlv_op_publish_all()`), and `ctlv_op_publish_all()` later compacts that very array in place by dropping absent entries. These pointers are correct only because no absent (optional) op happens to precede the root events in the sorted array. That invariant is nowhere stated or asserted; storing the indices (or resolving after publication) would remove the latent dangling/mis-pointing hazard. Same hazard for `ctlv_event_create()`'s `ctlv_op_find()` result, but that one is gated by `events_live`. > + const struct ctlv_op_entry *removed_op; > + u32 queue_min; > + u32 queue_reserved; > + u32 queue_default; > +}; > + > +#endif /* _CTLV_DEVICE_H */ > 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 Missing includes for what is used: `` (guard/`scoped_guard`), ``, ``/`` (`WARN_ON_ONCE`, lockdep). It works via transitive includes only. Conversely ``, `` are fine, but `` is the only reason `SZ_256K` builds. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "builder.h" > +#include "device.h" > +#include "event.h" > +#include "internal.h" > +#include "message.h" > +#include "schema.h" > + > +/* Header and root form one naturally aligned copy range. */ > +static_assert(offsetof(struct ctlv_event_record, root) == > + offsetof(struct ctlv_event_record, hdr) + sizeof(struct ctlv_event_hdr)); > +static_assert(sizeof(struct ctlv_event_hdr) % CTLV_ALIGNTO == 0); > + > +static u32 ctlv_event_len(const struct ctlv_op_entry *op) One-line wrapper around `op->sizes`.`event_record_max_len` used in four places; it adds an indirection and no meaning. Also the name is misleading: the value includes the event header (see room = `ctlv_event_len(op)` - `sizeof(hdr)` in `ctlv_event_create()`), which is not obvious from either the name or the generated field name `event_record_max_len`. > +{ > + return op->sizes.event_record_max_len; > +} > + > +int ctlv_event_sizes(struct ctlv_device *ctlvdev) > +{ > + u32 largest = 0; > + u32 index; > + > + for (index = 0; index < ctlvdev->ops_count; index++) { > + const struct ctlv_op_entry *op = &ctlvdev->ops[index]; > + > + if (op->schema->type != CTLV_OP_TYPE_EVENT) > + continue; > + largest = max(largest, ctlv_event_len(op)); > + if (op->schema->op_id == CTLV_OP_EVENT_OVERFLOW) > + ctlvdev->overflow_op = op; > + else if (op->schema->op_id == CTLV_OP_DEVICE_REMOVED) > + ctlvdev->removed_op = op; > + } > + /* Every chain inherits the root, so every device has both of them. */ > + if (WARN_ON_ONCE(!ctlvdev->overflow_op || !ctlvdev->removed_op)) If the invariant holds for every chain, this WARN is a dead branch. If it can happen, a family that is merely malformed (a userspace-visible registration failure) should not splat. Pick one. > + return -EINVAL; > + > + ctlvdev->queue_reserved = > + ctlv_event_len(ctlvdev->overflow_op) + ctlv_event_len(ctlvdev->removed_op); > + ctlvdev->queue_min = largest + ctlvdev->queue_reserved; > + if (ctlvdev->queue_min > CTLV_MAX_EVENT_QUEUE_LEN) > + return -EMSGSIZE; > + ctlvdev->queue_default = > + clamp(max(SZ_256K, 4 * largest), ctlvdev->queue_min, CTLV_MAX_EVENT_QUEUE_LEN); > + return 0; > +} > + > +void ctlv_event_device_init(struct ctlv_device *ctlvdev) > +{ > + spin_lock_init(&ctlvdev->events_lock); > + INIT_LIST_HEAD(&ctlvdev->files); > + init_waitqueue_head(&ctlvdev->event_context_wait); > +} > + > +void ctlv_event_contexts_start(struct ctlv_device *ctlvdev) Style: the series spells every guard as guard(spinlock) (&`ctlvdev->events_lock`); (clang-format artifact, ~12 occurrences in this patch and more in 11). The kernel spelling is guard(spinlock)(&lock); on one line. checkpatch/`--strict` will not like it. > +{ > + guard(spinlock) > + (&ctlvdev->events_lock); > + if (WARN_ON_ONCE(ctlvdev->events_live || ctlvdev->active_event_contexts)) > + return; > + ctlvdev->events_live = true; > +} > + > +void ctlv_event_contexts_stop(struct ctlv_device *ctlvdev) > +{ > + guard(spinlock) > + (&ctlvdev->events_lock); > + ctlvdev->events_live = false; > +} > + > +static bool ctlv_event_contexts_idle(struct ctlv_device *ctlvdev) > +{ > + guard(spinlock) > + (&ctlvdev->events_lock); > + return !ctlvdev->active_event_contexts; > +} > + > +void ctlv_event_contexts_wait(struct ctlv_device *ctlvdev) > +{ > + wait_event(ctlvdev->event_context_wait, ctlv_event_contexts_idle(ctlvdev)); > +} > + > +/* @room bounds storage from the header through the root. */ > +static struct ctlv_event_record *ctlv_event_record_alloc(u32 room, gfp_t gfp) > +{ > + struct ctlv_event_record *rec; > + > + rec = kmalloc(struct_size(rec, root, room - sizeof(rec->hdr)), gfp); > + if (!rec) > + return NULL; > + INIT_LIST_HEAD(&rec->node); > + rec->len = 0; > + rec->room = room; > + rec->reserved = false; > + return rec; > +} > + > +/* Core events use root schemas and may outlive the device family. */ > +static void ctlv_event_record_build(struct ctlv_op_ctx *ctx, struct ctlv_event_record *rec, > + const struct ctlv_op_entry *op) > +{ > + ctx->family = &ctlv_family; > + ctx->op = op; > + ctlv_message_output_init(&ctx->output, ctx, rec->root, rec->room - sizeof(rec->hdr), > + &op->schema->reply); > + ctlv_message_root(&ctx->output_root, &ctx->output); > + ctx->deepest = &ctx->output_root; > +} > + > +/* The padding is written rather than left as the allocation found it. */ > +static void ctlv_event_record_done(struct ctlv_op_ctx *ctx, struct ctlv_event_record *rec) > +{ > + u32 root_len = ctx->output.root->len; > + > + rec->len = sizeof(rec->hdr) + CTLV_ALIGN(root_len); > + rec->hdr.length = rec->len; > + memset(rec->root + root_len, 0, rec->len - sizeof(rec->hdr) - root_len); > +} > + > +static struct ctlv_event_record *ctlv_event_record_reserve(struct ctlv_device *ctlvdev, The @ctlvdev parameter is never used. The function also sets `rec->reserved` and the identity-based checks elsewhere (rec == `cfile->overflow`) mean the reserved flag is redundant with those checks in take/untake/consumed, or vice versa; keep one mechanism. > + const struct ctlv_op_entry *op) > +{ > + struct ctlv_event_record *rec; > + > + rec = ctlv_event_record_alloc(ctlv_event_len(op), GFP_KERNEL_ACCOUNT); > + if (!rec) > + return NULL; > + rec->hdr.flags = 0; > + rec->hdr.op_id = op->schema->op_id; > + rec->reserved = true; > + return rec; > +} > + > +int ctlv_event_file_init(struct ctlv_file *cfile) > +{ > + struct ctlv_device *ctlvdev = cfile->ctlvdev; > + struct ctlv_op_ctx ctx = { .ctlvdev = ctlvdev }; > + > + INIT_LIST_HEAD(&cfile->node); > + INIT_LIST_HEAD(&cfile->records); > + init_waitqueue_head(&cfile->readers); > + mutex_init(&cfile->reading); > + cfile->queue_len = ctlvdev->queue_default; > + cfile->queue_used = ctlvdev->queue_reserved; > + > + cfile->subscribed = bitmap_zalloc(ctlvdev->events_count, GFP_KERNEL_ACCOUNT); `struct ctlv_file` is assumed zero-initialized by the caller (overflowing, `reading_overflow`, sequence, dropped*, ...). Nothing in this patch or its header documents that; `ctlv_event_file_init()` sets some fields explicitly and relies on kzalloc for the rest. Please document, or initialize explicitly. > + cfile->overflow = ctlv_event_record_reserve(ctlvdev, ctlvdev->overflow_op); > + cfile->removed = ctlv_event_record_reserve(ctlvdev, ctlvdev->removed_op); > + if (!cfile->subscribed || !cfile->overflow || !cfile->removed) { > + ctlv_event_file_exit(cfile); > + return -ENOMEM; > + } > + ctlv_event_record_build(&ctx, cfile->removed, ctlvdev->removed_op); > + ctlv_event_record_done(&ctx, cfile->removed); > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + list_add_tail(&cfile->node, &ctlvdev->files); > + return 0; > +} > + > +void ctlv_event_file_exit(struct ctlv_file *cfile) > +{ > + struct ctlv_event_record *rec, *next; > + > + scoped_guard(spinlock, &cfile->ctlvdev->events_lock) > + list_del_init(&cfile->node); > + > + /* Unreachable records are ours; reserved records free by identity. */ > + list_for_each_entry_safe(rec, next, &cfile->records, node) > + if (!rec->reserved) > + kfree(rec); > + kfree(cfile->overflow); > + kfree(cfile->removed); > + bitmap_free(cfile->subscribed); > + mutex_destroy(&cfile->reading); > +} > + > +static void ctlv_event_queue_add(struct ctlv_file *cfile, struct ctlv_event_record *rec) > +{ > + list_add_tail(&rec->node, &cfile->records); > + wake_up_interruptible(&cfile->readers); > +} > + > +static void ctlv_event_queue_link(struct ctlv_file *cfile, struct ctlv_event_record *rec) > +{ > + rec->hdr.sequence = ++cfile->sequence; > + ctlv_event_queue_add(cfile, rec); > +} > + > +/* One reserved marker aggregates each contiguous drop epoch. */ > +static void ctlv_event_queue_dropped(struct ctlv_file *cfile, u64 sequence) > +{ > + if (!cfile->overflowing) { > + cfile->overflowing = true; > + cfile->first_dropped = sequence; > + cfile->dropped = 0; > + if (!cfile->reading_overflow) > + ctlv_event_queue_add(cfile, cfile->overflow); > + } > + cfile->last_dropped = sequence; > + cfile->dropped++; > +} > + > +/* Delivered and dropped subscribed events both consume a sequence number. */ > +static void ctlv_event_queue_event(struct ctlv_file *cfile, const struct ctlv_op_entry *op, > + struct ctlv_event_record *rec) > +{ > + u64 sequence; > + > + lockdep_assert_held(&cfile->ctlvdev->events_lock); > + if (!test_bit(op->event_index, cfile->subscribed)) > + return; > + sequence = ++cfile->sequence; > + if (cfile->overflowing || !rec || cfile->queue_used + rec->len > cfile->queue_len) { > + ctlv_event_queue_dropped(cfile, sequence); > + return; > + } > + cfile->queue_used += rec->len; > + rec->hdr.sequence = sequence; > + list_move_tail(&rec->node, &cfile->records); > + wake_up_interruptible(&cfile->readers); > +} > + > +static void ctlv_event_free(struct ctlv_op_ctx *ctx); > + > +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) { > + 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) { Is this reachable? `ctlv_device_fini()` clears `events_live` (`ctlv_event_contexts_stop`) before `ctlv_device_unregister` does `WRITE_ONCE(ctlvdev->family, NULL)`, and `events_live` is checked a few lines above under the same lock. If that ordering makes this impossible it is a dead check; if it is not guaranteed, the `WRITE_ONCE` side does not take `events_lock` so the `READ_ONCE` here does not synchronize anything. Also the comment "Admission pins the op" is misleading: what is pinned is the device (`get_device`) and the context count, not any op or the family module. > + 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); The scratch buffer is plain `kmalloc()` (uninitialized) and later memcpy'd as `root->len` bytes into the per-subscriber records. That is fine only as long as every byte of `root->len` is written by the builder (attr padding inside nested and string attrs included). Because these bytes reach userspace, please state/verify the builder zeroes attr padding, or use kzalloc here. > + 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); > +} > +EXPORT_SYMBOL_NS_GPL(ctlv_event_create, "CTLV"); > + > +static void ctlv_event_free(struct ctlv_op_ctx *ctx) > +{ > + struct ctlv_device *ctlvdev = ctx->ctlvdev; > + bool wake; > + > + kfree(ctx->output.root); > + kfree(ctx); > + > + scoped_guard(spinlock, &ctlvdev->events_lock) { > + if (WARN_ON_ONCE(!ctlvdev->active_event_contexts)) > + wake = true; > + else > + wake = !--ctlvdev->active_event_contexts; > + } > + if (wake) > + wake_up_all(&ctlvdev->event_context_wait); > + put_device(&ctlvdev->dev); > +} > + > +/* Reject an incomplete or schema-invalid family event before queueing. */ > +static int ctlv_event_finish(struct ctlv_op_ctx *ctx) > +{ > + struct ctlv_error refused = {}; > + > + /* Whatever poisoned it has warned about it already. */ > + if (ctx->output.poison) > + return ctx->output.poison; > + if (WARN_ON_ONCE(ctx->deepest != &ctx->output_root)) { > + /* A cursor of a frame that has returned is not followed. */ > + ctx->deepest = &ctx->output_root; > + return -EINVAL; > + } > + if (WARN_ON_ONCE(ctlv_schema_validate(&ctx->output, &refused))) 'refused' is filled and then discarded: the caller gets only `-EINVAL` and a splat without the reason. Print refused.message (or drop the struct by letting the validator take `NULL`). Also a `WARN_ON_ONCE` on a family-supplied value means a single buggy driver event is a reportable kernel warning; `pr_warn_once` would be more appropriate if `panic_on_warn` is a concern. > + return -EINVAL; > + return 0; > +} > + > +/* Allocate subscriber records before taking queue spinlocks. */ > +static void ctlv_event_copies(struct ctlv_op_ctx *ctx, u32 len, struct list_head *spare, u32 wanted) Three changes requested in `ctlv_event_copies()` and `ctlv_event_emit()`. 1. Allocate the per-subscriber records with `__GFP_ACCOUNT`, as the reserved records above use `GFP_KERNEL_ACCOUNT`; `ctx->gfp` alone does not charge them. 2. Log a partial allocation failure: `ctlv_event_copies()` returns silently when an allocation fails, and the subscribers left without a record are then counted as per-file overflow. Add a `pr_warn_ratelimited()`. 3. Add a comment that `wanted` is counted under one hold of `events_lock` and consumed under another, so a subscriber added in between gets no record and is counted as lost for an event that could have been delivered. > +{ > + const struct ctlv_attr *root = ctx->output.root; > + struct ctlv_event_record *rec; > + > + while (wanted--) { > + rec = ctlv_event_record_alloc(len, ctx->gfp); > + if (!rec) > + return; > + rec->len = len; > + rec->hdr.length = len; > + rec->hdr.flags = 0; > + rec->hdr.op_id = ctx->op->schema->op_id; > + /* Padded, since what is copied out is the whole record. */ > + memcpy_and_pad(rec->root, len - sizeof(rec->hdr), root, root->len, 0); > + list_add_tail(&rec->node, spare); > + } > +} > + > +static void ctlv_event_deliver(struct ctlv_op_ctx *ctx, struct list_head *spare) > +{ > + struct ctlv_device *ctlvdev = ctx->ctlvdev; > + struct ctlv_event_record *rec; > + struct ctlv_file *cfile; > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + if (ctlvdev->dead_announced) > + return; > + list_for_each_entry(cfile, &ctlvdev->files, node) { > + rec = list_first_entry_or_null(spare, struct ctlv_event_record, node); > + ctlv_event_queue_event(cfile, ctx->op, rec); > + } > +} > + > +int ctlv_event_emit(struct ctlv_op_ctx *ctx) When `ctlvdev->dead_announced` is set this returns 0 having delivered nothing; fine, but the kernel-doc in `ctlv.h` says only "0, or a negative error" and does not say that success does not imply delivery (also true for dropped events). > +{ > + struct ctlv_device *ctlvdev = ctx->ctlvdev; > + struct ctlv_event_record *rec, *next; > + struct ctlv_file *cfile; > + u32 len, wanted = 0; > + LIST_HEAD(spare); > + int ret; > + > + if (WARN_ON_ONCE(ctx->file)) > + return -EINVAL; > + ret = ctlv_event_finish(ctx); > + if (ret) > + goto out_free; > + > + len = sizeof(struct ctlv_event_hdr) + CTLV_ALIGN(ctx->output.root->len); > + scoped_guard(spinlock, &ctlvdev->events_lock) { > + if (ctlvdev->dead_announced) > + goto out_free; > + list_for_each_entry(cfile, &ctlvdev->files, node) > + if (test_bit(ctx->op->event_index, cfile->subscribed)) > + wanted++; > + } > + if (wanted) { > + ctlv_event_copies(ctx, len, &spare, wanted); > + ctlv_event_deliver(ctx, &spare); > + list_for_each_entry_safe(rec, next, &spare, node) > + kfree(rec); > + } > + > +out_free: > + ctlv_event_free(ctx); > + return ret; > +} > +EXPORT_SYMBOL_NS_GPL(ctlv_event_emit, "CTLV"); > + > +void ctlv_event_abort(struct ctlv_op_ctx *ctx) > +{ > + if (WARN_ON_ONCE(ctx->file)) > + return; > + ctlv_event_free(ctx); > +} > +EXPORT_SYMBOL_NS_GPL(ctlv_event_abort, "CTLV"); > + > +void ctlv_event_lost(struct ctlv_device *ctlvdev, u64 op_id) > +{ > + const struct ctlv_op_entry *op; > + struct ctlv_file *cfile; > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + if (!ctlvdev->events_live || ctlvdev->dead_announced) > + return; > + op = ctlv_op_find(ctlvdev, op_id); > + if (WARN_ON_ONCE(!op) || WARN_ON_ONCE(op->schema->type != CTLV_OP_TYPE_EVENT)) > + return; > + list_for_each_entry(cfile, &ctlvdev->files, node) > + ctlv_event_queue_event(cfile, op, NULL); > +} > +EXPORT_SYMBOL_NS_GPL(ctlv_event_lost, "CTLV"); > + > +void ctlv_event_op_changed(struct ctlv_device *ctlvdev, const struct ctlv_op_entry *op, This is called from `ctlv_device_op_state_set()` with `transition_lock` held, so the lock order `transition_lock` -> `events_lock` is established here, and the `GFP_NOWAIT` usage is required. Neither the ordering nor the atomic-context requirement is documented in this patch; add a comment/lockdep annotation, and state in `ctlv.h` that emit/abort are callable under a spinlock when the ctx was created with an atomic gfp (the header says only 'not for interrupt context'). > + bool disabled, u64 generation) > +{ > + struct ctlv_cursor *reply; > + struct ctlv_op_ctx *ctx; > + > + ctx = ctlv_op_changed_create(ctlvdev, &reply, GFP_NOWAIT); > + if (IS_ERR(ctx)) { > + ctlv_event_lost(ctlvdev, CTLV_OP_OP_CHANGED); > + return; > + } > + ctlv_op_id_put(reply, op->schema->op_id); > + ctlv_op_disabled_put(reply, disabled); > + ctlv_op_state_generation_put(reply, generation); > + ctlv_event_emit(ctx); The return value of `ctlv_event_emit()` is ignored; if the emit fails (finish rejects the event) the loss is not reported via `ctlv_event_lost()` as the create failure is. Subscribers then miss an op-changed event silently. > +} > + > +void ctlv_event_device_dead(struct ctlv_device *ctlvdev) > +{ > + struct ctlv_file *cfile; > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + if (WARN_ON_ONCE(ctlvdev->dead_announced)) > + return; > + list_for_each_entry(cfile, &ctlvdev->files, node) > + ctlv_event_queue_link(cfile, cfile->removed); > + /* Publish dead only after terminal records are queued. */ > + ctlvdev->dead_announced = true; > + list_for_each_entry(cfile, &ctlvdev->files, node) > + wake_up_interruptible(&cfile->readers); > +} > + > +/* Materialize the final loss count when taking the marker. */ > +static void ctlv_event_overflow_fill(struct ctlv_file *cfile) > +{ > + struct ctlv_device *ctlvdev = cfile->ctlvdev; > + struct ctlv_op_ctx ctx = { .ctlvdev = ctlvdev }; > + struct ctlv_cursor *root; > + > + ctlv_event_record_build(&ctx, cfile->overflow, ctlvdev->overflow_op); > + cfile->overflow->hdr.sequence = cfile->first_dropped; > + root = &ctx.output_root; > + ctlv_event_first_sequence_put(root, cfile->first_dropped); > + ctlv_event_last_sequence_put(root, cfile->last_dropped); > + ctlv_event_dropped_count_put(root, cfile->dropped); > + /* Reserved storage is the most this event can be, so it all fits. */ > + WARN_ON_ONCE(ctx.output.poison); > + ctlv_event_record_done(&ctx, cfile->overflow); > + > + cfile->reading_overflow = true; > + cfile->overflowing = false; > + cfile->first_reported = cfile->first_dropped; > + cfile->last_reported = cfile->last_dropped; > + cfile->dropped_reported = cfile->dropped; > + cfile->dropped = 0; > +} > + > +static struct ctlv_event_record *ctlv_event_queue_take(struct ctlv_file *cfile) > +{ > + struct ctlv_event_record *rec; > + > + guard(spinlock) > + (&cfile->ctlvdev->events_lock); > + rec = list_first_entry_or_null(&cfile->records, struct ctlv_event_record, node); > + if (!rec) > + return NULL; > + list_del_init(&rec->node); > + if (rec == cfile->overflow) > + ctlv_event_overflow_fill(cfile); > + else if (!rec->reserved) > + cfile->queue_used -= rec->len; > + return rec; > +} > + > +/* Restore an undelivered record, merging an overflow marker's epoch. */ > +static void ctlv_event_queue_untake(struct ctlv_file *cfile, struct ctlv_event_record *rec) > +{ > + guard(spinlock) > + (&cfile->ctlvdev->events_lock); > + if (rec == cfile->overflow) { > + cfile->reading_overflow = false; > + cfile->first_dropped = cfile->first_reported; > + if (!cfile->overflowing) > + cfile->last_dropped = cfile->last_reported; > + cfile->dropped += cfile->dropped_reported; > + cfile->overflowing = true; > + } else if (!rec->reserved) { > + cfile->queue_used += rec->len; > + } > + list_add(&rec->node, &cfile->records); > +} > + > +/* Requeue a marker if another loss epoch opened while it was out. */ > +static void ctlv_event_queue_consumed(struct ctlv_file *cfile, struct ctlv_event_record *rec) > +{ > + if (rec == cfile->overflow) { > + guard(spinlock) > + (&cfile->ctlvdev->events_lock); > + cfile->reading_overflow = false; > + if (cfile->overflowing) > + ctlv_event_queue_add(cfile, rec); > + return; > + } > + if (!rec->reserved) > + kfree(rec); > +} > + > +static bool ctlv_event_waiting(struct ctlv_file *cfile) > +{ > + struct ctlv_device *ctlvdev = cfile->ctlvdev; > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + return !list_empty(&cfile->records) || ctlvdev->dead_announced; > +} > + > +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; > + > + while (!(rec = ctlv_event_queue_take(cfile))) { > + /* Drained, and the device is gone. */ > + if (READ_ONCE(ctlvdev->dead_announced)) { Race: `ctlv_event_queue_take()` returned `NULL` under `events_lock`, and the device can then be marked dead (queuing every file's 'removed' record) before this unlocked `READ_ONCE`. The reader then returns `-ENODEV` although a removed record is now queued, so the first read after removal fails without delivering the device-removed event. A second read would deliver it, but a reader that stops on `-ENODEV` never sees it. Re-check with `take()` (or test `dead_announced` inside the same critical section as the emptiness check). Also `dead_announced` is written under the lock with a plain store, so the lockless `READ_ONCE` has no `WRITE_ONCE` partner. > + ret = -ENODEV; > + goto out_unlock; > + } > + if (!blocking) { > + ret = -EAGAIN; > + goto out_unlock; > + } > + ret = wait_event_interruptible(cfile->readers, ctlv_event_waiting(cfile)); `wait_event_interruptible()` already returns `-ERESTARTSYS`; the 'ret = `-ERESTARTSYS`' below is redundant. Also this sleeps with `cfile->reading` held, so any other reader of the same description blocks in `mutex_lock_interruptible()` behind it even if it wanted non-blocking behaviour (`read(2)` with `O_NONBLOCK` would not return `-EAGAIN`; the flag is per description so this needs a dup'd fd with a flag change, but a `mutex_trylock` for !blocking is cheap and correct). > + if (ret) { > + ret = -ERESTARTSYS; > + goto out_unlock; > + } > + } > + > + if (rec->len > len) { Three changes requested for a buffer that is too small. 1. Peek at the head record's length under `events_lock` and return `-EMSGSIZE` without dequeuing it. Today `take()` has already run `ctlv_event_overflow_fill()`, which rebuilds the overflow marker, and `untake()` must undo that and merge the loss epochs back. 2. Return 0 immediately for a zero-length `read()`. It now waits for an event and then fails with `-EMSGSIZE`. 3. Document in the UAPI header comment for `struct ctlv_event_hdr` that `read()` needs a buffer of at least `length` bytes, that a smaller one fails with `-EMSGSIZE` and consumes nothing (so `poll()` keeps reporting `EPOLLIN` and a loop with a too-small buffer spins), and that `event_record_max_len` from `ops-dump` (patch 11) gives the size to use. > + 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; > +} > + > +__poll_t ctlv_event_file_poll(struct ctlv_file *cfile, struct file *filp, > + struct poll_table_struct *wait) > +{ > + struct ctlv_device *ctlvdev = cfile->ctlvdev; > + __poll_t mask = 0; > + > + poll_wait(filp, &cfile->readers, wait); > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + if (!list_empty(&cfile->records)) > + mask |= EPOLLIN | EPOLLRDNORM; > + /* Which a reader sees beside what it has not drained yet. */ > + if (ctlvdev->dead_announced) > + mask |= EPOLLHUP; > + return mask; > +} > + > +/* Validate every ID before changing any subscription. */ > +static const struct ctlv_op_entry *ctlv_event_subscription_find(struct ctlv_device *ctlvdev, Two changes requested. 1. Move the "Validate every ID before changing any subscription" comment from `ctlv_event_subscription_find()` to `ctlv_event_subscriptions_set()`, which does the validating. 2. Do the lookup once. Validate the IDs and collect their `event_index` values into a bitmap outside `events_lock`, then apply it with `bitmap_or()` or `bitmap_andnot()` in one short critical section. This drops the second `find()` per attribute, and it bounds the time under the spinlock, which now covers up to ~100K attributes from a 1 MiB message. > + u64 op_id) > +{ > + const struct ctlv_op_entry *op = ctlv_op_find(ctlvdev, op_id); > + > + return op && op->schema->type == CTLV_OP_TYPE_EVENT ? op : NULL; > +} > + > +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); > + 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); > + } > + return 0; > +} > + > +int ctlv_subscriptions_add_action(void *priv, struct ctlv_op_ctx *ctx, @priv and @reply are unused in all the handlers here (callback signature, so fine), but `ctx->file` is dereferenced without a `NULL` check; these handlers must never be dispatched without a file. Document/assert that. > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + return ctlv_event_subscriptions_set(ctx, request, ctlv_subscriptions_add_event_op_id_ref(), > + true); > +} > + > +int ctlv_subscriptions_remove_action(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + return ctlv_event_subscriptions_set(ctx, request, > + ctlv_subscriptions_remove_event_op_id_ref(), false); > +} > + > +int ctlv_event_queue_get_query(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct ctlv_file *cfile = ctx->file; > + > + scoped_guard(spinlock, &ctx->ctlvdev->events_lock) { > + ctlv_event_queue_len_put(reply, cfile->queue_len); > + ctlv_event_queue_used_put(reply, cfile->queue_used); > + } > + /* One state, whole, so there is never a second fragment of it. */ > + return ctlv_op_query_done(ctx, 0); > +} > + > +/* Resizing never discards queued records or reserved capacity. */ > +int ctlv_event_queue_set_action(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct ctlv_device *ctlvdev = ctx->ctlvdev; > + u64 len = ctlv_event_queue_set_event_queue_len_get(request); > + > + if (len < ctlvdev->queue_min || len > CTLV_MAX_EVENT_QUEUE_LEN) > + return ctlv_op_errorf(ctx, -EINVAL, "event queue length %llu is outside %u..%u", > + (unsigned long long)len, ctlvdev->queue_min, > + CTLV_MAX_EVENT_QUEUE_LEN); > + > + guard(spinlock) > + (&ctlvdev->events_lock); > + if (len < ctx->file->queue_used) > + return ctlv_op_error(ctx, -EBUSY, Minor: `ctlv_op_error()` is called with `events_lock` held; it only `strscpy()`s so it is safe, but better to unlock first and keep the lock region minimal. Also the scoped/unscoped mixing (`guard()` for the whole function after the early return) is inconsistent with the `scoped_guard()` used in the sibling query. > + "event queue contains more data than the requested limit"); > + ctx->file->queue_len = len; > + return 0; > +} > diff --git a/drivers/ctlv/event.h b/drivers/ctlv/event.h > new file mode 100644 > index 000000000000..80898c770034 > --- /dev/null > +++ b/drivers/ctlv/event.h > @@ -0,0 +1,69 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef _CTLV_EVENT_H > +#define _CTLV_EVENT_H > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +struct ctlv_device; > +struct ctlv_op_entry; > +struct file; > +struct poll_table_struct; > + > +struct ctlv_event_record { > + struct list_head node; > + u32 len; > + u32 room; > + bool reserved; > + struct ctlv_event_hdr hdr; > + u8 root[] __aligned(8); > +}; > + > +struct ctlv_file { `struct ctlv_file` has no kernel-doc/locking annotations: which fields are protected by `events_lock` (everything but 'readers' and 'reading'), which by 'reading', and who owns lifetime. Given the amount of state (13 fields of overflow bookkeeping) this really needs a comment. Also placing `struct ctlv_file` in `event.h` while it is the per-open object of the whole device (`core.c`) is odd. > + struct ctlv_device *ctlvdev; > + struct list_head node; > + > + struct list_head records; > + u32 queue_len; > + u32 queue_used; > + u64 sequence; > + unsigned long *subscribed; > + struct ctlv_event_record *overflow; > + struct ctlv_event_record *removed; > + bool overflowing; > + u64 first_dropped; > + u64 last_dropped; > + u64 dropped; > + bool reading_overflow; > + u64 first_reported; > + u64 last_reported; > + u64 dropped_reported; > + > + wait_queue_head_t readers; > + /* One reader of this description at a time, however many hold it. */ > + struct mutex reading; > +}; > + > +int ctlv_event_sizes(struct ctlv_device *ctlvdev); > +void ctlv_event_device_init(struct ctlv_device *ctlvdev); > +void ctlv_event_contexts_start(struct ctlv_device *ctlvdev); > +void ctlv_event_contexts_stop(struct ctlv_device *ctlvdev); > +void ctlv_event_contexts_wait(struct ctlv_device *ctlvdev); > +void ctlv_event_device_dead(struct ctlv_device *ctlvdev); > +int ctlv_event_file_init(struct ctlv_file *cfile); > +void ctlv_event_file_exit(struct ctlv_file *cfile); > +ssize_t ctlv_event_file_read(struct ctlv_file *cfile, char __user *buf, > + size_t len, bool blocking); > +__poll_t ctlv_event_file_poll(struct ctlv_file *cfile, struct file *filp, > + struct poll_table_struct *wait); > +void ctlv_event_op_changed(struct ctlv_device *ctlvdev, > + const struct ctlv_op_entry *op, bool disabled, > + u64 generation); > + > +#endif /* _CTLV_EVENT_H */ > diff --git a/drivers/ctlv/internal.h b/drivers/ctlv/internal.h > index f74724269628..7e6ec9d1f73e 100644 > --- a/drivers/ctlv/internal.h > +++ b/drivers/ctlv/internal.h > @@ -16,6 +16,7 @@ > #include "message.h" > > struct task_struct; > +struct ctlv_file; > > #define CTLV_ERROR_MESSAGE_LEN 256 > > @@ -29,6 +30,7 @@ struct ctlv_op_entry { > struct ctlv_op_sizes sizes; > u32 state_index; > bool absent; > + u32 event_index; `event_index` is placed after a bool, leaving a hole in `ctlv_op_entry`; trivial. It is only assigned in the dispatch patch (`state->events`++), so here it is read (`test_bit(op->event_index, ...)`) before anything sets it. > }; > > struct ctlv_error { > @@ -52,9 +54,12 @@ struct ctlv_query_fragment { > }; > > struct ctlv_op_ctx { > + struct ctlv_device *ctlvdev; > const struct ctlv_family *family; > + struct ctlv_file *file; > /* Compared against current and never followed: identity, not a hold. */ > const struct task_struct *submitter; > + gfp_t gfp; gfp is meaningful only for event contexts; for every request context it stays 0 (not `GFP_KERNEL`, not `GFP_NOWAIT`: no reclaim bits at all). A field that is valid for one of the two uses of the struct is a trap; document it or keep it off ctx (e.g. in a wrapper for event ctxs). > const struct ctlv_op_entry *op; > > struct ctlv_msg request; > @@ -71,4 +76,6 @@ struct ctlv_op_ctx { > struct ctlv_error error; > }; > > +struct ctlv_op_entry *ctlv_op_find(struct ctlv_device *ctlvdev, u64 op_id); Declared here, but defined only in the dispatch patch (`core.c`), so event.o and introspection.o have an unresolved symbol until then. Not an issue while `CONFIG_CTLV` is unwired, but it contradicts the 'compile-checked' wording in the commit message (compile yes, link no). > + > #endif /* _CTLV_INTERNAL_H */ > diff --git a/include/linux/ctlv.h b/include/linux/ctlv.h > index dd2206cccfb5..cdfe3e8e930d 100644 > --- a/include/linux/ctlv.h > +++ b/include/linux/ctlv.h > @@ -164,4 +164,31 @@ int ctlv_blob_read(struct ctlv_blob *blob, u64 offset, void *dst, size_t len); > int ctlv_blob_write(struct ctlv_blob *blob, u64 offset, const void *src, > size_t len); > > +/** > + * ctlv_event_lost - Report an event that could not be created. Kernel-doc: `ctlv_event_lost()` says it 'does not allocate or sleep' but it takes a spinlock and may queue overflow markers (`wake_up`); fine, but it is also called by `ctlv_event_op_changed()` while holding `transition_lock`. Document that a caller may hold a spinlock. 'Not for interrupt context' is correct because `events_lock` is taken without irqsave, but then the reason should be stated. > + * @ctlvdev: CTLV device > + * @op_id: event operation ID > + * > + * This does not allocate or sleep. It is not for interrupt context. > + */ > +void ctlv_event_lost(struct ctlv_device *ctlvdev, u64 op_id); > + > +/** > + * ctlv_event_emit - Validate and publish a completed event. > + * @ctx: context returned by a generated event create helper > + * > + * This consumes @ctx. It is not for interrupt context. > + * > + * Return: 0, or a negative error. > + */ > +int ctlv_event_emit(struct ctlv_op_ctx *ctx); > + > +/** > + * ctlv_event_abort - Discard an event under construction. > + * @ctx: context returned by a generated event create helper > + * > + * This consumes @ctx. It is not for interrupt context. > + */ > +void ctlv_event_abort(struct ctlv_op_ctx *ctx); > + > #endif /* _LINUX_CTLV_H */ > diff --git a/include/linux/ctlv_family.h b/include/linux/ctlv_family.h > index 5cdcdfeec651..4f5141fc7ccf 100644 > --- a/include/linux/ctlv_family.h > +++ b/include/linux/ctlv_family.h > @@ -338,6 +338,17 @@ bool ctlv_nest_start(struct ctlv_cursor *nest, struct ctlv_cursor *child, > > struct ctlv_blob *ctlv_blob_resolve(struct ctlv_op_ctx *ctx, > const struct ctlv_cursor *blob); > +struct ctlv_op_ctx *ctlv_event_create(struct ctlv_device *ctlvdev, u64 op_id, > + struct ctlv_cursor **event, gfp_t gfp); No kernel-doc for `ctlv_event_create()`, and the parameter is named @event here but @reply in the definition. The `CTLV_EVENT_CREATE` macro below is also undocumented, is glued to the preceding declaration with no blank line, and the continuation backslashes are misaligned (the 'return `ctlv_event_create(...)`' line overshoots the column). Its first user (`ctlv_gen.h`) arrived four patches ago. > + > +#define CTLV_EVENT_CREATE(_name, _op_id) \ > +static inline struct ctlv_op_ctx * \ > +_name##_create(struct ctlv_device *ctlvdev, \ > + struct ctlv_cursor **reply, gfp_t gfp) \ > +{ \ > + return ctlv_event_create(ctlvdev, (_op_id), reply, gfp); \ > +} > + > #define __CTLV_DEFINE_PUT(_name, _type, _put_fn, _attr_id, _kind) \ > static inline bool _name##_put(struct ctlv_cursor *nest, \ > _type value) \ > -- > 2.55.0 > Summary of the most serious issues: - Commit message is wrong: the diff adds no generated root bindings, and `ctlv_gen`.o is not built until the last patch; `ctlv_gen.h` from patch 5 only becomes self-consistent here (`CTLV_EVENT_CREATE`). - `device.h` is created incomplete; patch 11 relies on fields (`transition_lock`, `state_generation`, `dynamic_ops_count`, disabled) that do not exist until the dispatch patch, so the series does not compile-check as claimed. - `ctlv_event_file_read()` can return `-ENODEV` just after the device-removed record was queued, dropping the removal notification for that read. - `overflow_op`/`removed_op` point into ops[] which is compacted in place later. - Redundant/dead checks (family `NULL`, WARN on missing root events), unused ctlvdev parameter, discarded validation error text, ignored emit return, undocumented lock ordering `transition_lock` -> `events_lock`, and the broken `guard()` formatting throughout.