[PATCH] Documentation: livepatch: Add kernel-doc link to klp_enable_patch

[PATCH] Documentation: livepatch: Add kernel-doc link to klp_enable_patch

[David Vernet]

The `klp_enable_patch()` function is the main entrypoint to the livepatch
subsystem, and is invoked by a KLP module from the module_init callback
when it is ready to be enabled.  The livepatch documentation specifies that
`klp_enable_patch()` should be invoked from the `module_init()` callback,
but does not actually link the user to the function's kerneldoc comment.

This simple change therefore adds a kernel-doc directive to link the
`klp_enable_patch()` function's kerneldoc comment in the livepatch
documentation page. With this, kernel/livepatch/core.c no longer comes up
as a file containing an unused doc with
`scripts/find-unused-docs.sh kernel/livepatch`

Signed-off-by: David Vernet <void@manifault.com>
---
 Documentation/livepatch/livepatch.rst | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/livepatch/livepatch.rst b/Documentation/livepatch/livepatch.rst
index 68e3651e8af9..4fa9e4ebcc3a 100644
--- a/Documentation/livepatch/livepatch.rst
+++ b/Documentation/livepatch/livepatch.rst
@@ -312,8 +312,15 @@ the patch cannot get enabled.
 -------------
 
 The livepatch gets enabled by calling klp_enable_patch() from
-the module_init() callback. The system will start using the new
-implementation of the patched functions at this stage.
+the module_init() callback:
+
+.. kernel-doc:: kernel/livepatch/core.c
+   :functions: klp_enable_patch
+
+----
+
+The system will start using the new implementation of the patched functions at
+this stage.
 
 First, the addresses of the patched functions are found according to their
 names. The special relocations, mentioned in the section "New functions",
-- 
2.30.2

Re: [PATCH] Documentation: livepatch: Add kernel-doc link to klp_enable_patch

[Petr Mladek]

On Thu 2021-12-09 08:53:04, David Vernet wrote:
> The `klp_enable_patch()` function is the main entrypoint to the livepatch
> subsystem, and is invoked by a KLP module from the module_init callback
> when it is ready to be enabled.  The livepatch documentation specifies that
> `klp_enable_patch()` should be invoked from the `module_init()` callback,
> but does not actually link the user to the function's kerneldoc comment.
> 
> This simple change therefore adds a kernel-doc directive to link the
> `klp_enable_patch()` function's kerneldoc comment in the livepatch
> documentation page. With this, kernel/livepatch/core.c no longer comes up
> as a file containing an unused doc with
> `scripts/find-unused-docs.sh kernel/livepatch`
> 
> --- a/Documentation/livepatch/livepatch.rst
> +++ b/Documentation/livepatch/livepatch.rst
> @@ -312,8 +312,15 @@ the patch cannot get enabled.
>  -------------
>  
>  The livepatch gets enabled by calling klp_enable_patch() from
> -the module_init() callback. The system will start using the new
> -implementation of the patched functions at this stage.
> +the module_init() callback:
> +
> +.. kernel-doc:: kernel/livepatch/core.c
> +   :functions: klp_enable_patch
> +
> +----
> +
> +The system will start using the new implementation of the patched functions at
> +this stage.
>  
>  First, the addresses of the patched functions are found according to their
>  names. The special relocations, mentioned in the section "New functions",

Honestly, I do not like this. It might be acceptable when it converts
klp_enable_patch() into a link pointing to another page describing the API.

But this patch causes the entire documentation of klp_enable_patch()
inserted into livepatch.html. It does not fit there and breaks
the text flow.


Heh, I had hard times to build the documentation (sphinx crashed, ...).
So, I paste the html output for others here:

<cut&paste>
5.2. Enabling¶
The livepatch gets enabled by calling klp_enable_patch() from the module_init() callback:

int klp_enable_patch(struct klp_patch *patch)¶
enable the livepatch

Parameters

struct klp_patch *patch
patch to be enabled

Description

Initializes the data structure associated with the patch, creates the sysfs interface, performs the needed symbol lookups and code relocations, registers the patched functions with ftrace.

This function is supposed to be called from the livepatch module_init() callback.

Return

0 on success, otherwise error

The system will start using the new implementation of the patched functions at this stage.

First, the addresses of the patched functions are found according to their names. The special relocations, mentioned in the section “New functions”, are applied. The relevant entries are created under /sys/kernel/livepatch/<name>. The patch is rejected when any above operation fails.

Second, livepatch enters into a transition state where tasks are converging to the patched state. If an original function is patched for the first time, a function specific struct klp_ops is created and an universal ftrace handler is registered1. This stage is indicated by a value of ‘1’ in /sys/kernel/livepatch/<name>/transition. For more information about this process, see the “Consistency model” section.

Finally, once all tasks have been patched, the ‘transition’ value changes to ‘0’.

1
Note that functions might be patched multiple times. The ftrace handler is registered only once for a given function. Further patches just add an entry to the list (see field func_stack) of the struct klp_ops. The right implementation is selected by the ftrace handler, see the “Consistency model” section.

That said, it is highly recommended to use cumulative livepatches
because they help keeping the consistency of all changes. In this
case, functions might be patched two times only during the transition
period.
</cut&paste>


Best Regards,
Petr

Re: [PATCH] Documentation: livepatch: Add kernel-doc link to klp_enable_patch

[David Vernet]

Petr Mladek <pmladek@suse.com> wrote on Fri [2021-Dec-10 10:25:05 +0100]:
> 
> Honestly, I do not like this. It might be acceptable when it converts
> klp_enable_patch() into a link pointing to another page describing the API.
> 
> But this patch causes the entire documentation of klp_enable_patch()
> inserted into livepatch.html. It does not fit there and breaks
> the text flow.

Thank you for taking a look at the patch, Petr.

I'm happy to revise the patch to instead add a new `api.rst` file that
contains the `kernel-doc` directive, which would cause `klp_enable_patch()`
in `livepatch.rst` to automatically link to the separate page as you
suggested.

Just to check though -- I see that `shadow-vars.rst` and `system-state.rst`
have their own "API" sections. Is it preferable to add such a section
directly to `livepatch.rst`, rather than creating a separate file?

Let me know either way and I'll send a v2 patch with your preference.

Kind regards,
David

Re: [PATCH] Documentation: livepatch: Add kernel-doc link to klp_enable_patch

[Petr Mladek]

On Fri 2021-12-10 10:24:58, David Vernet wrote:
> Petr Mladek <pmladek@suse.com> wrote on Fri [2021-Dec-10 10:25:05 +0100]:
> > 
> > Honestly, I do not like this. It might be acceptable when it converts
> > klp_enable_patch() into a link pointing to another page describing the API.
> > 
> > But this patch causes the entire documentation of klp_enable_patch()
> > inserted into livepatch.html. It does not fit there and breaks
> > the text flow.
> 
> Thank you for taking a look at the patch, Petr.
> 
> I'm happy to revise the patch to instead add a new `api.rst` file that
> contains the `kernel-doc` directive, which would cause `klp_enable_patch()`
> in `livepatch.rst` to automatically link to the separate page as you
> suggested.
> 
> Just to check though -- I see that `shadow-vars.rst` and `system-state.rst`
> have their own "API" sections.

Good point. Well, even these file do not point to the documentation
generated from the sources.


> Is it preferable to add such a section directly to `livepatch.rst`,
> rather than creating a separate file?

Good question. I am not expert on writing documentation and I can't
find any good example of API documentation at
https://www.kernel.org/doc/html/latest/index.html

One reason might be that most of the documentation was written as plain text
in the past. And many people still read it in the original .rst form.

Another problem is that livepatch documentation is a mix of several
independent pieces written by different authors. It would deserve
a lot of work:

    + Connect the pieces
    + Add missing information
    + Make the style and structure consistent


Anyway, I think that the documentation generated from the sources
is useful. But it is hard to integrate it into .rst file that should
be useful even in the .rst format.

From this POV, I suggest to create Documentation/livepatch/API.rst
and add there the documentation generated from the sources. I mean
something like:

    Documentation/core-api/kernel-api.rst

that results into

https://www.kernel.org/doc/html/latest/core-api/kernel-api.html


The livepatch/API.rst might include documentation from

    include/linux/livepatch.h
    kernel/livepatch/code.c
    kernel/livepatch/shadow.c
    kernel/livepatch/state.c


But let's wait if there are other opinions from another livepatch
developers.

Best Regards,
Petr

Re: [PATCH] Documentation: livepatch: Add kernel-doc link to klp_enable_patch

[Miroslav Benes]

> From this POV, I suggest to create Documentation/livepatch/API.rst
> and add there the documentation generated from the sources. I mean
> something like:
> 
>     Documentation/core-api/kernel-api.rst
> 
> that results into
> 
> https://www.kernel.org/doc/html/latest/core-api/kernel-api.html
> 
> 
> The livepatch/API.rst might include documentation from
> 
>     include/linux/livepatch.h
>     kernel/livepatch/code.c
>     kernel/livepatch/shadow.c
>     kernel/livepatch/state.c
> 
> 
> But let's wait if there are other opinions from another livepatch
> developers.

Yes, please. Do not include anything generated from the source into our 
"hand-written" documentation. Or at least not like the proposed patch. It 
breaks it as Petr pointed out. A separate API file sounds better to me.

Regards
Miroslav