Maybe I'm overcomplicating things (it's one of the things I do), but I'm still struggling through how to address all these issues. Some comments inline.
On 26/02/26 06:00PM, Alison Schofield wrote: > On Sun, Jan 18, 2026 at 10:36:38PM +0000, John Groves wrote: > > From: John Groves <[email protected]> > > > > 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? Good point; before famfs mode there were just 2 modes, and not-system-ram == devdax mode is the current standard, even if no driver is bound. At some level that's a conflation, but I'll revise and stick with that unless you have a better idea. Is that how you want it? No driver == devdax mode? Any thoughts? > > 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. Done, thanks... > > > + 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/[email protected]/ > > 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. Done, thanks Still chewing on the other stuff
