`scripts/kernel-doc -v -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.h:204: warning: No description found for return value of 'alloc_zeroed_user_highpage_movable'
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) moving the kernel-doc comments from highmem.h to
highmem-internal.h (which is the file were the kunmap_atomic() macro is
actually defined), (2) extending and merging it with the comment which was
already in highmem-internal.h, and (3) using correct parameter names.
Cc: Mike Rapoport <[email protected]>
Cc: Ira Weiny <[email protected]>
Suggested-by: Matthew Wilcox <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
include/linux/highmem-internal.h | 14 +++++++++++---
include/linux/highmem.h | 15 +++------------
2 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a77be5630209..b099a08e29d3 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
+ *
+ * Unmap an address previously mapped by kmap_atomic() and re-enables
+ * pagefaults and preemption. 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 it. The compiler will warn you if you pass the page.
*/
#define kunmap_atomic(__addr) \
do { \
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 39bb9b47fa9c..c3d562b5f0c1 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);
@@ -191,6 +180,8 @@ static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
* @vma: The VMA the page is to be allocated for
* @vaddr: The virtual address the page will be inserted into
*
+ * Returns: The allocated and zeroed HIGHMEM page
+ *
* This function will allocate a page for a VMA that the caller knows will
* be able to migrate in the future using move_pages() or reclaimed
*
--
2.34.1
On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> `scripts/kernel-doc -v -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.h:204: warning: No description found for return value of 'alloc_zeroed_user_highpage_movable'
> 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) moving the kernel-doc comments from highmem.h to
> highmem-internal.h (which is the file were the kunmap_atomic() macro is
> actually defined), (2) extending and merging it with the comment which was
> already in highmem-internal.h, and (3) using correct parameter names.
>
> Cc: Mike Rapoport <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Suggested-by: Matthew Wilcox <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> include/linux/highmem-internal.h | 14 +++++++++++---
> include/linux/highmem.h | 15 +++------------
> 2 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index a77be5630209..b099a08e29d3 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
> + *
> + * Unmap an address previously mapped by kmap_atomic() and re-enables
Unmap ... and re-enable
or
Unmaps ... and re-enables
Other than that
Acked-by: Mike Rapoport <[email protected]>
> + * pagefaults and preemption. 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 it. The compiler will warn you if you pass the page.
> */
> #define kunmap_atomic(__addr) \
> do { \
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 39bb9b47fa9c..c3d562b5f0c1 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);
> @@ -191,6 +180,8 @@ static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
> * @vma: The VMA the page is to be allocated for
> * @vaddr: The virtual address the page will be inserted into
> *
> + * Returns: The allocated and zeroed HIGHMEM page
> + *
> * This function will allocate a page for a VMA that the caller knows will
> * be able to migrate in the future using move_pages() or reclaimed
> *
> --
> 2.34.1
>
>
--
Sincerely yours,
Mike.
On venerdì 22 aprile 2022 10:24:14 CEST Mike Rapoport wrote:
> On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> > `scripts/kernel-doc -v -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.h:204: warning: No description found for return
value of 'alloc_zeroed_user_highpage_movable'
> > 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) moving the kernel-doc comments from highmem.h
to
> > highmem-internal.h (which is the file were the kunmap_atomic() macro is
> > actually defined), (2) extending and merging it with the comment which
was
> > already in highmem-internal.h, and (3) using correct parameter names.
> >
> > Cc: Mike Rapoport <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Suggested-by: Matthew Wilcox <[email protected]>
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> > include/linux/highmem-internal.h | 14 +++++++++++---
> > include/linux/highmem.h | 15 +++------------
> > 2 files changed, 14 insertions(+), 15 deletions(-)
> >
> > [...]
> >
> > + *
> > + * Unmap an address previously mapped by kmap_atomic() and re-enables
>
> Unmap ... and re-enable
>
> or
>
> Unmaps ... and re-enables
Sorry, I should have read it twice before submitting :(
This entire series has already been taken by Andrew Morton for "-mm"
immediately after submission. I think that probably the better suited
solution is to send a correction when they show upstream.
Do you agree with me or you prefer that I resubmit the whole series as a v2
now?
>
> Other than that
>
> Acked-by: Mike Rapoport <[email protected]>
>
I also saw your "Acked-by" tag in patch 2/4. Thanks!
Regards,
Fabio
On Fri, Apr 22, 2022 at 11:36:28AM +0200, Fabio M. De Francesco wrote:
> On venerd? 22 aprile 2022 10:24:14 CEST Mike Rapoport wrote:
> > On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> > > `scripts/kernel-doc -v -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.h:204: warning: No description found for return
> value of 'alloc_zeroed_user_highpage_movable'
> > > 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) moving the kernel-doc comments from highmem.h
> to
> > > highmem-internal.h (which is the file were the kunmap_atomic() macro is
> > > actually defined), (2) extending and merging it with the comment which
> was
> > > already in highmem-internal.h, and (3) using correct parameter names.
> > >
> > > Cc: Mike Rapoport <[email protected]>
> > > Cc: Ira Weiny <[email protected]>
> > > Suggested-by: Matthew Wilcox <[email protected]>
> > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > ---
> > > include/linux/highmem-internal.h | 14 +++++++++++---
> > > include/linux/highmem.h | 15 +++------------
> > > 2 files changed, 14 insertions(+), 15 deletions(-)
> > >
> > > [...]
> > >
> > > + *
> > > + * Unmap an address previously mapped by kmap_atomic() and re-enables
> >
> > Unmap ... and re-enable
> >
> > or
> >
> > Unmaps ... and re-enables
>
> Sorry, I should have read it twice before submitting :(
>
> This entire series has already been taken by Andrew Morton for "-mm"
> immediately after submission. I think that probably the better suited
> solution is to send a correction when they show upstream.
You can send a correction as an incremental patch against mmotm tree that's
mirrored here:
https://github.com/hnaz/linux-mm
I believe Andrew will add it to his mmotm queue.
> Do you agree with me or you prefer that I resubmit the whole series as a v2
> now?
>
> >
> > Other than that
> >
> > Acked-by: Mike Rapoport <[email protected]>
> >
>
> I also saw your "Acked-by" tag in patch 2/4. Thanks!
>
> Regards,
>
> Fabio
>
>
--
Sincerely yours,
Mike.
On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
> `scripts/kernel-doc -v -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.h:204: warning: No description found for return value of 'alloc_zeroed_user_highpage_movable'
> 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) moving the kernel-doc comments from highmem.h to
> highmem-internal.h (which is the file were the kunmap_atomic() macro is
> actually defined), (2) extending and merging it with the comment which was
> already in highmem-internal.h, and (3) using correct parameter names.
>
> Cc: Mike Rapoport <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Suggested-by: Matthew Wilcox <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> include/linux/highmem-internal.h | 14 +++++++++++---
> include/linux/highmem.h | 15 +++------------
> 2 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index a77be5630209..b099a08e29d3 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
> + *
> + * Unmap an address previously mapped by kmap_atomic() and re-enables
> + * pagefaults and preemption. 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 it. The compiler will warn you if you pass the page.
> */
> #define kunmap_atomic(__addr) \
> do { \
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 39bb9b47fa9c..c3d562b5f0c1 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
^^^^^^^^^^^^^^^
Page
Not only was the name wrong but the description of an address is wrong.
Other than that LGTM:
Reviewed-by: Ira Weiny <[email protected]>
> *
> * 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);
> @@ -191,6 +180,8 @@ static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
> * @vma: The VMA the page is to be allocated for
> * @vaddr: The virtual address the page will be inserted into
> *
> + * Returns: The allocated and zeroed HIGHMEM page
> + *
> * This function will allocate a page for a VMA that the caller knows will
> * be able to migrate in the future using move_pages() or reclaimed
> *
> --
> 2.34.1
>
(I don't know why it looks like my previous reply has not been sent and it
also disappeared from drafts. I apologize if for whatever reason you
receive this message twice)
On venerdì 22 aprile 2022 20:08:46 CEST Ira Weiny wrote:
> On Thu, Apr 21, 2022 at 08:01:57PM +0200, Fabio M. De Francesco wrote:
>
> [snip]
>
> > diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> > index 39bb9b47fa9c..c3d562b5f0c1 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
> ^^^^^^^^^^^^^^^
> Page
>
> Not only was the name wrong but the description of an address is wrong.
Yes, correct. I'll rewrite this line the following way and send a v2 of
this series while addressing also what it is required by your review of 3/4
(unless you have better suited suggestions):
- * @addr: Virtual address to be unmapped
+ * @page: Pointer to the page which was mapped by kmap()
> Other than that LGTM:
>
> Reviewed-by: Ira Weiny <[email protected]>
Thanks!
Fabio