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

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

[Ricardo Cañuelo]

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

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

[Petr Mladek]

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

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

[Ricardo Cañuelo]

Hi Petr, thanks for taking the time to review this

On Tue, 2020-05-19 at 15:15 +0200, Petr Mladek wrote:
> 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.

I'm sorry for that and I hope you're better now.

> 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

Indeed, I mentioned that in the patch. Maybe I should've been clearer in the
background description.

> 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.

Well, I think that some things may not be too obvious for beginners and that
documenting them explicitly is a positive thing overall. I don't think this kind
of comments (comment blocks before a function prototype, not interleaved in the
code) pollute the code or make it harder to read, but that's a personal opinion,
I guess.

I agree that this might introduce a maintainance overhead when it comes to
sync'ing the code and the documentation but, on the other hand, printk.h isn't
something that changes too frequently, is it?

Best regards,
Ricardo