Re: [PATCH RFC 05/20] ctlv: Prepare message input handling

[Jiri Pirko <jiri@resnulli.us>]

Wed, Oct 21, 2026 at 02:43:24AM +0200, jic23@kernel.org wrote:
>On Thu, 15 Oct 2026 16:50:38 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Add wire walking, request indexing and validated input accessors for
>> generated family readers.
>> 
>> Keep CONFIG_CTLV unwired until 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>
>
>Trivial stuff only. I didn't attempt (yet) to check
>all the walking code, just get a feeling for what was here. 
>
>
>> diff --git a/drivers/ctlv/Kconfig b/drivers/ctlv/Kconfig
>> new file mode 100644
>> index 000000000000..aed3243c55fd
>> --- /dev/null
>> +++ b/drivers/ctlv/Kconfig
>> @@ -0,0 +1,12 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +menuconfig CTLV
>> +	tristate "CTLV character device TLV UAPI"
>> +	help
>> +	  CTLV provides character device UAPIs described by a checked-in YAML
>> +	  specification. It owns the character device lifetime, the strict TLV
>> +	  message format, request validation, reply serialization, the large
>> +	  payloads a request describes rather than carries, per-open event
>> +	  queues and runtime schema introspection, so families only implement
>> +	  semantics.
>> +
>> +	  If you don't know what to do here, say N.
>
>I'd not say that.  Overly cautious for something we expect
>to get fairly extensive use.

Okay.


>
>
>> diff --git a/drivers/ctlv/message.c b/drivers/ctlv/message.c
>> new file mode 100644
>> index 000000000000..7e5742ba8f70
>> --- /dev/null
>> +++ b/drivers/ctlv/message.c
>
>> +
>> +bool ctlv_message_input_rebind(struct ctlv_cursor *nest, const struct ctlv_schema_tree *tree,
>> +			       u32 node)
>> +{
>> +	const struct ctlv_message_input_scope *bound_scope;
>> +	const struct ctlv_message_input_nest *indexed;
>> +	u32 i;
>> +
>> +	if (WARN_ON_ONCE(!nest->attr || !nest->msg || !nest->msg->index))
>> +		return false;
>> +	if (!ctlv_message_nest_index_ok(nest))
>> +		return false;
>> +	bound_scope = &nest->msg->index->scopes[nest->index_nest_scope];
>> +	indexed = &nest->msg->index->nests[bound_scope->nest];
>> +	for (i = 0; i < indexed->scopes_count; i++) {
>
>	for (u32 i = 0; ...

Okay.


>
>> +		u32 scope_index = indexed->first_scope + i;
>> +		const struct ctlv_message_input_scope *scope =
>> +			&nest->msg->index->scopes[scope_index];
>> +
>> +		if (scope->tree != tree || scope->node != node)
>> +			continue;
>> +		nest->tree = tree;
>> +		nest->node = node;
>> +		nest->index_nest_scope = scope_index;
>> +		return true;
>> +	}
>> +	return false;
>> +}
>
>> +
>> +void ctlv_attr_next(struct ctlv_cursor *cursor)
>> +{
>> +	const struct ctlv_message_input_slot *slot;
>> +	const struct ctlv_message_input_scope *scope;
>> +	const struct ctlv_node_schema *node;
>> +	const struct ctlv_attr *next;
>> +	u32 next_nest;
>> +	u32 next_offset;
>> +
>> +	slot = ctlv_message_cursor_index(cursor);
>> +	if (!slot || WARN_ON_ONCE(!cursor->attr))
>> +		return;
>> +	next = (const void *)((const u8 *)cursor->attr +
>> +			      ALIGN((size_t)cursor->attr->len, CTLV_ALIGNTO));
>
>kernel code is allowed to rely on void * arithmetic, so this can be
>	next = (const void *)cursor->attr +
>	       ALIGN((size_t)cursor->attr->len, CTLV_ALIGNTO);
>
>container_of() for instance makes use of sizeof(void) == 1.

Okay.


> 
>
>
>> +const void *ctlv_attr_binary(const struct ctlv_cursor *cursor, u32 *len)
>> +{
>> +	const void *value;
>> +
>> +	*len = 0;
>> +	value = ctlv_message_cursor_payload(cursor, BIT(CTLV_SCHEMA_BINARY), len);
>> +	if (!value)
>> +		*len = 0;
>
>I'd kind of have assumed that ctlv_message_cursor_payload() would not
>set len to anything if it returns NULL.  Thus the default *len = 0 would
>be intact.  From a quick look that seems to be the case making this
>code a little odd. 
>
>Perhaps overly defensive or keeping an LLM happy?
>Same applies on some other functions.

You are right. Will remove these.


>
>
>
>> +	return value;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(ctlv_attr_binary, "CTLV");
>> +
>> +/*
>> + * The room is measured before anything is written, which is what leaves a
>> + * refused attr's message exactly as it was, padding included.
>> + */
>
>Having a trailing comment in a file rarely adds much or indeed
>even more rarely gets read.

True, will remove.


Thanks!

>
>
>> diff --git a/drivers/ctlv/message.h b/drivers/ctlv/message.h