lkml.org 
[lkml]   [2021]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] Documentation: livepatch: Add livepatch API page
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

\
 
 \ /
  Last update: 2021-12-16 10:58    [W:0.053 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site