[PATCH V3 05/21] dax: Add dax_set_ops() for setting dax_operations at bind time

John Groves posted 21 patches 1 month ago
[PATCH V3 05/21] dax: Add dax_set_ops() for setting dax_operations at bind time
Posted by John Groves 1 month ago
From: John Groves <John@Groves.net>

The dax_device is created (in the non-pmem case) at hmem probe time via
devm_create_dev_dax(), before we know which driver (device_dax,
fsdev_dax, or kmem) will bind - by calling alloc_dax() with NULL ops,
drivers (i.e. fsdev_dax) that need specific dax_operations must set
them later.

Add dax_set_ops() exported function so fsdev_dax can set its ops at
probe time and clear them on remove. device_dax doesn't need ops since
it uses the mmap fault path directly.

Use cmpxchg() to atomically set ops only if currently NULL, returning
-EBUSY if ops are already set. This prevents accidental double-binding.
Clearing ops (NULL) always succeeds.

Signed-off-by: John Groves <john@groves.net>
---
 drivers/dax/fsdev.c | 12 ++++++++++++
 drivers/dax/super.c | 38 +++++++++++++++++++++++++++++++++++++-
 include/linux/dax.h |  1 +
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index 9e2f83aa2584..3f4f593896e3 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -330,12 +330,24 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
 	if (rc)
 		return rc;
 
+	/* Set the dax operations for fs-dax access path */
+	rc = dax_set_ops(dax_dev, &dev_dax_ops);
+	if (rc)
+		return rc;
+
 	run_dax(dax_dev);
 	return devm_add_action_or_reset(dev, fsdev_kill, dev_dax);
 }
 
+static void fsdev_dax_remove(struct dev_dax *dev_dax)
+{
+	/* Clear ops on unbind so they aren't used with a different driver */
+	dax_set_ops(dev_dax->dax_dev, NULL);
+}
+
 static struct dax_device_driver fsdev_dax_driver = {
 	.probe = fsdev_dax_probe,
+	.remove = fsdev_dax_remove,
 	.type = DAXDRV_FSDEV_TYPE,
 };
 
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index c00b9dff4a06..ba0b4cd18a77 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -157,6 +157,9 @@ long dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, long nr_pages,
 	if (!dax_alive(dax_dev))
 		return -ENXIO;
 
+	if (!dax_dev->ops)
+		return -EOPNOTSUPP;
+
 	if (nr_pages < 0)
 		return -EINVAL;
 
@@ -207,6 +210,10 @@ int dax_zero_page_range(struct dax_device *dax_dev, pgoff_t pgoff,
 
 	if (!dax_alive(dax_dev))
 		return -ENXIO;
+
+	if (!dax_dev->ops)
+		return -EOPNOTSUPP;
+
 	/*
 	 * There are no callers that want to zero more than one page as of now.
 	 * Once users are there, this check can be removed after the
@@ -223,7 +230,7 @@ EXPORT_SYMBOL_GPL(dax_zero_page_range);
 size_t dax_recovery_write(struct dax_device *dax_dev, pgoff_t pgoff,
 		void *addr, size_t bytes, struct iov_iter *iter)
 {
-	if (!dax_dev->ops->recovery_write)
+	if (!dax_dev->ops || !dax_dev->ops->recovery_write)
 		return 0;
 	return dax_dev->ops->recovery_write(dax_dev, pgoff, addr, bytes, iter);
 }
@@ -307,6 +314,35 @@ void set_dax_nomc(struct dax_device *dax_dev)
 }
 EXPORT_SYMBOL_GPL(set_dax_nomc);
 
+/**
+ * dax_set_ops - set the dax_operations for a dax_device
+ * @dax_dev: the dax_device to configure
+ * @ops: the operations to set (may be NULL to clear)
+ *
+ * This allows drivers to set the dax_operations after the dax_device
+ * has been allocated. This is needed when the device is created before
+ * the driver that needs specific ops is bound (e.g., fsdev_dax binding
+ * to a dev_dax created by hmem).
+ *
+ * When setting non-NULL ops, fails if ops are already set (returns -EBUSY).
+ * When clearing ops (NULL), always succeeds.
+ *
+ * Return: 0 on success, -EBUSY if ops already set
+ */
+int dax_set_ops(struct dax_device *dax_dev, const struct dax_operations *ops)
+{
+	if (ops) {
+		/* Setting ops: fail if already set */
+		if (cmpxchg(&dax_dev->ops, NULL, ops) != NULL)
+			return -EBUSY;
+	} else {
+		/* Clearing ops: always allowed */
+		dax_dev->ops = NULL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dax_set_ops);
+
 bool dax_alive(struct dax_device *dax_dev)
 {
 	lockdep_assert_held(&dax_srcu);
diff --git a/include/linux/dax.h b/include/linux/dax.h
index 74e098010016..3fcd8562b72b 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -246,6 +246,7 @@ static inline void dax_break_layout_final(struct inode *inode)
 
 bool dax_alive(struct dax_device *dax_dev);
 void *dax_get_private(struct dax_device *dax_dev);
+int dax_set_ops(struct dax_device *dax_dev, const struct dax_operations *ops);
 long dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, long nr_pages,
 		enum dax_access_mode mode, void **kaddr, unsigned long *pfn);
 size_t dax_copy_from_iter(struct dax_device *dax_dev, pgoff_t pgoff, void *addr,
-- 
2.49.0
Re: [PATCH V3 05/21] dax: Add dax_set_ops() for setting dax_operations at bind time
Posted by Jonathan Cameron 1 month ago
On Wed,  7 Jan 2026 09:33:14 -0600
John Groves <John@Groves.net> wrote:

> From: John Groves <John@Groves.net>
> 
> The dax_device is created (in the non-pmem case) at hmem probe time via
> devm_create_dev_dax(), before we know which driver (device_dax,
> fsdev_dax, or kmem) will bind - by calling alloc_dax() with NULL ops,
> drivers (i.e. fsdev_dax) that need specific dax_operations must set
> them later.
> 
> Add dax_set_ops() exported function so fsdev_dax can set its ops at
> probe time and clear them on remove. device_dax doesn't need ops since
> it uses the mmap fault path directly.
> 
> Use cmpxchg() to atomically set ops only if currently NULL, returning
> -EBUSY if ops are already set. This prevents accidental double-binding.
> Clearing ops (NULL) always succeeds.
> 
> Signed-off-by: John Groves <john@groves.net>
Hi John

This one runs into the fun mess of mixing devm and other calls.
I'd advise you just don't do it because it makes code much harder
to review and hits the 'smells bad' button.

Jonathan

> ---
>  drivers/dax/fsdev.c | 12 ++++++++++++
>  drivers/dax/super.c | 38 +++++++++++++++++++++++++++++++++++++-
>  include/linux/dax.h |  1 +
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 9e2f83aa2584..3f4f593896e3 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -330,12 +330,24 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  	if (rc)
>  		return rc;
>  
> +	/* Set the dax operations for fs-dax access path */
> +	rc = dax_set_ops(dax_dev, &dev_dax_ops);
> +	if (rc)
> +		return rc;
> +
>  	run_dax(dax_dev);
>  	return devm_add_action_or_reset(dev, fsdev_kill, dev_dax);
>  }
>  
> +static void fsdev_dax_remove(struct dev_dax *dev_dax)
> +{
> +	/* Clear ops on unbind so they aren't used with a different driver */
> +	dax_set_ops(dev_dax->dax_dev, NULL);

Generally orderings of calls that mix devm and stuff done manually in remove are
a bad idea.  They can be safe (and this one probably is) but it adds a review
burden that is best avoided.

Once you stop using devm_ you need to stop it for everything.  So either
use a devm_add_action_or_reset for this or drop the one for fsdev_kill and
call that code here instead.

> +}
> +
>  static struct dax_device_driver fsdev_dax_driver = {
>  	.probe = fsdev_dax_probe,
> +	.remove = fsdev_dax_remove,
>  	.type = DAXDRV_FSDEV_TYPE,
>  };
Re: [PATCH V3 05/21] dax: Add dax_set_ops() for setting dax_operations at bind time
Posted by John Groves 1 month ago
On 26/01/08 12:06PM, Jonathan Cameron wrote:
> On Wed,  7 Jan 2026 09:33:14 -0600
> John Groves <John@Groves.net> wrote:
> 
> > From: John Groves <John@Groves.net>
> > 
> > The dax_device is created (in the non-pmem case) at hmem probe time via
> > devm_create_dev_dax(), before we know which driver (device_dax,
> > fsdev_dax, or kmem) will bind - by calling alloc_dax() with NULL ops,
> > drivers (i.e. fsdev_dax) that need specific dax_operations must set
> > them later.
> > 
> > Add dax_set_ops() exported function so fsdev_dax can set its ops at
> > probe time and clear them on remove. device_dax doesn't need ops since
> > it uses the mmap fault path directly.
> > 
> > Use cmpxchg() to atomically set ops only if currently NULL, returning
> > -EBUSY if ops are already set. This prevents accidental double-binding.
> > Clearing ops (NULL) always succeeds.
> > 
> > Signed-off-by: John Groves <john@groves.net>
> Hi John
> 
> This one runs into the fun mess of mixing devm and other calls.
> I'd advise you just don't do it because it makes code much harder
> to review and hits the 'smells bad' button.
> 
> Jonathan

If I don't stink up something, I'm not trying hard enough :D

Next iteration will be full-devm.

[ ... ]

Thanks,
John