`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 <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
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
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.
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
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
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
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
>
>
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 <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> 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