[PATCH v2] Documentation: livepatch: Add livepatch API page

[PATCH v2] Documentation: livepatch: Add livepatch API page

[David Vernet]

The livepatch subsystem has several exported functions and objects with
kerneldoc comments. Though the livepatch documentation contains
handwritten descriptions of all of these exported functions, they are
currently not pulled into the docs build using the kernel-doc directive.

In order to allow readers of the documentation to see the full kerneldoc
comments in the generated documentation files, this change adds a new
Documentation/livepatch/api.rst page which contains kernel-doc
directives to link the kerneldoc comments directly in the documentation.
With this, all of the hand-written descriptions of the APIs now
cross-reference the kerneldoc comments on the new Livepatching APIs
page, and running ./scripts/find-unused-docs.sh on kernel/livepatch no
longer shows any files as missing documentation.

Note that all of the handwritten API descriptions were left alone with
the exception of Documentation/livepatch/system-state.rst, which was
updated to allow the cross-referencing to work correctly. The file now
follows the cross-referencing formatting guidance specified in
Documentation/doc-guide/kernel-doc.rst. Furthermore, some comments
around klp_shadow_free_all() were updated to say <obj, id> rather than
<*, id> to match the rest of the file, and to prevent the docs build
from emitting an "Inline emphasis start-string without end string"
error.

Signed-off-by: David Vernet <void@manifault.com>
---
v2:
  - Updated patch description to not use markdown, and to use imperative
    language.
  - Added a new API page that exports all of the public livepatch functions and
    types.
  - Fixed klp_shadow_free_all() documentation to match the rest of the file and
    avoid an rst error.
  - Updated system-state.rst to properly cross-reference functions.

 Documentation/livepatch/api.rst          | 28 ++++++++++++++++++++++++
 Documentation/livepatch/index.rst        |  1 +
 Documentation/livepatch/system-state.rst |  4 ++--
 kernel/livepatch/shadow.c                |  6 ++---
 4 files changed, 34 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/livepatch/api.rst

diff --git a/Documentation/livepatch/api.rst b/Documentation/livepatch/api.rst
new file mode 100644
index 000000000000..8c71b5bd73e8
--- /dev/null
+++ b/Documentation/livepatch/api.rst
@@ -0,0 +1,28 @@
+=================
+Livepatching APIs
+=================
+
+Livepatch Enablement
+====================
+
+.. kernel-doc:: kernel/livepatch/core.c
+   :export:
+
+
+Shadow Variables
+================
+
+.. kernel-doc:: kernel/livepatch/shadow.c
+   :export:
+
+System State Changes
+====================
+
+.. kernel-doc:: kernel/livepatch/state.c
+   :export:
+
+Object Types
+============
+
+.. kernel-doc:: include/linux/livepatch.h
+   :identifiers: klp_patch klp_object klp_func klp_callbacks klp_state
diff --git a/Documentation/livepatch/index.rst b/Documentation/livepatch/index.rst
index 43cce5fad705..cebf1c71d4a5 100644
--- a/Documentation/livepatch/index.rst
+++ b/Documentation/livepatch/index.rst
@@ -14,6 +14,7 @@ Kernel Livepatching
     shadow-vars
     system-state
     reliable-stacktrace
+    api
 
 .. only::  subproject and html
 
diff --git a/Documentation/livepatch/system-state.rst b/Documentation/livepatch/system-state.rst
index c6d127c2d9aa..7a3935fd812b 100644
--- a/Documentation/livepatch/system-state.rst
+++ b/Documentation/livepatch/system-state.rst
@@ -52,12 +52,12 @@ struct klp_state:
 
 The state can be manipulated using two functions:
 
-  - *klp_get_state(patch, id)*
+  - klp_get_state()
 
     - Get struct klp_state associated with the given livepatch
       and state id.
 
-  - *klp_get_prev_state(id)*
+  - klp_get_prev_state()
 
     - Get struct klp_state associated with the given feature id and
       already installed livepatches.
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index e5c9fb295ba9..4c6617aadf87 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -272,12 +272,12 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
 EXPORT_SYMBOL_GPL(klp_shadow_free);
 
 /**
- * klp_shadow_free_all() - detach and free all <*, id> shadow variables
+ * klp_shadow_free_all() - detach and free all <obj, id> shadow variables
  * @id:		data identifier
  * @dtor:	custom callback that can be used to unregister the variable
  *		and/or free data that the shadow variable points to (optional)
  *
- * This function releases the memory for all <*, id> shadow variable
+ * This function releases the memory for all <obj, id> shadow variable
  * instances, callers should stop referencing them accordingly.
  */
 void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
@@ -288,7 +288,7 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
 
 	spin_lock_irqsave(&klp_shadow_lock, flags);
 
-	/* Delete all <*, id> from hash */
+	/* Delete all <obj, id> from hash */
 	hash_for_each(klp_shadow_hash, i, shadow, node) {
 		if (klp_shadow_match(shadow, shadow->obj, id))
 			klp_shadow_free_struct(shadow, dtor);
-- 
2.30.2

Re: [PATCH v2] Documentation: livepatch: Add livepatch API page

[Petr Mladek]

On Wed 2021-12-15 09:47:00, David Vernet wrote:
> The livepatch subsystem has several exported functions and objects with
> kerneldoc comments. Though the livepatch documentation contains
> handwritten descriptions of all of these exported functions, they are
> currently not pulled into the docs build using the kernel-doc directive.
> 
> Note that all of the handwritten API descriptions were left alone with
> the exception of Documentation/livepatch/system-state.rst, which was
> updated to allow the cross-referencing to work correctly. The file now
> follows the cross-referencing formatting guidance specified in
> Documentation/doc-guide/kernel-doc.rst. Furthermore, some comments
> around klp_shadow_free_all() were updated to say <obj, id> rather than
> <*, id> to match the rest of the file, and to prevent the docs build
> from emitting an "Inline emphasis start-string without end string"
> error.
> 
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -272,12 +272,12 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
>  EXPORT_SYMBOL_GPL(klp_shadow_free);
>  
>  /**
> - * klp_shadow_free_all() - detach and free all <*, id> shadow variables
> + * klp_shadow_free_all() - detach and free all <obj, id> shadow variables

This change is not good. The function releases all existing shadow
variables with the given @id for any @obj. And it is not longer clear.

I guess that the primary motivation was to remove  "Inline emphasis
start-string without end string" mentioned in the commit message.

A solution would be replace '*' with something else, for example, < , id>.
Another solution would be to describe it another way, for example:

 * klp_shadow_free_all() - detach and free all <obj, id> shadow variables
 *		with the given @id.

>   * @id:		data identifier
>   * @dtor:	custom callback that can be used to unregister the variable
>   *		and/or free data that the shadow variable points to (optional)
>   *
> - * This function releases the memory for all <*, id> shadow variable
> + * This function releases the memory for all <obj, id> shadow variable

Same here.

>   * instances, callers should stop referencing them accordingly.
>   */
>  void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
> @@ -288,7 +288,7 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
>  
>  	spin_lock_irqsave(&klp_shadow_lock, flags);
>  
> -	/* Delete all <*, id> from hash */
> +	/* Delete all <obj, id> from hash */

and here

>  	hash_for_each(klp_shadow_hash, i, shadow, node) {
>  		if (klp_shadow_match(shadow, shadow->obj, id))
>  			klp_shadow_free_struct(shadow, dtor);

BTW: There is likely the same problem in Documentation/livepatch/shadow-vars.rst.
     I see <*, id> there as well.


Otherwise, the patch looks fine to me.

Best Regards,
Petr

Re: [PATCH v2] Documentation: livepatch: Add livepatch API page

[David Vernet]

Petr Mladek <pmladek@suse.com> wrote on Thu [2021-Dec-16 10:57:04 +0100]:

> This change is not good. The function releases all existing shadow
> variables with the given @id for any @obj. And it is not longer clear.

Good point. I'll address that in v3.

> I guess that the primary motivation was to remove  "Inline emphasis
> start-string without end string" mentioned in the commit message.

Yes, this was the primary and only motivation. <*, id> is much clearer and I'm
with you on finding a better alternative.

> A solution would be replace '*' with something else, for example, < , id>.

I think this is better than just obj, but in my opinion this may be confusing
for readers and look like a typo. I think I prefer your second suggestion,
though obj really makes more sense in the case where we're actually passing an
@obj to the function. I'll probably (deservedly?) get lambasted for suggesting
this, but what about taking a page out of rust's book and doing something like
this:

  * klp_shadow_free_all() - detach and free all <_, id> shadow variables
  *		with the given @id.

to indicate that in this case we don't care about the obj. Even for a reader
unfamiliar with rust, hopefully it would get the point across.

> Another solution would be to describe it another way, for example:
> 
>  * klp_shadow_free_all() - detach and free all <obj, id> shadow variables
>  *		with the given @id.

I'm fine with this as well. Let me know what you think about <_, id> vs. what
you suggested, and I'll send out the v3 patch with your preference.

> BTW: There is likely the same problem in Documentation/livepatch/shadow-vars.rst.
>      I see <*, id> there as well.

Indeed you're correct. There's no warning in the build system because there
happen to be two <*, id> ... <*, id> in a row, so rst happily italicizes what's
between them without question. I'll fix this in the v3 of the patch as well.

> Otherwise, the patch looks fine to me.

Thanks for taking a look and for the helpful suggestions.

- David

Re: [PATCH v2] Documentation: livepatch: Add livepatch API page

[Petr Mladek]

On Thu 2021-12-16 07:00:38, David Vernet wrote:
> Petr Mladek <pmladek@suse.com> wrote on Thu [2021-Dec-16 10:57:04 +0100]:
> 
> > This change is not good. The function releases all existing shadow
> > variables with the given @id for any @obj. And it is not longer clear.
> 
> > A solution would be replace '*' with something else, for example, < , id>.
> 
> I think this is better than just obj, but in my opinion this may be confusing
> for readers and look like a typo. I think I prefer your second suggestion,
> though obj really makes more sense in the case where we're actually passing an
> @obj to the function. I'll probably (deservedly?) get lambasted for suggesting
> this, but what about taking a page out of rust's book and doing something like
> this:
> 
>   * klp_shadow_free_all() - detach and free all <_, id> shadow variables
>   *		with the given @id.
> 
> to indicate that in this case we don't care about the obj. Even for a reader
> unfamiliar with rust, hopefully it would get the point across.

<_, id> looks good to me. And I not even familiar with rust ;-)

Best Regards,
Petr