Date:   Thu, 26 Jan 2023 11:50:19 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ira Weiny <ira.weiny@intel.com>,
        "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Tony Luck <tony.luck@intel.com>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        David Sterba <dsterba@suse.com>,
        Kees Cook <keescook@chromium.org>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>
CC:     Thomas Gleixner <tglx@linutronix.de>,
        Al Viro <viro@zeniv.linux.org.uk>, Helge Deller <deller@gmx.de>
Subject: Re: [PATCH] mm/highmem: Align-down to page the address for
 kunmap_flush_on_unmap()
Message-ID: <63d2d97bce4c7_63e3f29442@iweiny-mobl.notmuch>
References: <20230126143346.12086-1-fmdefrancesco@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230126143346.12086-1-fmdefrancesco@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?83sl1xzS14Ja3zRPOGtcQ8D7etyzQOJruGGXOG3Z/MFMV6CvvRK5FY5l9FyW?=
 =?us-ascii?Q?kaL/xF2OHx7SbtGxApteUWoeIrOBHpizL9eWMkVk5TxHdiOoTgbccZTxB/pb?=
 =?us-ascii?Q?sahliJTzeFaRoIwIJXXfmB51ynZrLsT1V0jEhVeTXlN3IRLMAVs8ynmw8Z50?=
 =?us-ascii?Q?XSLu7XwcFuvpjA078dxFG1WbMStM+G68V3mBARACjrg6rRzzkRIj9NrKl5ZM?=
 =?us-ascii?Q?vwfygBYYIyYejCkbk6dRa2vi6kfYzy9i7H2xnXYJAn+Zb0vq6eGGIHbyFFo6?=
 =?us-ascii?Q?M11FLOaD+P05m1cP6/MC9TcpcTb0x4bXlSin1w4dOkEVSfbW0FG15FjKyNL1?=
 =?us-ascii?Q?Bqx2aQ9iV+slXT2j0564HbUQQppw5cre73ypjjka+1IbIp8teZdYYmjkZ7TT?=
 =?us-ascii?Q?8GkAU4mPqXyHT8XpUw95diKpRl9Kq8NISKzpkS4pgjRmBQH57gxLdsU39e+V?=
 =?us-ascii?Q?zBf5ypvX0Av10g3yi+ZbQ69XbU6QI19rQth+jcnUtCuLH4inGoHP4F/CbXuM?=
 =?us-ascii?Q?r4fj5tJaMJiOGLwXhohQTBQoABXOn/hljxkoabGOlCfuDZi3MGMyrLeBL5hY?=
 =?us-ascii?Q?xlucPc9/csHMeAgHxLJFykpcvyXAxp+Uyb4lIZT2iC79PWQYrbimWEbdUqqi?=
 =?us-ascii?Q?/bGbQxlH74qCDXAE7uRPGTAkH8XM7tanjgrp2rRQTsJlj9vt8GnLg+43Ml1B?=
 =?us-ascii?Q?w2/YzAB6lSI8iTYXUtO5IhHKrGo6Jq2ZiH+aQGSf2cJx5BdmjupjNJ/l3ytN?=
 =?us-ascii?Q?FrZk/oeb29dNevTIyUnD9o94Uv/dvCQGIga3/h6mLPo0/uVaZBE2teT54rhB?=
 =?us-ascii?Q?/r3KEjBRGoGVBx6csTj2fXJR48L+jaNvcryXZNMsSqspprVa9kcV2JUBEBcf?=
 =?us-ascii?Q?QYs7e7wM77xasjSw/Y8fGkNe9I9Q0fGoTKHPfsmi1G5wwAl1PO6Q6s5TRyC9?=
 =?us-ascii?Q?iU2nPM0mn01C8x0RgIR4sY/RuMyYC2OOVwnWuZleINbp6bR4zFpl0eW+5ioY?=
 =?us-ascii?Q?c25f/lgatv4xUfXTmowH5WsPkjpSMsiqD4yE+CYcX1Q5f20JArxYyamnFMIO?=
 =?us-ascii?Q?8pgjcLKmK7ygNFxNmpnMFIPygvTMZdtj564MkJZxA8udo7aB4WmkKmszF9Uc?=
 =?us-ascii?Q?NoXqArFCBJWUofCf4lN4jYRDHn3YUw15eb9uWhn6A1XCz2lNRmvKSqLLJq36?=
 =?us-ascii?Q?L0IIf7lnkaChD5gQdT4EP9C+JIgQbkBg/PYfXOeTsuN+wWHo/yX0AS4yzW7r?=
 =?us-ascii?Q?CIFm/toIzS/mwubb5rLLzE8EZKEmKdwVRkTISz1IYU1+SDX5WcyOl8W1Gj6F?=
 =?us-ascii?Q?rC8PsOyhtUyiOTHt0j96A7hXumaA6+qu8+ueQuz/DjU8F6ZJs9D2WmSX7IBx?=
 =?us-ascii?Q?HDvbTDOejGUggZNDKLaz7QDSOhLWPHR84hoLRzBgeTFbU0vldcERn/zWuZqP?=
 =?us-ascii?Q?znNmA9LjaLKEDa7/Yl7jDdHHIvM2bfOjkgFLNoHtgF9gcJmzGnDVqDKT3M1C?=
 =?us-ascii?Q?GXyn4bB5ec1nVdBjo+EoFEuYQhkY++K+vFpWRYu4n+e85XgZ1fSljuYoH46d?=
 =?us-ascii?Q?Qm9QrswjT+VjRilGS+T/66gY7ztaM5naa5pcqHhS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 89fb8c82-7b32-4204-47a7-08daffd69081
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2023 19:50:24.0852
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hSQR1i6GHlAvDk6hnOQ3TUJCxeB4ItOj5u79s9qmXG/VBQudG/Nxf/Y4FqDtQt2L6BzDboSR3wh6t7ai9D5nmQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6824
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

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?

Ira

> 
>  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
>  }
>  
> -- 
> 2.39.0
> 
>