Received-SPF: pass (google.com: domain of linux-kernel+bounces-58732-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 8 Feb 2024 13:39:31 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Dan Williams
	<dan.j.williams@intel.com>, Arnd Bergmann <arnd@arndb.de>, Dave Chinner
	<david@fromorbit.com>
CC: <linux-kernel@vger.kernel.org>, Mathieu Desnoyers
	<mathieu.desnoyers@efficios.com>, Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>, Vishal Verma
	<vishal.l.verma@intel.com>, Dave Jiang <dave.jiang@intel.com>, Matthew Wilcox
	<willy@infradead.org>, Russell King <linux@armlinux.org.uk>,
	<linux-arch@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-mm@kvack.org>,
	<linux-xfs@vger.kernel.org>, <dm-devel@lists.linux.dev>,
	<nvdimm@lists.linux.dev>, <linux-s390@vger.kernel.org>
Subject: RE: [PATCH v4 06/12] dax: Check for data cache aliasing at runtime
Message-ID: <65c54a13c52e_afa429444@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240208184913.484340-1-mathieu.desnoyers@efficios.com>
 <20240208184913.484340-7-mathieu.desnoyers@efficios.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240208184913.484340-7-mathieu.desnoyers@efficios.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?/nRGhgD8evUSN1FoSZfuUnDJiVdVczMg7t9uKrzlEjGyarUprsEkzpeaCGcl?=
 =?us-ascii?Q?68nMWvD9wpjS22R/a+mKlMWw8Vn49/YZMMQQu84kS+PmKjZ5xkLPdVu+jfOh?=
 =?us-ascii?Q?FYOJ0iH/3OaxRXgBs+uY3vAshMzKFYRvYSUolI2SB/2L5USB3X1N/nNFr/to?=
 =?us-ascii?Q?iw6Yn37uYzWTbOAapUvk00vKlYqDCTY0AL1bCIf726ILInEYoGJYgLzJvS8w?=
 =?us-ascii?Q?+/fD7FE1gXmuL7ZK0Wfirtg/0PLdBB+IDN9MZQK8IV/2gim/Xuwmwu+bkeou?=
 =?us-ascii?Q?DHq5dr9t9Gm3MF/usgqjbOFxpd7YLH0mkS7ZC27mMv5Ke0do+89DBe4GSkfa?=
 =?us-ascii?Q?0UmFGhfT6vY/ShUfrPjsJgW83lKQtsCwRIa7+yPqtUTBAGUoK+Uj0eUUfnvf?=
 =?us-ascii?Q?bUeLgE9JehOQ6jdrImDDaEn8JMv5yVzxgnJefB55SDV5id9kg41m7H1aZBA5?=
 =?us-ascii?Q?YWIHPamuvMgww88XzCYmjgsdgFdY2gk7w2ZnmzuFsqwtMvvbTtdrnGl3iy0D?=
 =?us-ascii?Q?20UdUHb0SWsPOzqlL5ppnpCuCIifguKszT7NUKy1raQ2QGK211r5WZSo1FDP?=
 =?us-ascii?Q?hFNyohjQx2TtxlTSbeEBoCz3dQ+zti/i5pCJvAKU2/+ANQXLKqF3ayzqewj2?=
 =?us-ascii?Q?mwm7ZvVYp47/13uxDr6ZaE0pNNIgBDwXcTt0Naa+3f6fXdQpGZfWaLkSYuQV?=
 =?us-ascii?Q?YoZKsgDQFzJDCHYyNnPu9oLq22m3RNayz16b1C6lxxblBRMeS5UHQiUnnpfa?=
 =?us-ascii?Q?BsFEnvkKk7cMoSPPq+5dME0lDhdFf2KfuvolYfIQGls318+xYsWlxbdlssJK?=
 =?us-ascii?Q?gQgk+WOCBfn876mY4nPQNJALVLB0+xAw814JT70CnXo3K3OijAX11PW7v/WP?=
 =?us-ascii?Q?OwFeVTHTjTdrgq9eia79YpDueEkNPubXtouh3gN3r97tlyVCTwnonU7cIJqR?=
 =?us-ascii?Q?jnHuCzehDHR/6LzOpW6SxW4ZYSGUVB5/V3Y64AJ2Y0jebk5UYQt/PgCZazrb?=
 =?us-ascii?Q?ko2jUf+QEnLxYrnQRZAYtryQg/IiQyWfMF7D0dxLbEWJUFj4kuufHE/XZM6M?=
 =?us-ascii?Q?0gAmm46QK8iEmSF8GdD4XYuKeWKHDSDT4iTzVR2xkuiZR0Eo6uNNSdCOQrhz?=
 =?us-ascii?Q?mCwFkT4hwSq9puqi9k1DvYTxCQ3MpAtwJk/w/QvQxFaCBiJAy0t+9RsPeWo5?=
 =?us-ascii?Q?1CmRyVl/DMg/gY8oI4TxKtUlKOYaVZD/vC14DymPpWy0ctddEOO8YCMAqSaB?=
 =?us-ascii?Q?G1uI/LSBvR/v+Qgs/ea2rlROm5ExXjmod/QJpuG9pOQpmpawLJhKX4EVgaBY?=
 =?us-ascii?Q?scDQ4mOjeUQ0lKTvcWlHCPkADiJojTF/hFX5D+QBqK3/lGeNmmmzTrX90pvS?=
 =?us-ascii?Q?a2uSc30Z/eG+cNl1g++6CDpLZJKwAJHbMG/T1kKnZL8AtfaWCIK/Z4tkHhi6?=
 =?us-ascii?Q?aWeO+dd+C+mHMdWjJXmFVTmrwTxUH6XBvzPSA4zsKGlwFRl7TA96gw36vTU1?=
 =?us-ascii?Q?TzAhriBthmuF+KrnIB+ZUDjEC2W/VWTnb6waJMeXm2WTHeG+WA0r1Ac94nwe?=
 =?us-ascii?Q?x2r9StQ06DGG4yFIeFnq6HuV/WX4Aw0cwNQ8+dkIpxqmaxme7BTEHA+nAlVV?=
 =?us-ascii?Q?/A=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9037cc41-42d8-4aaf-e585-08dc28ee70c0
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2024 21:39:33.9996
 (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: ahbSPHZ7bAH6pFyze9pjnNVjJSSLfzhTS7kaAu6gQY/I+mU1HXK8OWTZesPelOHtxx7/97417q1uM/whzsick3BnNS0fpviokC8IMd03eBk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5567
X-OriginatorOrg: intel.com

Mathieu Desnoyers wrote:
> Replace the following fs/Kconfig:FS_DAX dependency:
> 
>   depends on !(ARM || MIPS || SPARC)
> 
> By a runtime check within alloc_dax(). This runtime check returns
> ERR_PTR(-EOPNOTSUPP) if the @ops parameter is non-NULL (which means
> the kernel is using an aliased mapping) on an architecture which
> has data cache aliasing.
> 
> Change the return value from NULL to PTR_ERR(-EOPNOTSUPP) for
> CONFIG_DAX=n for consistency.
> 
> This is done in preparation for using cpu_dcache_is_aliasing() in a
> following change which will properly support architectures which detect
> data cache aliasing at runtime.
> 
> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-cxl@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-xfs@vger.kernel.org
> Cc: dm-devel@lists.linux.dev
> Cc: nvdimm@lists.linux.dev
> ---
>  drivers/dax/super.c | 15 +++++++++++++++
>  fs/Kconfig          |  1 -
>  include/linux/dax.h |  6 +-----
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 0da9232ea175..ce5bffa86bba 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -319,6 +319,11 @@ EXPORT_SYMBOL_GPL(dax_alive);
>   * that any fault handlers or operations that might have seen
>   * dax_alive(), have completed.  Any operations that start after
>   * synchronize_srcu() has run will abort upon seeing !dax_alive().
> + *
> + * Note, because alloc_dax() returns an ERR_PTR() on error, callers
> + * typically store its result into a local variable in order to check
> + * the result. Therefore, care must be taken to populate the struct
> + * device dax_dev field make sure the dax_dev is not leaked.
>   */
>  void kill_dax(struct dax_device *dax_dev)
>  {
> @@ -445,6 +450,16 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops)
>  	dev_t devt;
>  	int minor;
>  
> +	/*
> +	 * Unavailable on architectures with virtually aliased data caches,
> +	 * except for device-dax (NULL operations pointer), which does
> +	 * not use aliased mappings from the kernel.
> +	 */
> +	if (ops && (IS_ENABLED(CONFIG_ARM) ||
> +	    IS_ENABLED(CONFIG_MIPS) ||
> +	    IS_ENABLED(CONFIG_SPARC)))
> +		return ERR_PTR(-EOPNOTSUPP);
> +
>  	if (WARN_ON_ONCE(ops && !ops->zero_page_range))
>  		return ERR_PTR(-EINVAL);
>  
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 42837617a55b..e5efdb3b276b 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -56,7 +56,6 @@ endif # BLOCK
>  config FS_DAX
>  	bool "File system based Direct Access (DAX) support"
>  	depends on MMU
> -	depends on !(ARM || MIPS || SPARC)
>  	depends on ZONE_DEVICE || FS_DAX_LIMITED
>  	select FS_IOMAP
>  	select DAX
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index b463502b16e1..df2d52b8a245 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -86,11 +86,7 @@ static inline void *dax_holder(struct dax_device *dax_dev)
>  static inline struct dax_device *alloc_dax(void *private,
>  		const struct dax_operations *ops)
>  {
> -	/*
> -	 * Callers should check IS_ENABLED(CONFIG_DAX) to know if this
> -	 * NULL is an error or expected.
> -	 */
> -	return NULL;
> +	return ERR_PTR(-EOPNOTSUPP);
>  }

So per other feedback on earlier patches, I think this hunk deserves to
be moved to its own patch earlier in the series as a standalone fixup.

Rest of this patch looks good to me.