Re: [PATCH] docs: printk-basics: update the pr_debug() kerneldoc

[Petr Mladek <pmladek@suse.com>]

On Wed 2020-04-22 16:03:34, Ricardo Cañuelo wrote:
> This updates the kerneldoc comment for the pr_debug() macro to describe
> the new set of kernel config options it's affected by.
> 
> It also simplifies the description of the pr_debug() and pr_devel()
> macros in printk-basics.rst, forwarding the reader to the function
> reference.
> 
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
> Some background:
> 
> The previous patch I sent to add kerneldocs to printk.h:
> https://lore.kernel.org/linux-doc/20200420171544.3c443e36@lwn.net/
> 
> conflicted with this other patch:
> https://lkml.org/lkml/2020/4/20/1320
> 
> during the manual linux-next merge. Stephen Rothwell fixed the conflict
> but the description of what pr_debug() does needed to be updated to
> reflect the changes introduced in the patch by Orson Zhai.
> 
> Tested on linux-next with make htmldocs and make pdfdocs.
> 
>  Documentation/core-api/printk-basics.rst | 4 ++--
>  include/linux/printk.h                   | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-basics.rst b/Documentation/core-api/printk-basics.rst
> index 563a9ce5fe1d..84c853e17200 100644
> --- a/Documentation/core-api/printk-basics.rst
> +++ b/Documentation/core-api/printk-basics.rst
> @@ -100,8 +100,8 @@ would prefix every pr_*() message in that file with the module and function name
>  that originated the message.
>  
>  For debugging purposes there are also two conditionally-compiled macros:
> -pr_debug() and pr_devel(), which are compiled-out unless ``DEBUG`` (or
> -also ``CONFIG_DYNAMIC_DEBUG`` in the case of pr_debug()) is defined.
> +pr_debug() and pr_devel(), which are compiled-out depending on the kernel
> +configuration options (See the function reference below for more info).
>  
>  
>  Function reference
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 768ac6bc637d..dab23bcbdeb0 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -408,9 +408,10 @@ extern int kptr_restrict;
>   * @fmt: format string
>   * @...: arguments for the format string
>   *
> - * This macro expands to dynamic_pr_debug() if CONFIG_DYNAMIC_DEBUG is
> - * set. Otherwise, if DEBUG is defined, it's equivalent to a printk with
> - * KERN_DEBUG loglevel. If DEBUG is not defined it does nothing.
> + * This macro expands to dynamic_pr_debug() if CONFIG_DYNAMIC_DEBUG is set or if
> + * CONFIG_DYNAMIC_DEBUG_CORE and DYNAMIC_DEBUG_MODULE are both set.  Otherwise,
> + * if DEBUG is defined, it's equivalent to a printk with KERN_DEBUG loglevel.
> + * If none of the above is defined it does nothing.
>   *
>   * It uses pr_fmt() to generate the format string (dynamic_pr_debug() uses
>   * pr_fmt() internally).
> -- 
> 2.18.0

It is pity that you did not add other printk maintainers into CC for
the patches adding this documentation and comments. I was sick
last two months and was not able to check mails.

Adding them now. Note that the following patch is already in
linux-next, see
https://lore.kernel.org/r/20200403093617.18003-1-ricardo.canuelo@collabora.com

One note about printk-basics.rst. It should mention that pr_*()
variants are preferred over the generic printk(KERN_* ).

Otherwise, I do not see anything critical.


Well, I have mixed feelings about this type of documentation. It might explain
some things that are less obvious, for example, the meaning of
pr_fmt(). On the other hand:

  + It might be complicated to keep it in sync.

  + I wonder how many developers would actually read it.

  + The doc comments in include/linux/prinkt.h are really
    long and describe obvious things.

By other words. These comments make the headers and sources hard to
read. And at least in this particular case, the gain is questionable.

Best Regards,
Petr