Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 14 Jun 2022 07:39:57 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Kees Cook <keescook@chromium.org>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        <linux-kernel@vger.kernel.org>, David Sterba <dsterba@suse.cz>
Subject: Re: [PATCH] highmem: Make __kunmap_{local,atomic}() take "const void
 *"
Message-ID: <YqidvW4Yw6p5Bcmv@iweiny-desk3>
References: <20220614142531.16478-1-fmdefrancesco@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220614142531.16478-1-fmdefrancesco@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?L/ST6vGuph7aRTUdYv3I+66wMOQx+o8eL2z2gn0jDEpPrVV6NhkLSisubKIj?=
 =?us-ascii?Q?br8Gc7T1uFgcoHD+fzspTGJtIczlqGe74KiWJVTYNDzT1etA1DEnZqnpvoKA?=
 =?us-ascii?Q?SEJ5KLJjFY70KnyWOTKJTf7w3JGPofKb/JVOz61Qyr1WIfIdU6Zk91E35sCy?=
 =?us-ascii?Q?US/VmIPsN1bd975J0cm4Sj0ZRp8UXgnr/LkHs6ONx4y6CQ52zn6yx4tTviAT?=
 =?us-ascii?Q?dHL+CZOcuBtbQ0oGE9FolAAaeE443Rll0wtp1t5ahNf67iaS402n5AbBCx++?=
 =?us-ascii?Q?6cORZLpvbLl0IYsD5am7i8fRhJoQRrzpq9zkiO6f1PWPeqd+bZExiHt2P3e+?=
 =?us-ascii?Q?+eoMC6RqgrfwKXDFcrqfNk4snkLQt90aRvKDhUg6jlXoEZuHG4cNwpLktqMm?=
 =?us-ascii?Q?JrLkE/V4s4qMOs1csQLQioZiM67AEEI2//AxHwSDFxeHzOD//Ob6gmWObteX?=
 =?us-ascii?Q?we+MPp66hjHs9QIgeS0zf9ZK4UzATDwkJePq+bmPIfdAojICNGFzeJd54A4f?=
 =?us-ascii?Q?ZEQI1+PWWDj7X92aYjuIRmu9K4cVZV3aaoQ1fmtY4Br/rdK/6k3enD7/cYr4?=
 =?us-ascii?Q?WtoSUQAKL0GDALsfV/kQSi1oZIwOpHn2V+kAYG4TIeBZ/r1EcM6JX1ZY8beL?=
 =?us-ascii?Q?07eCdHYT4Dq8IN1XX7mtI7jxpeSU7l/F9BHT2F8Pko88eKKbCctuhb8DlFi5?=
 =?us-ascii?Q?S+d1kHZlq4toWVPrLtUX+QWPYX1KheZqZM+19lm20WdUsBTSv6ZXwhPrmfTD?=
 =?us-ascii?Q?T8BgAZXWGEE9Lon97WbDuIbPN1Ye1gfFEa3arRgrfg+5HqxQUf3FJEGAU17I?=
 =?us-ascii?Q?I+Rb9V+zzLXID3R96UCkijcowoxQocgTzKUbFu7PJSTDXdEnFzwYoBLCBMQk?=
 =?us-ascii?Q?b1vx5CdymPDGNdJfUHALkDuekKJos7w0OOO0f+wcfmVoUZcrTYnTMDMbMXvE?=
 =?us-ascii?Q?fWKmPJPZiw2EaViDgZRWsQcq+SlZdY9uX1C/aqKpWSn3wFlk2ClchrVGOqvJ?=
 =?us-ascii?Q?JXlHur55iSkYWL6/SREwtR1+BrClNYIMhoT5leGaOKavQ9NckAfNk/mDldX6?=
 =?us-ascii?Q?hHtvzvAblICLUaXOUKZ19WD+7yvTWuc+pRzm1A7QsfekK2Jm7bi0/zsLAnK0?=
 =?us-ascii?Q?g5gk5KBJZFjGbvWo2xdOa492jk/6zQ3Czw38QxbzoHaFYifBlxqA5egGBOnq?=
 =?us-ascii?Q?/40D3cLs5Iqkb5hShG5AhHGS3gslUNgdWTEwo9VYxBWx4xJDJY6QPEeuOXce?=
 =?us-ascii?Q?6JNKnlGf3srSd/SjH7Zwo81gu1Q3tgk1lOrHTVeUe9zMAUbtS5n17qlMZfJ4?=
 =?us-ascii?Q?ogOccMp4deQYH4Aa7L4QAt/9ke4fwg7Fy5dNpNwDd8d8gE+ttKwinDOJ7fEB?=
 =?us-ascii?Q?OVokvY3w41esnNQwwbPpGZjkuqqCwSKBpMvq1iVbgFjrEy24Ix8ZfnJg8fCo?=
 =?us-ascii?Q?Nc+gflDIFbFks/uZuEr39CN4mj5pbt9Aj5zYDKtqfFy3iVIvpi9YXUvazrBx?=
 =?us-ascii?Q?EyIJZ0s5WguEYH3zL1waMrzUF/lL0QWjx50/13WfCpfNHz0TR2Mc0P8kMw/M?=
 =?us-ascii?Q?oiqZreO26O//jv/ZjZ20iFsprU9kVYl9w4Jd2BGbwCC3eVqzdcYEtBDa/3b2?=
 =?us-ascii?Q?RJKkvbu++vpZ6FinEpZpXltIoK+EkwUi91nrJCDAKS1Pl36yCJN0RVDLgyQD?=
 =?us-ascii?Q?rvfsxqiUlUixOhG4DuM5cKfUypVjXORNvjTWiOK1i2LeKgNNolcvBtWvzZgZ?=
 =?us-ascii?Q?t6uXGxvyJcC3Ta263x/OSJt8TUuWMgA=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 61986fba-6451-40ac-f4b1-08da4e13c639
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2022 14:40:06.4472
 (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: ivzeEdN1mrhkWekusENHjMpVHScByu1cOvRVxk8dLCnt3BUaHx3O9TrhC1OpS1qA9DGWAiquSkH3nQ2cKJ0oew==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB5900
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jun 14, 2022 at 04:25:31PM +0200, Fabio M. De Francesco wrote:
> Sometimes __kunmap_{local,atomic}() should take pointers to const void.
> Currently their prototypes take pointers to void, therefore compilations
> break in cases like the above-mentioned.

I think the compilation problem is just a symptom of the real issue.
kunmap_local() has no reason to change the vaddr parameter and therefore it is
correct to declare it a const.

This allows callers to also maintain vaddr as a const value to be passed back
to kumap_local() if they wish.

Ira

> 
> This is the output of GCC-12 when it encounters one those cases:
> 
> ./linux/fs/btrfs/zstd.c:547:33: warning: passing
> argument 1 of '__kunmap_local' discards 'const' qualifier from pointer
> target type [-Wdiscarded-qualifiers]
>   547 |   kunmap_local(workspace->in_buf.src);
>       |                ~~~~~~~~~~~~~~~~~^~~~
> ./linux/include/linux/highmem-internal.h:284:17:
> note: in definition of macro 'kunmap_local'
>   284 |  __kunmap_local(__addr);     \
>       |                 ^~~~~~
> ./linux/include/linux/highmem-internal.h:92:41:
> note: expected 'void *' but argument is of type 'const void *'
>    92 | static inline void __kunmap_local(void *vaddr)
>       |                                   ~~~~~~^~~~~
> 
> Make __kunmap_{local,atomic}() take pointers to const void.
> 
> Suggested-by: David Sterba <dsterba@suse.cz>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  include/linux/highmem-internal.h | 10 +++++-----
>  mm/highmem.c                     |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index cddb42ff0473..034b1106d022 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -8,7 +8,7 @@
>  #ifdef CONFIG_KMAP_LOCAL
>  void *__kmap_local_pfn_prot(unsigned long pfn, pgprot_t prot);
>  void *__kmap_local_page_prot(struct page *page, pgprot_t prot);
> -void kunmap_local_indexed(void *vaddr);
> +void kunmap_local_indexed(const void *vaddr);
>  void kmap_local_fork(struct task_struct *tsk);
>  void __kmap_local_sched_out(void);
>  void __kmap_local_sched_in(void);
> @@ -89,7 +89,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
>  	return __kmap_local_pfn_prot(pfn, kmap_prot);
>  }
>  
> -static inline void __kunmap_local(void *vaddr)
> +static inline void __kunmap_local(const void *vaddr)
>  {
>  	kunmap_local_indexed(vaddr);
>  }
> @@ -121,7 +121,7 @@ static inline void *kmap_atomic_pfn(unsigned long pfn)
>  	return __kmap_local_pfn_prot(pfn, kmap_prot);
>  }
>  
> -static inline void __kunmap_atomic(void *addr)
> +static inline void __kunmap_atomic(const void *addr)
>  {
>  	kunmap_local_indexed(addr);
>  	pagefault_enable();
> @@ -197,7 +197,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
>  	return kmap_local_page(pfn_to_page(pfn));
>  }
>  
> -static inline void __kunmap_local(void *addr)
> +static inline void __kunmap_local(const void *addr)
>  {
>  #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>  	kunmap_flush_on_unmap(addr);
> @@ -224,7 +224,7 @@ static inline void *kmap_atomic_pfn(unsigned long pfn)
>  	return kmap_atomic(pfn_to_page(pfn));
>  }
>  
> -static inline void __kunmap_atomic(void *addr)
> +static inline void __kunmap_atomic(const void *addr)
>  {
>  #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>  	kunmap_flush_on_unmap(addr);
> diff --git a/mm/highmem.c b/mm/highmem.c
> index 1a692997fac4..e32083e4ce0d 100644
> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -561,7 +561,7 @@ void *__kmap_local_page_prot(struct page *page, pgprot_t prot)
>  }
>  EXPORT_SYMBOL(__kmap_local_page_prot);
>  
> -void kunmap_local_indexed(void *vaddr)
> +void kunmap_local_indexed(const void *vaddr)
>  {
>  	unsigned long addr = (unsigned long) vaddr & PAGE_MASK;
>  	pte_t *kmap_pte;
> -- 
> 2.36.1
>