On 1/26/23 20:50, Ira Weiny wrote:
Fabio M. De Francesco wrote:
FWIW I think I would simplify the subject
[PATCH] mm/highmem: Fix kunmap_local() on flush on unmap architectures
Or something like that.
If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
address (as confirmed by Matthew Wilcox and Helge Deller). Al Viro
proposed to call kunmap_flush_on_unmap() on an aligned-down to page
address in order to fix this issue. Consensus has been reached on this
solution.
Therefore, if ARCH_HAS_FLUSH_ON_KUNMAP is defined, call
kunmap_flush_on_unmap() on an aligned-down to page address computed with
the PTR_ALIGN_DOWN() macro.
Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Confirmed-by: Helge Deller <deller@xxxxxx>
Confirmed-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
---
I have (at least) two problems with this patch...
1) checkpatch.pl complains about the use of the non-standard
"Confirmed-by" tags. I don't know how else I can give credit to Helge
and Matthew. However, this is not the first time that I see non-standard
tags in patches applied upstream (I too had a non-standard
"Analysed-by" tag in patch which fixes a SAC bug). Any objections?
I think you can add Matthew and Helge as Suggested-by: All 3 had input on
the solution.
2) I'm not sure whether or not the "Fixes" tag is appropriate in this
patch. Can someone either confirm or deny it?
This 'fixes' looks correct to me. I don't know how many folks are running
highmem with parisc but if they are I am sure they would appreciate the
extra knowledge.
It seems nobody is running highmem on parisc, because it can't be enabled.
AFAICS, it's not in any parisc related Kconfig file.
I do wonder if this should be cc'ed to stable to ensure it gets
backported? Helge do you think there is a need for that?
For correctness I think it's nevertheless good to backport it.
include/linux/highmem-internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index 034b1106d022..e247c9ac4583 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -200,7 +200,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
static inline void __kunmap_local(const void *addr)
{
#ifdef ARCH_HAS_FLUSH_ON_KUNMAP
- kunmap_flush_on_unmap(addr);
+ kunmap_flush_on_unmap(PTR_ALIGN_DOWN(addr, PAGE_SIZE));
#endif
}
That would be another possibility:
diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index 0bdee6724132..ce5d1f8a23bd 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -77,6 +77,7 @@ void flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned lon
#define ARCH_HAS_FLUSH_ON_KUNMAP
static inline void kunmap_flush_on_unmap(const void *addr)
{
+ addr = PTR_ALIGN_DOWN(addr, PAGE_SIZE);
flush_kernel_dcache_page_addr(addr);
}
Helge
