Re: [PATCH RFC v2 13/15] Documentation: ctlv: Describe how to write a family

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] docs: ctlv: direct -ESTALE return in query example skips
  error-info
- [Low] docs: ctlv: inaccurate claim that unfilled extension adds no
  record
--

commit 37d8b6224a7e92519379e383dd7fb525d99c7126
Author: Jiri Pirko <jiri@resnulli.us>

Documentation: ctlv: Describe how to write a family

This commit adds driver-api documentation explaining how to define a CTLV
family specification in YAML and implement its kernel callbacks.

> diff --git a/Documentation/driver-api/ctlv.rst b/Documentation/driver-api/ctlv.rst
> new file mode 100644
> index 0000000000000..34d20c14a9bfa
> --- /dev/null
> +++ b/Documentation/driver-api/ctlv.rst
> @@ -0,0 +1,716 @@

[ ... ]

> +    int ctlv_example_item_get_query(struct ctlv *ctlv,
> +                                      struct ctlv_op_ctx *ctx,
> +                                      const struct ctlv_cursor *request,
> +                                      struct ctlv_cursor *reply)
> +    {
> +            struct example_device *example = to_example(ctlv);
> +            u64 generation = READ_ONCE(example->generation);
> +            u64 cursor = ctlv_op_query_cursor(ctx);
> +            struct ctlv_cursor item;
> +            u64 i = cursor ? cursor - 1 : 0;
> +
> +            if (cursor && ctlv_op_query_generation(ctx) != generation)
> +                    return -ESTALE;

[Severity: Low]
Does returning -ESTALE directly here skip populating ctx->error for
ctlv_error_info()?

ctlv_query_fragment_end() (called by ctlv_op_query_done() and
ctlv_op_query_more()) is the only place that sets
CTLV_VALIDATION_REASON_STALE_GENERATION along with expected_generation and
current_generation:

lib/ctlv/builder.c:ctlv_query_fragment_end() {
    ...
	if (ctx->frag.cursor && generation != ctx->frag.generation) {
		ctx->error.reason = CTLV_VALIDATION_REASON_STALE_GENERATION;
		ctx->error.expected_generation = ctx->frag.generation;
		ctx->error.current_generation = generation;
		return -ESTALE;
	}
    ...
}

When a callback returns -ESTALE directly, ctlv_error_info() sees
ctx->error.reason == 0 and omits the stale-generation error-info message
documented in Documentation/userspace-api/ctlv.rst.  Calling
ctlv_op_query_done(ctx, generation) on mismatch or letting the loop finish
into ctlv_op_query_done() would populate the error info and return
-ESTALE.

[ ... ]

> +The ``device-get`` callback in ``ctlv-example`` opens the record and has the
> +device's family fill it:
> +
> +.. code-block:: c
> +
> +    if (ctlv_example_device_get_details_start(reply, &details)) {
> +            ret = ctlv_ext_fill(ctx, NULL, &details);
> +            ctlv_record_end(&details);
> +            if (ret)
> +                    return ret;
> +    }
> +
> +The foo family implements the generated fill callback:
> +
> +.. code-block:: c
> +
> +    int ctlv_example_foo_device_details_fill(struct ctlv *ctlv,
> +                                             struct ctlv_op_ctx *ctx,
> +                                             const struct ctlv_cursor *request,
> +                                             struct ctlv_cursor *record)
> +    {
> +            struct foo_device *foo = container_of(ctlv, struct foo_device,
> +                                                  example.ctlv);
> +
> +            ctlv_example_foo_device_details_sram_bank_put(record,
> +                                                          foo->sram_bank);
> +            return 0;
> +    }
> +
> +``request`` is the matching record of the request, for an op whose request has
> +an extension point too; ``device-get`` has none and passes ``NULL``. An
> +extension point no family fills adds no record and no length.

[Severity: Low]
Is it accurate that an unfilled extension point adds no record and no
length?

ctlv_example_device_get_details_start() calls ctlv_record_start(), which
writes a 16-byte struct ctlv_hdr with CTLV_HDR_F_RECORD into the reply and
adds 16 bytes to each ancestor record's length.  When no family in the
chain fills the extension point, ctlv_ext_fill() returns 0 and
ctlv_record_end() leaves that empty 16-byte record in the output buffer
(and the schema size calculations in schema.c and sizes.py likewise count
16 bytes for the empty extension record header).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155515.2510194-1-jiri@resnulli.us?part=13