[PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

`scripts/kernel-doc -none include/linux/highmem*` reports the following
warnings:

include/linux/highmem.h:160: warning: expecting prototype for kunmap_atomic(). Prototype was for nr_free_highpages() instead
include/linux/highmem-internal.h:256: warning: Function parameter or member '__addr' not described in 'kunmap_atomic'
include/linux/highmem-internal.h:256: warning: Excess function parameter 'addr' description in 'kunmap_atomic'

Fix these warnings by (1) move the kernel-doc comments from highmem.h to
highmem-internal.h (which is the file were the kunmap_atomic() macro is
actually defined), (2) merge it with the comment which already was in
highmem-internal.h, and (3) use correct parameter names.

Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

v1->v2: Re-word the last sentence of the commit message and add a missing
	number to the second entry in the fixes list. Add Mike Rapoport's
	"Acked-by:" tag (thanks!).

 include/linux/highmem-internal.h | 14 +++++++++++---
 include/linux/highmem.h          | 13 +------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a77be5630209..7307de391288 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -236,9 +236,17 @@ static inline unsigned long totalhigh_pages(void) { return 0UL; }
 
 #endif /* CONFIG_HIGHMEM */
 
-/*
- * Prevent people trying to call kunmap_atomic() as if it were kunmap()
- * kunmap_atomic() should get the return value of kmap_atomic, not the page.
+/**
+ * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
+ * @__addr:       Virtual address to be unmapped
+ *
+ * Counterpart to kmap_atomic().
+ *
+ * Effectively a wrapper around kunmap_local() which additionally undoes
+ * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
+ * preemption. Prevent people trying to call kunmap_atomic() as if it
+ * were kunmap() because kunmap_atomic() should get the return value of
+ * kmap_atomic(), not its argument which is a pointer to struct page.
  */
 #define kunmap_atomic(__addr)					\
 do {								\
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 39bb9b47fa9c..0a7a89721e5d 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -37,7 +37,7 @@ static inline void *kmap(struct page *page);
 
 /**
  * kunmap - Unmap the virtual address mapped by kmap()
- * @addr:	Virtual address to be unmapped
+ * @page:	Virtual address to be unmapped
  *
  * Counterpart to kmap(). A NOOP for CONFIG_HIGHMEM=n and for mappings of
  * pages in the low memory area.
@@ -145,17 +145,6 @@ static inline void *kmap_local_folio(struct folio *folio, size_t offset);
  */
 static inline void *kmap_atomic(struct page *page);
 
-/**
- * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
- * @addr:	Virtual address to be unmapped
- *
- * Counterpart to kmap_atomic().
- *
- * Effectively a wrapper around kunmap_local() which additionally undoes
- * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
- * preemption.
- */
-
 /* Highmem related interfaces for management code */
 static inline unsigned int nr_free_highpages(void);
 static inline unsigned long totalhigh_pages(void);
-- 
2.34.1

Re: [PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Matthew Wilcox]

On Mon, Apr 18, 2022 at 07:56:38PM +0200, Fabio M. De Francesco wrote:
> +/**
> + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> + * @__addr:       Virtual address to be unmapped
> + *
> + * Counterpart to kmap_atomic().

I don't think this is a terribly useful paragraph?

> + * Effectively a wrapper around kunmap_local() which additionally undoes
> + * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
> + * preemption. Prevent people trying to call kunmap_atomic() as if it
> + * were kunmap() because kunmap_atomic() should get the return value of
> + * kmap_atomic(), not its argument which is a pointer to struct page.

I'd rather this were useful advice to the caller than documentation of
how it works.  How about:

 * Unmap an address previously mapped by kmap_atomic().  Mappings
 * should be unmapped in the reverse order that they were mapped.
 * See kmap_local_page() for details.  @__addr can be any address within
 * the mapped page, so there is no need to subtract any offset that has
 * been added.  In contrast to kunmap(), this function takes the address
 * returned from kmap_atomic(), not the page passed to kmap_atomic().
 * The compiler will warn you if you pass the page.

Re: [PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

On martedì 19 aprile 2022 14:44:14 CEST Matthew Wilcox wrote:
> On Mon, Apr 18, 2022 at 07:56:38PM +0200, Fabio M. De Francesco wrote:
> > +/**
> > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> > + * @__addr:       Virtual address to be unmapped
> > + *
> > + * Counterpart to kmap_atomic().
> 
> I don't think this is a terribly useful paragraph?

I agree but let me remind you that this patch is _only_ about fixing 
kernel-doc warnings. This warning was simply fixed by moving kdoc comment 
from highmem.h to highmem-internal.h (which is the file where the 
definition of kunmap_atomic() resides) and merging the text with few lines 
that already were in highmem-internal.h.

Furthermore, I've already had an "Acked-by:" tag from Mike Rapoport. I 
suppose that if I changed the paragraph here I could not forward his ack to 
the next version.

> > + * Effectively a wrapper around kunmap_local() which additionally 
undoes
> > + * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
> > + * preemption. Prevent people trying to call kunmap_atomic() as if it
> > + * were kunmap() because kunmap_atomic() should get the return value 
of
> > + * kmap_atomic(), not its argument which is a pointer to struct page.
> 
> I'd rather this were useful advice to the caller than documentation of
> how it works.  How about:
> 
>  * Unmap an address previously mapped by kmap_atomic().  Mappings
>  * should be unmapped in the reverse order that they were mapped.
>  * See kmap_local_page() for details.  @__addr can be any address within
>  * the mapped page, so there is no need to subtract any offset that has
>  * been added.  In contrast to kunmap(), this function takes the address
>  * returned from kmap_atomic(), not the page passed to kmap_atomic().
>  * The compiler will warn you if you pass the page.

A change like this should go to a separate patch and indeed I'll send it 
ASAP. Probably, when I'll rework this text in a separate patch, I'll also 
copy-paste the paragraph you wrote as-is (too easy!).

However, since the rework of the text in paragraph can only be applied on 
top of this patch, I'm not sure if I should either (1) make a series with 
two patches or (2) make a separate patch with a warning to Maintainers that 
the changes in the new patch can only be applied on top of this patch.

Actually, I don't yet know how the Community wants tasks like these to be 
carried out. Any suggestion?

Thanks for your review and for suggesting a better suited text for the next 
patch.

Fabio M. De Francesco

Re: [PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Ira Weiny]

On Tue, Apr 19, 2022 at 03:25:16PM +0200, Fabio M. De Francesco wrote:
> On martedì 19 aprile 2022 14:44:14 CEST Matthew Wilcox wrote:
> > On Mon, Apr 18, 2022 at 07:56:38PM +0200, Fabio M. De Francesco wrote:
> > > +/**
> > > + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> > > + * @__addr:       Virtual address to be unmapped
> > > + *
> > > + * Counterpart to kmap_atomic().
> > 
> > I don't think this is a terribly useful paragraph?
> 
> I agree but let me remind you that this patch is _only_ about fixing 
> kernel-doc warnings. This warning was simply fixed by moving kdoc comment 
> from highmem.h to highmem-internal.h (which is the file where the 
> definition of kunmap_atomic() resides) and merging the text with few lines 
> that already were in highmem-internal.h.
> 
> Furthermore, I've already had an "Acked-by:" tag from Mike Rapoport. I 
> suppose that if I changed the paragraph here I could not forward his ack to 
> the next version.

No drop the ack for now.

> 
> > > + * Effectively a wrapper around kunmap_local() which additionally 
> undoes
> > > + * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
> > > + * preemption. Prevent people trying to call kunmap_atomic() as if it
> > > + * were kunmap() because kunmap_atomic() should get the return value 
> of
> > > + * kmap_atomic(), not its argument which is a pointer to struct page.
> > 
> > I'd rather this were useful advice to the caller than documentation of
> > how it works.  How about:
> > 
> >  * Unmap an address previously mapped by kmap_atomic().  Mappings
> >  * should be unmapped in the reverse order that they were mapped.
> >  * See kmap_local_page() for details.  @__addr can be any address within
> >  * the mapped page, so there is no need to subtract any offset that has
> >  * been added.  In contrast to kunmap(), this function takes the address
> >  * returned from kmap_atomic(), not the page passed to kmap_atomic().
> >  * The compiler will warn you if you pass the page.
> 
> A change like this should go to a separate patch and indeed I'll send it 
> ASAP. Probably, when I'll rework this text in a separate patch, I'll also 
> copy-paste the paragraph you wrote as-is (too easy!).
> 
> However, since the rework of the text in paragraph can only be applied on 
> top of this patch, I'm not sure if I should either (1) make a series with 
> two patches or (2) make a separate patch with a warning to Maintainers that 
> the changes in the new patch can only be applied on top of this patch.
> 
> Actually, I don't yet know how the Community wants tasks like these to be 
> carried out. Any suggestion?
> 
> Thanks for your review and for suggesting a better suited text for the next 
> patch.

I think you should gather all these patches together into a series and submit.
If I am following correctly there are at least 4 patches now?  But I'm unsure
of which ones are stand alone.

It would be easier to see what changes are being made along the way as well as
the final result of the fixes.

Ira

> 
> Fabio M. De Francesco
> 
> 

Re: [PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

On martedì 19 aprile 2022 16:52:22 CEST Ira Weiny wrote:
> On Tue, Apr 19, 2022 at 03:25:16PM +0200, Fabio M. De Francesco wrote:
>
> [snip]
> 
> I think you should gather all these patches together into a series and 
submit.
> If I am following correctly there are at least 4 patches now?  But I'm 
unsure
> of which ones are stand alone.
> 
> It would be easier to see what changes are being made along the way as 
well as
> the final result of the fixes.
> 
> Ira

Yes, this is perfectly reasonable. There are only three patches (this patch 
plus two more in a series). Since they are all related in some way, it is 
best to bring them together into one series which I will rework taking into 
account both yours and Matthew's suggestions.

Thanks,

Fabio

Re: [PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

On martedì 19 aprile 2022 17:09:04 CEST Fabio M. De Francesco wrote:
> On martedì 19 aprile 2022 16:52:22 CEST Ira Weiny wrote:
> > On Tue, Apr 19, 2022 at 03:25:16PM +0200, Fabio M. De Francesco wrote:
> >
> > [snip]
> > 
> > I think you should gather all these patches together into a series and 
> submit.
> > If I am following correctly there are at least 4 patches now?  But I'm 
> unsure
> > of which ones are stand alone.
> > 
> > It would be easier to see what changes are being made along the way as 
> well as
> > the final result of the fixes.
> > 
> > Ira
> 
> Yes, this is perfectly reasonable. There are only three patches (this 
patch 
> plus two more in a series). 

No, you are right. There are 4 patches in my queue for Highmem related 
files. I think I'm at a point where I'm starting to lose sight of the tasks 
due :(

> Since they are all related in some way, it is 
> best to bring them together into one series which I will rework taking 
into 
> account both yours and Matthew's suggestions.

The collection of all these patches in one series is confirmed for all 4.

Thanks,

Fabio

Re: [PATCH v2] mm/highmem: Fix kernel-doc warnings in highmem*.h

[Fabio M. De Francesco]

On lunedì 18 aprile 2022 19:56:38 CEST Fabio M. De Francesco wrote:
> `scripts/kernel-doc -none include/linux/highmem*` reports the following
> warnings:
> 
> include/linux/highmem.h:160: warning: expecting prototype for 
kunmap_atomic(). Prototype was for nr_free_highpages() instead
> include/linux/highmem-internal.h:256: warning: Function parameter or 
member '__addr' not described in 'kunmap_atomic'
> include/linux/highmem-internal.h:256: warning: Excess function parameter 
'addr' description in 'kunmap_atomic'
> 
> Fix these warnings by (1) move the kernel-doc comments from highmem.h to
> highmem-internal.h (which is the file were the kunmap_atomic() macro is
> actually defined), (2) merge it with the comment which already was in
> highmem-internal.h, and (3) use correct parameter names.
> 
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> v1->v2: Re-word the last sentence of the commit message and add a missing
> 	number to the second entry in the fixes list. Add Mike Rapoport's
> 	"Acked-by:" tag (thanks!).
> 
>  include/linux/highmem-internal.h | 14 +++++++++++---
>  include/linux/highmem.h          | 13 +------------
>  2 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-
internal.h
> index a77be5630209..7307de391288 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -236,9 +236,17 @@ static inline unsigned long totalhigh_pages(void) { 
return 0UL; }
>  
>  #endif /* CONFIG_HIGHMEM */
>  
> -/*
> - * Prevent people trying to call kunmap_atomic() as if it were kunmap()
> - * kunmap_atomic() should get the return value of kmap_atomic, not the 
page.
> +/**
> + * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> + * @__addr:       Virtual address to be unmapped
> + *
> + * Counterpart to kmap_atomic().
> + *
> + * Effectively a wrapper around kunmap_local() which additionally undoes
> + * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
> + * preemption. Prevent people trying to call kunmap_atomic() as if it
> + * were kunmap() because kunmap_atomic() should get the return value of
> + * kmap_atomic(), not its argument which is a pointer to struct page.
>   */
>  #define kunmap_atomic(__addr)					
\
>  do {								
\
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 39bb9b47fa9c..0a7a89721e5d 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -37,7 +37,7 @@ static inline void *kmap(struct page *page);
>  
>  /**
>   * kunmap - Unmap the virtual address mapped by kmap()
> - * @addr:	Virtual address to be unmapped
> + * @page:	Virtual address to be unmapped
>   *
>   * Counterpart to kmap(). A NOOP for CONFIG_HIGHMEM=n and for mappings 
of
>   * pages in the low memory area.
> @@ -145,17 +145,6 @@ static inline void *kmap_local_folio(struct folio 
*folio, size_t offset);
>   */
>  static inline void *kmap_atomic(struct page *page);
>  
> -/**
> - * kunmap_atomic - Unmap the virtual address mapped by kmap_atomic()
> - * @addr:	Virtual address to be unmapped
> - *
> - * Counterpart to kmap_atomic().
> - *
> - * Effectively a wrapper around kunmap_local() which additionally undoes
> - * the side effects of kmap_atomic(), i.e. reenabling pagefaults and
> - * preemption.
> - */
> -
>  /* Highmem related interfaces for management code */
>  static inline unsigned int nr_free_highpages(void);
>  static inline unsigned long totalhigh_pages(void);
> -- 
> 2.34.1
> 
Dear Maintainers,

Please drop this patch because it has been reworked and inserted as part of 
a new series whose subject is "Extend and reorganize Highmem's 
documentation" and which is about to be submitted.

Thanks,

Fabio M. De Francesco