lkml.org 
[lkml]   [2023]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH] mm/highmem: Align-down to page the address for kunmap_flush_on_unmap()
    From
    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

    \
     
     \ /
      Last update: 2023-03-26 23:57    [W:3.545 / U:0.484 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site