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

[Alison Schofield]

On Sun, Jan 18, 2026 at 10:36:38PM +0000, John Groves wrote:
> From: John Groves <John@xxxxxxxxxx>
> 
> 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.
> 
> The test is added to the destructive test suite since it
> modifies device modes.

Make it clear that it is added in a separate patch. (and assume you
can drop the destructive part too.)

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


The precedence check (ram->famfs->devdax->unknown) now happens in multiple
places. How about adding a daxctl_dev_get_mode() helper to centralize that.
It could be private for now, unless you expect external users to need it.

daxctl_dev_is_famfs_mode() and _is_devdax_mode() are nearly identical aside
from the module name. Refactoring the shared part into a single helper will
also make it easier to add a daxctl_dev_get_mode() without duplicating the
precedence logic.

> 
> Fix 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 now explicitly check each mode
> - Handle unknown mode with error instead of wrong assumption

Wondering about 'Fix' mode transition logic. Was prior logic broken and
should any of these changes be in a precursor patch that is a 'fix'.


> 
> Modify json.c to show 'unknown' if device is not in a recognized mode.

I think this means disabled devices will always look unknown even when
the intended mode is devdax or famfs, but disabled. This seems to
change the meaning of mode from 'configured' to 'active' personality.
Can you detect the configured mode even when disabled?
Perhaps a man page change about this new behavior?

snip


>  
> @@ -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)

Please check the return code semantics.
This gets rid of the <0 vs >0 distinction. That means a '1' skip
becomes an error return to the caller. Is that what you want?

Previously, we had a return 1 from disable_devdax_device for
“not applicable / already in other mode” and I think that is now
gone.


> +		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;
> +		}
>  	}
>  

snip

>  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)

disable_* helpers print an error message on disable failure.
Seems this should too.


> +				return rc;
> +		} else {
> +			fprintf(stderr, "%s: unknown mode\n", devname);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	rc = daxctl_dev_enable_devdax(dev);
> @@ -801,6 +870,40 @@ 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)
> +				return rc;
> +		} else if (daxctl_dev_is_devdax_mode(dev)) {
> +			rc = disable_devdax_device(dev);
> +			if (rc)

and here too...the disable error message.


> +				return rc;
> +		} else {
> +			fprintf(stderr, "%s: unknown mode\n", devname);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	rc = daxctl_dev_enable_famfs(dev);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}

snip

> +DAXCTL_EXPORT int daxctl_dev_is_famfs_mode(struct daxctl_dev *dev)
> +{
> +	const char *devname = daxctl_dev_get_devname(dev);
> +	struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
> +	char *mod_path, *mod_base;
> +	char path[200];

We have PATH_MAX for the above.

> +	const int len = sizeof(path);
> +
> +	if (!device_model_is_dax_bus(dev))
> +		return false;
> +
> +	if (!daxctl_dev_is_enabled(dev))
> +		return false;
> +
> +	if (snprintf(path, len, "%s/driver", dev->dev_path) >= len) {
> +		err(ctx, "%s: buffer too small!\n", devname);
> +		return false;
> +	}
> +
> +	mod_path = realpath(path, NULL);
> +	if (!mod_path)

Maybe a dbg() level err msg here

> +		return false;
> +
> +	mod_base = basename(mod_path);

Please use path_basename() because of this:
https://lore.kernel.org/all/20260116043056.542346-1-alison.schofield@xxxxxxxxx/

Give me a minute ;) to push that to the pending branch and you can
work from there: https://github.com/pmem/ndctl/commits/pending/

snip to end.