Re: [PATCH V5 1/2] daxctl: Add support for famfs mode

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Apr 30, 2026 at 03:34:11PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>

Hi John,

Please add an intro paragraph saying what famfs is, preferably
with links to reference material.

This patch doesn't update any docs. Please take a look at what is
needed. I think at least this one:
Documentation/daxctl/daxctl-reconfigure-device.txt needs to add
famfs as a 'mode' option.

> Putting a daxdev in famfs mode means binding it to fsdev_dax.ko
> (drivers/dax/fsdev.c). Finding a daxdev bound to fsdev_dax means
> it is in famfs mode.
>
> A test for this functionality is added in the next commit.
>
> With devdax, famfs, and system-ram modes, the previous logic that assumed
> 'not in mode X means in mode Y' needed to get slightly more complicated.
>
> Add explicit mode detection functions:
> - daxctl_dev_is_famfs_mode(): check if bound to fsdev_dax driver
> - daxctl_dev_is_devdax_mode(): check if bound to device_dax driver
> Both delegate to a shared static helper daxctl_dev_bound_to_module() to
> avoid duplicating the driver-symlink lookup, as does the pre-existing
> daxctl_dev_is_system_ram_capable().
>
> Update mode transition logic in device.c:
> - disable_devdax_device(): verify device is actually in devdax mode
> - disable_famfs_device(): verify device is actually in famfs mode
> - All reconfig_mode_*() functions explicitly check each mode
> - Handle unrecognized mode with an error instead of wrong assumption
>
> Update json.c to report fsdev_dax-bound devices as 'famfs' mode.  An
> unbound device continues to be reported as 'devdax' (the legacy default
> when no driver is bound), to preserve existing behavior.
>
> Signed-off-by: John Groves <john@groves.net>
> ---
>  daxctl/device.c                | 132 +++++++++++++++++++++++++++++++++++++----
>  daxctl/json.c                  |  13 +++-
>  daxctl/lib/libdaxctl-private.h |   2 +
>  daxctl/lib/libdaxctl.c         |  39 ++++++++++--
>  daxctl/lib/libdaxctl.sym       |   7 +++
>  daxctl/libdaxctl.h             |   3 +
>  6 files changed, 181 insertions(+), 15 deletions(-)
>
> diff --git a/daxctl/device.c b/daxctl/device.c
> index a4e36b130a09..003609e4abba 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -42,6 +42,7 @@ enum dev_mode {
>  	DAXCTL_DEV_MODE_UNKNOWN,
>  	DAXCTL_DEV_MODE_DEVDAX,
>  	DAXCTL_DEV_MODE_RAM,
> +	DAXCTL_DEV_MODE_FAMFS,
>  };


The above enum dev_mode in device.c shares enum names with enum
daxctl_dev_mode later in this patch (in libdaxctl-private.h) but assigns
different numeric values. The overlap predates this patch, so not
something you introduced, but adding FAMFS to both is a good moment to
fix it before it gets more entrenched.

Two enums with identical member names but different values is confusing
and risks silent cross-assignment bugs with no compiler warning.

Suggest renaming this local enum to `enum reconfig_mode` with members
`RECONFIG_MODE_{UNKNOWN,DEVDAX,RAM,FAMFS}`. That confines it to the reconfig
path. Then the library enum gets unambiguous ownership of `DAXCTL_DEV_MODE_*`.
After that accidental cross-assignments are obvious type mismatches rather
than a silent wrong-value bug.


>
>  struct mapping {
>  	unsigned long long start, end, pgoff;
> @@ -471,6 +472,13 @@ static const char *parse_device_options(int argc, const char **argv,
>  					"--no-online is incompatible with --mode=devdax\n");
>  				rc =  -EINVAL;
>  			}
> +		} else if (strcmp(param.mode, "famfs") == 0) {
> +			reconfig_mode = DAXCTL_DEV_MODE_FAMFS;
> +			if (param.no_online) {
> +				fprintf(stderr,
> +					"--no-online is incompatible with --mode=famfs\n");
> +				rc =  -EINVAL;


rm extra whitespace after =


> +			}
>  		}
>  		break;
>  	case ACTION_CREATE:
> @@ -696,8 +704,42 @@ static int disable_devdax_device(struct daxctl_dev *dev)
>  	int rc;
>
>  	if (mem) {
> -		fprintf(stderr, "%s was already in system-ram mode\n",
> -			devname);
> +		fprintf(stderr, "%s is in system-ram mode\n", devname);
> +		return 1;
> +	}
> +	if (daxctl_dev_is_famfs_mode(dev)) {
> +		fprintf(stderr, "%s is in famfs mode\n", devname);
> +		return 1;
> +	}
> +	if (!daxctl_dev_is_devdax_mode(dev)) {
> +		fprintf(stderr, "%s is not in devdax mode\n", devname);
> +		return 1;
> +	}
> +	rc = daxctl_dev_disable(dev);
> +	if (rc) {
> +		fprintf(stderr, "%s: disable failed: %s\n",
> +			daxctl_dev_get_devname(dev), strerror(-rc));
> +		return rc;
> +	}
> +	return 0;
> +}
> +
> +static int disable_famfs_device(struct daxctl_dev *dev)
> +{
> +	struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
> +	const char *devname = daxctl_dev_get_devname(dev);
> +	int rc;
> +
> +	if (mem) {
> +		fprintf(stderr, "%s is in system-ram mode\n", devname);
> +		return 1;
> +	}
> +	if (daxctl_dev_is_devdax_mode(dev)) {
> +		fprintf(stderr, "%s is in devdax mode\n", devname);
> +		return 1;
> +	}
> +	if (!daxctl_dev_is_famfs_mode(dev)) {
> +		fprintf(stderr, "%s is not in famfs mode\n", devname);
>  		return 1;
>  	}
>  	rc = daxctl_dev_disable(dev);


disable_devdax_device() and disable_famfs_device() differ only in
which mode they accept vs. reject. With reconfig_mode_* switching on
daxctl_dev_get_mode() first, the caller already knows the mode, so
the internal mode-sanity checks are redundant.

What's left in each function is daxctl_dev_disable() and an error
fprintf, the same code in both, so collapse to a single helper:

    static int disable_mode_device(struct daxctl_dev *dev);

No mode parameter: the precondition (caller has matched the mode)
means nothing mode-specific remains.


> @@ -711,6 +753,7 @@ static int disable_devdax_device(struct daxctl_dev *dev)
>
>  static int reconfig_mode_system_ram(struct daxctl_dev *dev)
>  {
> +	struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
>  	const char *devname = daxctl_dev_get_devname(dev);
>  	int rc, skip_enable = 0;
>
> @@ -724,11 +767,21 @@ static int reconfig_mode_system_ram(struct daxctl_dev *dev)
>  	}
>
>  	if (daxctl_dev_is_enabled(dev)) {
> -		rc = disable_devdax_device(dev);
> -		if (rc < 0)
> -			return rc;
> -		if (rc > 0)
> +		if (mem) {
> +			/* already in system-ram mode */
>  			skip_enable = 1;
> +		} else if (daxctl_dev_is_famfs_mode(dev)) {
> +			rc = disable_famfs_device(dev);
> +			if (rc)
> +				return rc;
> +		} else if (daxctl_dev_is_devdax_mode(dev)) {
> +			rc = disable_devdax_device(dev);
> +			if (rc)
> +				return rc;
> +		} else {
> +			fprintf(stderr, "%s: unknown mode\n", devname);
> +			return -EINVAL;
> +		}
>  	}

This if-else chain is repeated in all three reconfig_mode_*() functions below.
Please add a private helper and switch on the result. Something like:

    static enum daxctl_dev_mode daxctl_dev_get_mode(struct daxctl_dev *dev)
    {
            if (daxctl_dev_get_memory(dev))
                    return DAXCTL_DEV_MODE_RAM;
            if (daxctl_dev_is_famfs_mode(dev))
                    return DAXCTL_DEV_MODE_FAMFS;
            if (daxctl_dev_is_devdax_mode(dev))
                    return DAXCTL_DEV_MODE_DEVDAX;
            return DAXCTL_DEV_MODE_UNKNOWN;
    }

Then each reconfig_mode_* becomes a switch on the current mode. The
precedence (system-ram first because it's detected via `mem` rather
than driver binding) lives in one place instead of three.

> @@ -750,7 +803,7 @@ static int disable_system_ram_device(struct daxctl_dev *dev)
>  	int rc;
>
>  	if (!mem) {
> -		fprintf(stderr, "%s was already in devdax mode\n", devname);
> +		fprintf(stderr, "%s is not in system-ram mode\n", devname);
>  		return 1;
>  	}
>
> @@ -786,12 +839,31 @@ static int disable_system_ram_device(struct daxctl_dev *dev)
>
>  static int reconfig_mode_devdax(struct daxctl_dev *dev)
>  {
> +	struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
> +	const char *devname = daxctl_dev_get_devname(dev);
>  	int rc;
>
>  	if (daxctl_dev_is_enabled(dev)) {
> -		rc = disable_system_ram_device(dev);
> -		if (rc)
> -			return rc;
> +		if (mem) {
> +			rc = disable_system_ram_device(dev);
> +			if (rc)
> +				return rc;
> +		} else if (daxctl_dev_is_famfs_mode(dev)) {
> +			rc = disable_famfs_device(dev);
> +			if (rc)
> +				return rc;
> +		} else if (daxctl_dev_is_devdax_mode(dev)) {
> +			/* already in devdax mode, just re-enable */
> +			rc = daxctl_dev_disable(dev);
> +			if (rc) {
> +				fprintf(stderr, "%s: disable failed: %s\n",
> +					devname, strerror(-rc));
> +				return rc;
> +			}
> +		} else {
> +			fprintf(stderr, "%s: unknown mode\n", devname);
> +			return -EINVAL;
> +		}
>  	}


Same if-else chain, second copy. See the helper suggestion above.


>  	rc = daxctl_dev_enable_devdax(dev);
> @@ -801,6 +873,43 @@ static int reconfig_mode_devdax(struct daxctl_dev *dev)
>  	return 0;
>  }
>
> +static int reconfig_mode_famfs(struct daxctl_dev *dev)
> +{
> +	struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
> +	const char *devname = daxctl_dev_get_devname(dev);
> +	int rc;
> +
> +	if (daxctl_dev_is_enabled(dev)) {
> +		if (mem) {
> +			fprintf(stderr,
> +				"%s is in system-ram mode; must be in devdax mode to convert to famfs\n",
> +				devname);
> +			return -EINVAL;
> +		} else if (daxctl_dev_is_famfs_mode(dev)) {
> +			/* already in famfs mode, just re-enable */
> +			rc = daxctl_dev_disable(dev);
> +			if (rc) {
> +				fprintf(stderr, "%s: disable failed: %s\n",
> +					devname, strerror(-rc));
> +				return rc;
> +			}
> +		} else if (daxctl_dev_is_devdax_mode(dev)) {
> +			rc = disable_devdax_device(dev);
> +			if (rc)
> +				return rc;
> +		} else {
> +			fprintf(stderr, "%s: unknown mode\n", devname);
> +			return -EINVAL;
> +		}
> +	}


Same if-else chain, third copy.


> +
> +	rc = daxctl_dev_enable_famfs(dev);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
>  static int do_create(struct daxctl_region *region, long long val,
>  		     struct json_object **jdevs)
>  {
> @@ -887,6 +996,9 @@ static int do_reconfig(struct daxctl_dev *dev, enum dev_mode mode,
>  	case DAXCTL_DEV_MODE_DEVDAX:
>  		rc = reconfig_mode_devdax(dev);
>  		break;
> +	case DAXCTL_DEV_MODE_FAMFS:
> +		rc = reconfig_mode_famfs(dev);
> +		break;
>  	default:
>  		fprintf(stderr, "%s: unknown mode requested: %d\n",
>  			devname, mode);
> diff --git a/daxctl/json.c b/daxctl/json.c
> index 3cbce9dcd651..2a4b12c2f925 100644
> --- a/daxctl/json.c
> +++ b/daxctl/json.c
> @@ -48,8 +48,19 @@ struct json_object *util_daxctl_dev_to_json(struct daxctl_dev *dev,
>
>  	if (mem)
>  		jobj = json_object_new_string("system-ram");
> -	else
> +	else if (daxctl_dev_is_famfs_mode(dev))
> +		jobj = json_object_new_string("famfs");
> +	else if (daxctl_dev_is_devdax_mode(dev))
>  		jobj = json_object_new_string("devdax");


The 'else if' above, and the 'else' below, both assign "devdax",
so the 'else if' is redundant. And once daxctl_dev_get_mode()
exists, this becomes a switch on the result with "devdax"
as the UNKNOWN fallback.


> +	else {
> +		/* Legacy condition; if a daxdev is not in any "mode", that
> +		 * means no driver is bound. We report that as a disabled
> +		 * device in devdax mode. (the disabled modifier is added later
> +		 * in this function if applicable)
> +		 */
> +		jobj = json_object_new_string("devdax");
> +	}
> +
>  	if (jobj)
>  		json_object_object_add(jdev, "mode", jobj);
>
> diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl-private.h
> index ae45311e5d57..0bb73e8c04bf 100644
> --- a/daxctl/lib/libdaxctl-private.h
> +++ b/daxctl/lib/libdaxctl-private.h
> @@ -21,12 +21,14 @@ static const char *dax_subsystems[] = {
>  enum daxctl_dev_mode {
>  	DAXCTL_DEV_MODE_DEVDAX = 0,
>  	DAXCTL_DEV_MODE_RAM,
> +	DAXCTL_DEV_MODE_FAMFS,
>  	DAXCTL_DEV_MODE_END,
>  };
>
>  static const char *dax_modules[] = {
>  	[DAXCTL_DEV_MODE_DEVDAX] = "device_dax",
>  	[DAXCTL_DEV_MODE_RAM] = "kmem",
> +	[DAXCTL_DEV_MODE_FAMFS] = "fsdev_dax",
>  };


Add a DAXCTL_DEV_MODE_UNKNOWN sentinel here to support the
daxctl_dev_get_mode() helper cleanly. Doesn't have to be assigned a
value, can reuse END or add it alongside, your call.


>  enum memory_op {
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 02ae7e50b123..33121dcb1d1b 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -385,13 +385,13 @@ static bool device_model_is_dax_bus(struct daxctl_dev *dev)
>  	return false;
>  }
>
> -DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
> +static int daxctl_dev_bound_to_module(struct daxctl_dev *dev, const char *mod_name)
>  {
>  	const char *devname = daxctl_dev_get_devname(dev);
>  	struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
>  	const char *mod_base;
>  	char *mod_path;
> -	char path[200];
> +	char path[PATH_MAX];
>  	const int len = sizeof(path);
>
>  	if (!device_model_is_dax_bus(dev))


Nice!


> @@ -406,11 +406,13 @@ DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
>  	}
>
>  	mod_path = realpath(path, NULL);
> -	if (!mod_path)
> +	if (!mod_path) {
> +		dbg(ctx, "%s: realpath failed for driver link\n", devname);
>  		return false;
> +	}
>
>  	mod_base = path_basename(mod_path);
> -	if (strcmp(mod_base, dax_modules[DAXCTL_DEV_MODE_RAM]) == 0) {
> +	if (strcmp(mod_base, mod_name) == 0) {
>  		free(mod_path);
>  		return true;
>  	}
> @@ -419,6 +421,30 @@ DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
>  	return false;
>  }
>
> +DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
> +{
> +	return daxctl_dev_bound_to_module(dev, dax_modules[DAXCTL_DEV_MODE_RAM]);
> +}


With daxctl_dev_is_famfs_mode() and daxctl_dev_is_devdax_mode() now
alongside it, the _capable vs _mode naming split looks inconsistent.
Post-refactor all three delegate to daxctl_dev_bound_to_module(), which
checks current driver binding.

Either document the semantic distinction more explicitly or consider
adding daxctl_dev_is_system_ram_mode() as the preferred interface.


> +
> +/*
> + * Check if device is currently in famfs mode (bound to fsdev_dax driver).
> + * Returns false for disabled devices: the DAX bus does not retain the previous
> + * driver binding after unbind, so mode cannot be determined without a driver.
> + */
> +DAXCTL_EXPORT int daxctl_dev_is_famfs_mode(struct daxctl_dev *dev)
> +{
> +	return daxctl_dev_bound_to_module(dev, dax_modules[DAXCTL_DEV_MODE_FAMFS]);
> +}
> +
> +/*
> + * Check if device is currently in devdax mode (bound to device_dax driver).
> + * Returns false for disabled devices; see daxctl_dev_is_famfs_mode().
> + */
> +DAXCTL_EXPORT int daxctl_dev_is_devdax_mode(struct daxctl_dev *dev)
> +{
> +	return daxctl_dev_bound_to_module(dev, dax_modules[DAXCTL_DEV_MODE_DEVDAX]);
> +}

These comments claim "Returns false for disabled devices" as if that
were unique to these two helpers, but it isn't. daxctl_dev_is_system_ram_capable
returns false for disabled devices too. Either drop the comment or move it to
to daxctl_dev_bound_to_module().

> +
>  /*
>   * This checks for the device to be in system-ram mode, so calling
>   * daxctl_dev_get_memory() on a devdax mode device will always return NULL.
> @@ -983,6 +1009,11 @@ DAXCTL_EXPORT int daxctl_dev_enable_ram(struct daxctl_dev *dev)
>  	return daxctl_dev_enable(dev, DAXCTL_DEV_MODE_RAM);
>  }
>
> +DAXCTL_EXPORT int daxctl_dev_enable_famfs(struct daxctl_dev *dev)
> +{
> +	return daxctl_dev_enable(dev, DAXCTL_DEV_MODE_FAMFS);
> +}

OK.

> +
>  DAXCTL_EXPORT int daxctl_dev_disable(struct daxctl_dev *dev)
>  {
>  	const char *devname = daxctl_dev_get_devname(dev);
> diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
> index 309881196c86..2a812c6ad918 100644
> --- a/daxctl/lib/libdaxctl.sym
> +++ b/daxctl/lib/libdaxctl.sym
> @@ -104,3 +104,10 @@ LIBDAXCTL_10 {
>  global:
>  	daxctl_dev_is_system_ram_capable;
>  } LIBDAXCTL_9;
> +
> +LIBDAXCTL_11 {
> +global:
> +	daxctl_dev_enable_famfs;
> +	daxctl_dev_is_famfs_mode;
> +	daxctl_dev_is_devdax_mode;
> +} LIBDAXCTL_10;


If you resolve the _capable/_mode naming inconsistency by adding
daxctl_dev_is_system_ram_mode(), please export it here as well.


> diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
> index 53c6bbdae5c3..84fcdb40c7a9 100644
> --- a/daxctl/libdaxctl.h
> +++ b/daxctl/libdaxctl.h
> @@ -72,12 +72,15 @@ int daxctl_dev_is_enabled(struct daxctl_dev *dev);
>  int daxctl_dev_disable(struct daxctl_dev *dev);
>  int daxctl_dev_enable_devdax(struct daxctl_dev *dev);
>  int daxctl_dev_enable_ram(struct daxctl_dev *dev);
> +int daxctl_dev_enable_famfs(struct daxctl_dev *dev);
>  int daxctl_dev_get_target_node(struct daxctl_dev *dev);
>  int daxctl_dev_will_auto_online_memory(struct daxctl_dev *dev);
>  int daxctl_dev_has_online_memory(struct daxctl_dev *dev);
>
>  struct daxctl_memory;
>  int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev);
> +int daxctl_dev_is_famfs_mode(struct daxctl_dev *dev);
> +int daxctl_dev_is_devdax_mode(struct daxctl_dev *dev);


Mirror whatever you do about _capable vs _mode here. If you add
daxctl_dev_get_mode() as a public library function, declare it here
too.


>  struct daxctl_memory *daxctl_dev_get_memory(struct daxctl_dev *dev);
>  struct daxctl_dev *daxctl_memory_get_dev(struct daxctl_memory *mem);
>  const char *daxctl_memory_get_node_path(struct daxctl_memory *mem);