Date:   Thu, 16 Dec 2021 10:57:04 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     David Vernet <void@manifault.com>
Cc:     live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, jpoimboe@redhat.com, jikos@kernel.org,
        mbenes@suse.cz, joe.lawrence@redhat.com, corbet@lwn.net
Subject: Re: [PATCH v2] Documentation: livepatch: Add livepatch API page
Message-ID: <YbsNcAKzRCxGqXUA@alley>
References: <20211215174659.2332589-1-void@manifault.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211215174659.2332589-1-void@manifault.com>
Precedence: bulk

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