Messages in this thread Patch in this message | | | Date | Thu, 26 Jan 2023 21:37:08 +0100 | Subject | Re: [PATCH] mm/highmem: Align-down to page the address for kunmap_flush_on_unmap() | From | Helge Deller <> |
| |
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@intel.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Suggested-by: Al Viro <viro@zeniv.linux.org.uk> >> Confirmed-by: Helge Deller <deller@gmx.de> >> Confirmed-by: Matthew Wilcox <willy@infradead.org> >> Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") >> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> >> --- >> >> 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
| |