On Thu, Apr 30, 2026 at 03:34:11PM +0000, John Groves wrote: > From: John Groves <[email protected]>
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 <[email protected]> > --- > 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);
