[ndctl PATCH v3 1/4] libndctl: test enablement for non-nfit devices

[ndctl PATCH v3 1/4] libndctl: test enablement for non-nfit devices

[Santosh Sivaraj]

Unify adding dimms for papr and nfit families, this will help in adding
all attributes needed for the unit tests too. We don't fail adding a dimm
if some of the dimm attributes are missing, so this will work fine on PAPR
platforms where most dimm attributes are provided.

Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
---
 ndctl/lib/libndctl.c | 103 ++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 65 deletions(-)

v3:
* Drop patch which skips SMART tests, smart test enablement will be posted
  soon.

v2:
* Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
  INTEL family instead. That change was there to test the same code on x86, but
  accidently committed. Now have a environment variable to force test PAPR
  family on x86.

* Patch 4: Remove stray code, artifact of refactoring in patch 1.

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 36fb6fe..26b9317 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1646,41 +1646,9 @@ static int ndctl_bind(struct ndctl_ctx *ctx, struct kmod_module *module,
 static int ndctl_unbind(struct ndctl_ctx *ctx, const char *devpath);
 static struct kmod_module *to_module(struct ndctl_ctx *ctx, const char *alias);
 
-static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
-{
-	int rc = -ENODEV;
-	char buf[SYSFS_ATTR_SIZE];
-	struct ndctl_ctx *ctx = dimm->bus->ctx;
-	char *path = calloc(1, strlen(dimm_base) + 100);
-	const char * const devname = ndctl_dimm_get_devname(dimm);
-
-	dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
-
-	if (!path)
-		return -ENOMEM;
-
-	/* construct path to the papr compatible dimm flags file */
-	sprintf(path, "%s/papr/flags", dimm_base);
-
-	if (ndctl_bus_is_papr_scm(dimm->bus) &&
-	    sysfs_read_attr(ctx, path, buf) == 0) {
-
-		dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname, buf);
-		dimm->cmd_family = NVDIMM_FAMILY_PAPR;
-
-		/* Parse dimm flags */
-		parse_papr_flags(dimm, buf);
-
-		/* Allocate monitor mode fd */
-		dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
-		rc = 0;
-	}
-
-	free(path);
-	return rc;
-}
-
-static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
+static int populate_dimm_attributes(struct ndctl_dimm *dimm,
+				    const char *dimm_base,
+				    const char *bus_prefix)
 {
 	int i, rc = -1;
 	char buf[SYSFS_ATTR_SIZE];
@@ -1694,7 +1662,7 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
 	 * 'unique_id' may not be available on older kernels, so don't
 	 * fail if the read fails.
 	 */
-	sprintf(path, "%s/nfit/id", dimm_base);
+	sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0) {
 		unsigned int b[9];
 
@@ -1709,68 +1677,74 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
 		}
 	}
 
-	sprintf(path, "%s/nfit/handle", dimm_base);
+	sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		goto err_read;
 	dimm->handle = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/phys_id", dimm_base);
+	sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		goto err_read;
 	dimm->phys_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/serial", dimm_base);
+	sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->serial = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/vendor", dimm_base);
+	sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->vendor_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/device", dimm_base);
+	sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->device_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/rev_id", dimm_base);
+	sprintf(path, "%s/%s/rev_id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->revision_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/dirty_shutdown", dimm_base);
+	sprintf(path, "%s/%s/dirty_shutdown", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->dirty_shutdown = strtoll(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/subsystem_vendor", dimm_base);
+	sprintf(path, "%s/%s/subsystem_vendor", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/subsystem_device", dimm_base);
+	sprintf(path, "%s/%s/subsystem_device", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_device_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/subsystem_rev_id", dimm_base);
+	sprintf(path, "%s/%s/subsystem_rev_id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_revision_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/family", dimm_base);
+	sprintf(path, "%s/%s/family", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->cmd_family = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
+	sprintf(path, "%s/%s/dsm_mask", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/format", dimm_base);
+	sprintf(path, "%s/%s/format", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->format[0] = strtoul(buf, NULL, 0);
 	for (i = 1; i < dimm->formats; i++) {
-		sprintf(path, "%s/nfit/format%d", dimm_base, i);
+		sprintf(path, "%s/%s/format%d", dimm_base, bus_prefix, i);
 		if (sysfs_read_attr(ctx, path, buf) == 0)
 			dimm->format[i] = strtoul(buf, NULL, 0);
 	}
 
-	sprintf(path, "%s/nfit/flags", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) == 0)
-		parse_nfit_mem_flags(dimm, buf);
+	sprintf(path, "%s/%s/flags", dimm_base, bus_prefix);
+	if (sysfs_read_attr(ctx, path, buf) == 0) {
+		if (ndctl_bus_has_nfit(dimm->bus))
+			parse_nfit_mem_flags(dimm, buf);
+		else if (ndctl_bus_is_papr_scm(dimm->bus)) {
+			dimm->cmd_family = NVDIMM_FAMILY_PAPR;
+			parse_papr_flags(dimm, buf);
+		}
+	}
 
 	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
 	rc = 0;
@@ -1792,7 +1766,8 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 	if (!path)
 		return NULL;
 
-	sprintf(path, "%s/nfit/formats", dimm_base);
+	sprintf(path, "%s/%s/formats", dimm_base,
+		ndctl_bus_has_nfit(bus) ? "nfit" : "papr");
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		formats = 1;
 	else
@@ -1866,13 +1841,12 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 	else
 		dimm->fwa_result = fwa_result_to_result(buf);
 
+	dimm->formats = formats;
 	/* Check if the given dimm supports nfit */
 	if (ndctl_bus_has_nfit(bus)) {
-		dimm->formats = formats;
-		rc = add_nfit_dimm(dimm, dimm_base);
-	} else if (ndctl_bus_has_of_node(bus)) {
-		rc = add_papr_dimm(dimm, dimm_base);
-	}
+		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
+	} else if (ndctl_bus_has_of_node(bus))
+		rc = populate_dimm_attributes(dimm, dimm_base, "papr");
 
 	if (rc == -ENODEV) {
 		/* Unprobed dimm with no family */
@@ -2531,13 +2505,12 @@ static void *add_region(void *parent, int id, const char *region_base)
 		goto err_read;
 	region->num_mappings = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/range_index", region_base);
-	if (ndctl_bus_has_nfit(bus)) {
-		if (sysfs_read_attr(ctx, path, buf) < 0)
-			goto err_read;
-		region->range_index = strtoul(buf, NULL, 0);
-	} else
+	sprintf(path, "%s/%s/range_index", region_base,
+		ndctl_bus_has_nfit(bus) ? "nfit": "papr");
+	if (sysfs_read_attr(ctx, path, buf) < 0)
 		region->range_index = -1;
+	else
+		region->range_index = strtoul(buf, NULL, 0);
 
 	sprintf(path, "%s/read_only", region_base);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing

[Santosh Sivaraj]

For NFIT to be available ACPI is a must, so don't fail when nfit modules
are missing on a platform that doesn't support ACPI.

Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
---
 test.h                        |  2 +-
 test/ack-shutdown-count-set.c |  2 +-
 test/blk_namespaces.c         |  2 +-
 test/core.c                   | 23 +++++++++++++++++++++--
 test/dpa-alloc.c              |  2 +-
 test/dsm-fail.c               |  2 +-
 test/libndctl.c               |  2 +-
 test/multi-pmem.c             |  2 +-
 test/parent-uuid.c            |  2 +-
 test/pmem_namespaces.c        |  2 +-
 10 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/test.h b/test.h
index cba8d41..7de13fe 100644
--- a/test.h
+++ b/test.h
@@ -20,7 +20,7 @@ void builtin_xaction_namespace_reset(void);
 
 struct kmod_ctx;
 struct kmod_module;
-int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
+int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
 		struct ndctl_ctx *nd_ctx, int log_level,
 		struct ndctl_test *test);
 
diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
index fb1d82b..c561ff3 100644
--- a/test/ack-shutdown-count-set.c
+++ b/test/ack-shutdown-count-set.c
@@ -99,7 +99,7 @@ static int test_ack_shutdown_count_set(int loglevel, struct ndctl_test *test,
 	int result = EXIT_FAILURE, err;
 
 	ndctl_set_log_priority(ctx, loglevel);
-	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
+	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
 	if (err < 0) {
 		result = 77;
 		ndctl_test_skip(test);
diff --git a/test/blk_namespaces.c b/test/blk_namespaces.c
index d7f00cb..f076e85 100644
--- a/test/blk_namespaces.c
+++ b/test/blk_namespaces.c
@@ -228,7 +228,7 @@ int test_blk_namespaces(int log_level, struct ndctl_test *test,
 
 	if (!bus) {
 		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
-		rc = nfit_test_init(&kmod_ctx, &mod, NULL, log_level, test);
+		rc = ndctl_test_init(&kmod_ctx, &mod, NULL, log_level, test);
 		ndctl_invalidate(ctx);
 		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
 		if (rc < 0 || !bus) {
diff --git a/test/core.c b/test/core.c
index cc7d8d9..903034a 100644
--- a/test/core.c
+++ b/test/core.c
@@ -11,6 +11,7 @@
 #include <util/log.h>
 #include <util/sysfs.h>
 #include <ndctl/libndctl.h>
+#include <ndctl/ndctl.h>
 #include <ccan/array_size/array_size.h>
 
 #define KVER_STRLEN 20
@@ -106,11 +107,11 @@ int ndctl_test_get_skipped(struct ndctl_test *test)
 	return test->skip;
 }
 
-int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
+int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
 		struct ndctl_ctx *nd_ctx, int log_level,
 		struct ndctl_test *test)
 {
-	int rc;
+	int rc, family = NVDIMM_FAMILY_INTEL;
 	unsigned int i;
 	const char *name;
 	struct ndctl_bus *bus;
@@ -127,6 +128,19 @@ int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
 		"nd_e820",
 		"nd_pmem",
 	};
+	char *test_env;
+
+	/* Do we want to force test PAPR? */
+	test_env = getenv("NDCTL_TEST_FAMILY");
+	if (test_env && strcmp(test_env, "PAPR") == 0)
+		family = NVDIMM_FAMILY_PAPR;
+
+	/* ACPI is a must for nfit, so if ACPI is not available let's default to
+	 * PAPR */
+	if (access("/sys/bus/acpi", F_OK) == -1) {
+		if (errno == ENOENT)
+			family = NVDIMM_FAMILY_PAPR;
+	}
 
 	log_init(&log_ctx, "test/init", "NDCTL_TEST");
 	log_ctx.log_priority = log_level;
@@ -185,6 +199,11 @@ retry:
 
 		path = kmod_module_get_path(*mod);
 		if (!path) {
+			if (family == NVDIMM_FAMILY_PAPR &&
+			    (strcmp(name, "nfit") == 0 ||
+			     strcmp(name, "nd_e820") == 0))
+				continue;
+
 			log_err(&log_ctx, "%s.ko: failed to get path\n", name);
 			break;
 		}
diff --git a/test/dpa-alloc.c b/test/dpa-alloc.c
index e922009..0b3bb7a 100644
--- a/test/dpa-alloc.c
+++ b/test/dpa-alloc.c
@@ -289,7 +289,7 @@ int test_dpa_alloc(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
 		return 77;
 
 	ndctl_set_log_priority(ctx, loglevel);
-	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
+	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
 	if (err < 0) {
 		ndctl_test_skip(test);
 		fprintf(stderr, "nfit_test unavailable skipping tests\n");
diff --git a/test/dsm-fail.c b/test/dsm-fail.c
index 9dfd8b0..0a6383d 100644
--- a/test/dsm-fail.c
+++ b/test/dsm-fail.c
@@ -346,7 +346,7 @@ int test_dsm_fail(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
 	int result = EXIT_FAILURE, err;
 
 	ndctl_set_log_priority(ctx, loglevel);
-	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
+	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
 	if (err < 0) {
 		result = 77;
 		ndctl_test_skip(test);
diff --git a/test/libndctl.c b/test/libndctl.c
index 24d72b3..0e88fce 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -2692,7 +2692,7 @@ int test_libndctl(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
 	daxctl_set_log_priority(daxctl_ctx, loglevel);
 	ndctl_set_private_data(ctx, test);
 
-	err = nfit_test_init(&kmod_ctx, &mod, ctx, loglevel, test);
+	err = ndctl_test_init(&kmod_ctx, &mod, ctx, loglevel, test);
 	if (err < 0) {
 		ndctl_test_skip(test);
 		fprintf(stderr, "nfit_test unavailable skipping tests\n");
diff --git a/test/multi-pmem.c b/test/multi-pmem.c
index 3d10952..3ea08cc 100644
--- a/test/multi-pmem.c
+++ b/test/multi-pmem.c
@@ -249,7 +249,7 @@ int test_multi_pmem(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx
 
 	ndctl_set_log_priority(ctx, loglevel);
 
-	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
+	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
 	if (err < 0) {
 		result = 77;
 		ndctl_test_skip(test);
diff --git a/test/parent-uuid.c b/test/parent-uuid.c
index 6424e9f..bded33a 100644
--- a/test/parent-uuid.c
+++ b/test/parent-uuid.c
@@ -218,7 +218,7 @@ int test_parent_uuid(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ct
 		return 77;
 
 	ndctl_set_log_priority(ctx, loglevel);
-	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
+	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
 	if (err < 0) {
 		ndctl_test_skip(test);
 		fprintf(stderr, "nfit_test unavailable skipping tests\n");
diff --git a/test/pmem_namespaces.c b/test/pmem_namespaces.c
index f0f2edd..a4db1ae 100644
--- a/test/pmem_namespaces.c
+++ b/test/pmem_namespaces.c
@@ -191,7 +191,7 @@ int test_pmem_namespaces(int log_level, struct ndctl_test *test,
 
 	if (!bus) {
 		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
-		rc = nfit_test_init(&kmod_ctx, &mod, NULL, log_level, test);
+		rc = ndctl_test_init(&kmod_ctx, &mod, NULL, log_level, test);
 		ndctl_invalidate(ctx);
 		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
 		if (rc < 0 || !bus) {
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v3 3/4] papr: Add support to parse save_fail flag for dimm

[Santosh Sivaraj]

This will help in getting the dimm fail tests to run on papr family too.
Also add nvdimm_test compatibility string for recognizing the test module.

Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
---
 ndctl/lib/libndctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 26b9317..dd1a5fc 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -805,6 +805,8 @@ static void parse_papr_flags(struct ndctl_dimm *dimm, char *flags)
 			dimm->flags.f_restore = 1;
 		else if (strcmp(start, "smart_notify") == 0)
 			dimm->flags.f_smart = 1;
+		else if (strcmp(start, "save_fail") == 0)
+			dimm->flags.f_save = 1;
 		start = end + 1;
 	}
 	if (end != start)
@@ -1035,7 +1037,8 @@ NDCTL_EXPORT int ndctl_bus_is_papr_scm(struct ndctl_bus *bus)
 	if (sysfs_read_attr(bus->ctx, bus->bus_buf, buf) < 0)
 		return 0;
 
-	return (strcmp(buf, "ibm,pmemory") == 0);
+	return (strcmp(buf, "ibm,pmemory") == 0 ||
+		strcmp(buf, "nvdimm_test") == 0);
 }
 
 /**
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v3 4/4] Use page size as alignment value

[Santosh Sivaraj]

The alignment sizes passed to ndctl in the tests are all hardcoded to 4k,
the default page size on x86. Change those to the default page size on that
architecture (sysconf/getconf). No functional changes otherwise.

Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
---
 test/dpa-alloc.c    | 15 ++++++++-------
 test/multi-dax.sh   |  6 ++++--
 test/sector-mode.sh |  4 +++-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/test/dpa-alloc.c b/test/dpa-alloc.c
index 0b3bb7a..59185cf 100644
--- a/test/dpa-alloc.c
+++ b/test/dpa-alloc.c
@@ -38,12 +38,13 @@ static int do_test(struct ndctl_ctx *ctx, struct ndctl_test *test)
 	struct ndctl_region *region, *blk_region = NULL;
 	struct ndctl_namespace *ndns;
 	struct ndctl_dimm *dimm;
-	unsigned long size;
+	unsigned long size, page_size;
 	struct ndctl_bus *bus;
 	char uuid_str[40];
 	int round;
 	int rc;
 
+	page_size = sysconf(_SC_PAGESIZE);
 	/* disable nfit_test.1, not used in this test */
 	bus = ndctl_bus_get_by_provider(ctx, NFIT_PROVIDER1);
 	if (!bus)
@@ -124,11 +125,11 @@ static int do_test(struct ndctl_ctx *ctx, struct ndctl_test *test)
 			return rc;
 		}
 		ndctl_namespace_disable_invalidate(ndns);
-		rc = ndctl_namespace_set_size(ndns, SZ_4K);
+		rc = ndctl_namespace_set_size(ndns, page_size);
 		if (rc) {
-			fprintf(stderr, "failed to init %s to size: %d\n",
+			fprintf(stderr, "failed to init %s to size: %lu\n",
 					ndctl_namespace_get_devname(ndns),
-					SZ_4K);
+					page_size);
 			return rc;
 		}
 		namespaces[i].ndns = ndns;
@@ -150,7 +151,7 @@ static int do_test(struct ndctl_ctx *ctx, struct ndctl_test *test)
 		ndns = namespaces[i % ARRAY_SIZE(namespaces)].ndns;
 		if (i % ARRAY_SIZE(namespaces) == 0)
 			round++;
-		size = SZ_4K * round;
+		size = page_size * round;
 		rc = ndctl_namespace_set_size(ndns, size);
 		if (rc) {
 			fprintf(stderr, "%s: set_size: %lx failed: %d\n",
@@ -166,7 +167,7 @@ static int do_test(struct ndctl_ctx *ctx, struct ndctl_test *test)
 	i--;
 	round++;
 	ndns = namespaces[i % ARRAY_SIZE(namespaces)].ndns;
-	size = SZ_4K * round;
+	size = page_size * round;
 	rc = ndctl_namespace_set_size(ndns, size);
 	if (rc) {
 		fprintf(stderr, "%s failed to update while labels full\n",
@@ -175,7 +176,7 @@ static int do_test(struct ndctl_ctx *ctx, struct ndctl_test *test)
 	}
 
 	round--;
-	size = SZ_4K * round;
+	size = page_size * round;
 	rc = ndctl_namespace_set_size(ndns, size);
 	if (rc) {
 		fprintf(stderr, "%s failed to reduce size while labels full\n",
diff --git a/test/multi-dax.sh b/test/multi-dax.sh
index e932569..9451ed0 100755
--- a/test/multi-dax.sh
+++ b/test/multi-dax.sh
@@ -12,6 +12,8 @@ check_min_kver "4.13" || do_skip "may lack multi-dax support"
 
 trap 'err $LINENO' ERR
 
+ALIGN_SIZE=`getconf PAGESIZE`
+
 # setup (reset nfit_test dimms)
 modprobe nfit_test
 $NDCTL disable-region -b $NFIT_TEST_BUS0 all
@@ -22,9 +24,9 @@ rc=1
 query=". | sort_by(.available_size) | reverse | .[0].dev"
 region=$($NDCTL list -b $NFIT_TEST_BUS0 -t pmem -Ri | jq -r "$query")
 
-json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m devdax -a 4096 -s 16M)
+json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m devdax -a $ALIGN_SIZE -s 16M)
 chardev1=$(echo $json | jq ". | select(.mode == \"devdax\") | .daxregion.devices[0].chardev")
-json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m devdax -a 4096 -s 16M)
+json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m devdax -a $ALIGN_SIZE -s 16M)
 chardev2=$(echo $json | jq ". | select(.mode == \"devdax\") | .daxregion.devices[0].chardev")
 
 _cleanup
diff --git a/test/sector-mode.sh b/test/sector-mode.sh
index dd7013e..d03c0ca 100755
--- a/test/sector-mode.sh
+++ b/test/sector-mode.sh
@@ -9,6 +9,8 @@ rc=77
 set -e
 trap 'err $LINENO' ERR
 
+ALIGN_SIZE=`getconf PAGESIZE`
+
 # setup (reset nfit_test dimms)
 modprobe nfit_test
 $NDCTL disable-region -b $NFIT_TEST_BUS0 all
@@ -25,7 +27,7 @@ NAMESPACE=$($NDCTL list -b $NFIT_TEST_BUS1 -N | jq -r "$query")
 REGION=$($NDCTL list -R --namespace=$NAMESPACE | jq -r "(.[]) | .dev")
 echo 0 > /sys/bus/nd/devices/$REGION/read_only
 $NDCTL create-namespace --no-autolabel -e $NAMESPACE -m sector -f -l 4K
-$NDCTL create-namespace --no-autolabel -e $NAMESPACE -m dax -f -a 4K
+$NDCTL create-namespace --no-autolabel -e $NAMESPACE -m dax -f -a $ALIGN_SIZE
 $NDCTL create-namespace --no-autolabel -e $NAMESPACE -m sector -f -l 4K
 
 _cleanup
-- 
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 1/4] libndctl: test enablement for non-nfit devices

[Verma, Vishal L]

On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
> Unify adding dimms for papr and nfit families, this will help in adding

Minor nit, but it seems like the subject line and the first sentence in
the body should be swapped. The one-line description of what's happening
in this patch is "Unify adding dimms for papr and nfit families", and
the body can go into more detail about why - i.e. in preparation for
enabling tests on non-nfit devices.

> all attributes needed for the unit tests too. We don't fail adding a dimm
> if some of the dimm attributes are missing, so this will work fine on PAPR
> platforms where most dimm attributes are provided.

Does this mean 'most - but not all'? Might be a good clarification to
make here.

> 
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
>  ndctl/lib/libndctl.c | 103 ++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 65 deletions(-)
> 
> v3:
> * Drop patch which skips SMART tests, smart test enablement will be posted
>   soon.
> 
> v2:
> * Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
>   INTEL family instead. That change was there to test the same code on x86, but
>   accidently committed. Now have a environment variable to force test PAPR
>   family on x86.
> 
> * Patch 4: Remove stray code, artifact of refactoring in patch 1.
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 36fb6fe..26b9317 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1646,41 +1646,9 @@ static int ndctl_bind(struct ndctl_ctx *ctx, struct kmod_module *module,
>  static int ndctl_unbind(struct ndctl_ctx *ctx, const char *devpath);
>  static struct kmod_module *to_module(struct ndctl_ctx *ctx, const char *alias);
>  
> -static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> -{
> -	int rc = -ENODEV;
> -	char buf[SYSFS_ATTR_SIZE];
> -	struct ndctl_ctx *ctx = dimm->bus->ctx;
> -	char *path = calloc(1, strlen(dimm_base) + 100);
> -	const char * const devname = ndctl_dimm_get_devname(dimm);
> -
> -	dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
> -
> -	if (!path)
> -		return -ENOMEM;
> -
> -	/* construct path to the papr compatible dimm flags file */
> -	sprintf(path, "%s/papr/flags", dimm_base);
> -
> -	if (ndctl_bus_is_papr_scm(dimm->bus) &&
> -	    sysfs_read_attr(ctx, path, buf) == 0) {
> -
> -		dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname, buf);
> -		dimm->cmd_family = NVDIMM_FAMILY_PAPR;
> -
> -		/* Parse dimm flags */
> -		parse_papr_flags(dimm, buf);
> -
> -		/* Allocate monitor mode fd */
> -		dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
> -		rc = 0;
> -	}
> -
> -	free(path);
> -	return rc;
> -}
> -
> -static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> +static int populate_dimm_attributes(struct ndctl_dimm *dimm,
> +				    const char *dimm_base,
> +				    const char *bus_prefix)
>  {
>  	int i, rc = -1;
>  	char buf[SYSFS_ATTR_SIZE];
> @@ -1694,7 +1662,7 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>  	 * 'unique_id' may not be available on older kernels, so don't
>  	 * fail if the read fails.
>  	 */
> -	sprintf(path, "%s/nfit/id", dimm_base);
> +	sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0) {
>  		unsigned int b[9];
> 
> @@ -1709,68 +1677,74 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>  		}
>  	}
> 
> -	sprintf(path, "%s/nfit/handle", dimm_base);
> +	sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		goto err_read;
>  	dimm->handle = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/phys_id", dimm_base);
> +	sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		goto err_read;
>  	dimm->phys_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/serial", dimm_base);
> +	sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->serial = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/vendor", dimm_base);
> +	sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->vendor_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/device", dimm_base);
> +	sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->device_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/rev_id", dimm_base);
> +	sprintf(path, "%s/%s/rev_id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->revision_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/dirty_shutdown", dimm_base);
> +	sprintf(path, "%s/%s/dirty_shutdown", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->dirty_shutdown = strtoll(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/subsystem_vendor", dimm_base);
> +	sprintf(path, "%s/%s/subsystem_vendor", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/subsystem_device", dimm_base);
> +	sprintf(path, "%s/%s/subsystem_device", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->subsystem_device_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/subsystem_rev_id", dimm_base);
> +	sprintf(path, "%s/%s/subsystem_rev_id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->subsystem_revision_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/family", dimm_base);
> +	sprintf(path, "%s/%s/family", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->cmd_family = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
> +	sprintf(path, "%s/%s/dsm_mask", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/format", dimm_base);
> +	sprintf(path, "%s/%s/format", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->format[0] = strtoul(buf, NULL, 0);
>  	for (i = 1; i < dimm->formats; i++) {
> -		sprintf(path, "%s/nfit/format%d", dimm_base, i);
> +		sprintf(path, "%s/%s/format%d", dimm_base, bus_prefix, i);
>  		if (sysfs_read_attr(ctx, path, buf) == 0)
>  			dimm->format[i] = strtoul(buf, NULL, 0);
>  	}
>  
> -	sprintf(path, "%s/nfit/flags", dimm_base);
> -	if (sysfs_read_attr(ctx, path, buf) == 0)
> -		parse_nfit_mem_flags(dimm, buf);
> +	sprintf(path, "%s/%s/flags", dimm_base, bus_prefix);
> +	if (sysfs_read_attr(ctx, path, buf) == 0) {
> +		if (ndctl_bus_has_nfit(dimm->bus))
> +			parse_nfit_mem_flags(dimm, buf);
> +		else if (ndctl_bus_is_papr_scm(dimm->bus)) {
> +			dimm->cmd_family = NVDIMM_FAMILY_PAPR;
> +			parse_papr_flags(dimm, buf);
> +		}
> +	}
>  
>  	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
>  	rc = 0;
> @@ -1792,7 +1766,8 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>  	if (!path)
>  		return NULL;
>  
> -	sprintf(path, "%s/nfit/formats", dimm_base);
> +	sprintf(path, "%s/%s/formats", dimm_base,
> +		ndctl_bus_has_nfit(bus) ? "nfit" : "papr");
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		formats = 1;
>  	else
> @@ -1866,13 +1841,12 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>  	else
>  		dimm->fwa_result = fwa_result_to_result(buf);
>  
> +	dimm->formats = formats;
>  	/* Check if the given dimm supports nfit */
>  	if (ndctl_bus_has_nfit(bus)) {
> -		dimm->formats = formats;
> -		rc = add_nfit_dimm(dimm, dimm_base);
> -	} else if (ndctl_bus_has_of_node(bus)) {
> -		rc = add_papr_dimm(dimm, dimm_base);
> -	}
> +		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
> +	} else if (ndctl_bus_has_of_node(bus))
> +		rc = populate_dimm_attributes(dimm, dimm_base, "papr");
>  
>  	if (rc == -ENODEV) {
>  		/* Unprobed dimm with no family */
> @@ -2531,13 +2505,12 @@ static void *add_region(void *parent, int id, const char *region_base)
>  		goto err_read;
>  	region->num_mappings = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/range_index", region_base);
> -	if (ndctl_bus_has_nfit(bus)) {
> -		if (sysfs_read_attr(ctx, path, buf) < 0)
> -			goto err_read;
> -		region->range_index = strtoul(buf, NULL, 0);
> -	} else
> +	sprintf(path, "%s/%s/range_index", region_base,
> +		ndctl_bus_has_nfit(bus) ? "nfit": "papr");
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		region->range_index = -1;
> +	else
> +		region->range_index = strtoul(buf, NULL, 0);
>  
>  	sprintf(path, "%s/read_only", region_base);
>  	if (sysfs_read_attr(ctx, path, buf) < 0)


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing

[Verma, Vishal L]

On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
> For NFIT to be available ACPI is a must, so don't fail when nfit modules
> are missing on a platform that doesn't support ACPI.
> 
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
>  test.h                        |  2 +-
>  test/ack-shutdown-count-set.c |  2 +-
>  test/blk_namespaces.c         |  2 +-
>  test/core.c                   | 23 +++++++++++++++++++++--
>  test/dpa-alloc.c              |  2 +-
>  test/dsm-fail.c               |  2 +-
>  test/libndctl.c               |  2 +-
>  test/multi-pmem.c             |  2 +-
>  test/parent-uuid.c            |  2 +-
>  test/pmem_namespaces.c        |  2 +-
>  10 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/test.h b/test.h
> index cba8d41..7de13fe 100644
> --- a/test.h
> +++ b/test.h
> @@ -20,7 +20,7 @@ void builtin_xaction_namespace_reset(void);
>  
> 
>  struct kmod_ctx;
>  struct kmod_module;
> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>  		struct ndctl_ctx *nd_ctx, int log_level,
>  		struct ndctl_test *test);
>  
> 
> diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
> index fb1d82b..c561ff3 100644
> --- a/test/ack-shutdown-count-set.c
> +++ b/test/ack-shutdown-count-set.c
> @@ -99,7 +99,7 @@ static int test_ack_shutdown_count_set(int loglevel, struct ndctl_test *test,
>  	int result = EXIT_FAILURE, err;
>  
> 
>  	ndctl_set_log_priority(ctx, loglevel);
> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>  	if (err < 0) {
>  		result = 77;
>  		ndctl_test_skip(test);
> diff --git a/test/blk_namespaces.c b/test/blk_namespaces.c
> index d7f00cb..f076e85 100644
> --- a/test/blk_namespaces.c
> +++ b/test/blk_namespaces.c
> @@ -228,7 +228,7 @@ int test_blk_namespaces(int log_level, struct ndctl_test *test,
>  
> 
>  	if (!bus) {
>  		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
> -		rc = nfit_test_init(&kmod_ctx, &mod, NULL, log_level, test);
> +		rc = ndctl_test_init(&kmod_ctx, &mod, NULL, log_level, test);
>  		ndctl_invalidate(ctx);
>  		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
>  		if (rc < 0 || !bus) {
> diff --git a/test/core.c b/test/core.c
> index cc7d8d9..903034a 100644
> --- a/test/core.c
> +++ b/test/core.c
> @@ -11,6 +11,7 @@
>  #include <util/log.h>
>  #include <util/sysfs.h>
>  #include <ndctl/libndctl.h>
> +#include <ndctl/ndctl.h>
>  #include <ccan/array_size/array_size.h>
>  
> 
>  #define KVER_STRLEN 20
> @@ -106,11 +107,11 @@ int ndctl_test_get_skipped(struct ndctl_test *test)
>  	return test->skip;
>  }
>  
> 
> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>  		struct ndctl_ctx *nd_ctx, int log_level,
>  		struct ndctl_test *test)
>  {
> -	int rc;
> +	int rc, family = NVDIMM_FAMILY_INTEL;
>  	unsigned int i;
>  	const char *name;
>  	struct ndctl_bus *bus;
> @@ -127,6 +128,19 @@ int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>  		"nd_e820",
>  		"nd_pmem",
>  	};
> +	char *test_env;
> +
> +	/* Do we want to force test PAPR? */
> +	test_env = getenv("NDCTL_TEST_FAMILY");
> +	if (test_env && strcmp(test_env, "PAPR") == 0)
> +		family = NVDIMM_FAMILY_PAPR;
> +
> +	/* ACPI is a must for nfit, so if ACPI is not available let's default to
> +	 * PAPR */

fix multi line comment to the right formatting:
/*
 * line 1, etc
 */

> +	if (access("/sys/bus/acpi", F_OK) == -1) {
> +		if (errno == ENOENT)
> +			family = NVDIMM_FAMILY_PAPR;
> +	}

Instead of a blind default, can we perform a similar check for presence of
PAPR too?

>  
> 
>  	log_init(&log_ctx, "test/init", "NDCTL_TEST");
>  	log_ctx.log_priority = log_level;
> @@ -185,6 +199,11 @@ retry:
>  
> 
>  		path = kmod_module_get_path(*mod);
>  		if (!path) {
> +			if (family == NVDIMM_FAMILY_PAPR &&
> +			    (strcmp(name, "nfit") == 0 ||
> +			     strcmp(name, "nd_e820") == 0))
> +				continue;
> +
>  			log_err(&log_ctx, "%s.ko: failed to get path\n", name);
>  			break;
>  		}
> diff --git a/test/dpa-alloc.c b/test/dpa-alloc.c
> index e922009..0b3bb7a 100644
> --- a/test/dpa-alloc.c
> +++ b/test/dpa-alloc.c
> @@ -289,7 +289,7 @@ int test_dpa_alloc(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
>  		return 77;
>  
> 
>  	ndctl_set_log_priority(ctx, loglevel);
> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>  	if (err < 0) {
>  		ndctl_test_skip(test);
>  		fprintf(stderr, "nfit_test unavailable skipping tests\n");
> diff --git a/test/dsm-fail.c b/test/dsm-fail.c
> index 9dfd8b0..0a6383d 100644
> --- a/test/dsm-fail.c
> +++ b/test/dsm-fail.c
> @@ -346,7 +346,7 @@ int test_dsm_fail(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
>  	int result = EXIT_FAILURE, err;
>  
> 
>  	ndctl_set_log_priority(ctx, loglevel);
> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>  	if (err < 0) {
>  		result = 77;
>  		ndctl_test_skip(test);
> diff --git a/test/libndctl.c b/test/libndctl.c
> index 24d72b3..0e88fce 100644
> --- a/test/libndctl.c
> +++ b/test/libndctl.c
> @@ -2692,7 +2692,7 @@ int test_libndctl(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
>  	daxctl_set_log_priority(daxctl_ctx, loglevel);
>  	ndctl_set_private_data(ctx, test);
>  
> 
> -	err = nfit_test_init(&kmod_ctx, &mod, ctx, loglevel, test);
> +	err = ndctl_test_init(&kmod_ctx, &mod, ctx, loglevel, test);
>  	if (err < 0) {
>  		ndctl_test_skip(test);
>  		fprintf(stderr, "nfit_test unavailable skipping tests\n");
> diff --git a/test/multi-pmem.c b/test/multi-pmem.c
> index 3d10952..3ea08cc 100644
> --- a/test/multi-pmem.c
> +++ b/test/multi-pmem.c
> @@ -249,7 +249,7 @@ int test_multi_pmem(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx
>  
> 
>  	ndctl_set_log_priority(ctx, loglevel);
>  
> 
> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>  	if (err < 0) {
>  		result = 77;
>  		ndctl_test_skip(test);
> diff --git a/test/parent-uuid.c b/test/parent-uuid.c
> index 6424e9f..bded33a 100644
> --- a/test/parent-uuid.c
> +++ b/test/parent-uuid.c
> @@ -218,7 +218,7 @@ int test_parent_uuid(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ct
>  		return 77;
>  
> 
>  	ndctl_set_log_priority(ctx, loglevel);
> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>  	if (err < 0) {
>  		ndctl_test_skip(test);
>  		fprintf(stderr, "nfit_test unavailable skipping tests\n");
> diff --git a/test/pmem_namespaces.c b/test/pmem_namespaces.c
> index f0f2edd..a4db1ae 100644
> --- a/test/pmem_namespaces.c
> +++ b/test/pmem_namespaces.c
> @@ -191,7 +191,7 @@ int test_pmem_namespaces(int log_level, struct ndctl_test *test,
>  
> 
>  	if (!bus) {
>  		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
> -		rc = nfit_test_init(&kmod_ctx, &mod, NULL, log_level, test);
> +		rc = ndctl_test_init(&kmod_ctx, &mod, NULL, log_level, test);
>  		ndctl_invalidate(ctx);
>  		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
>  		if (rc < 0 || !bus) {


_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 3/4] papr: Add support to parse save_fail flag for dimm

[Verma, Vishal L]

On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
> This will help in getting the dimm fail tests to run on papr family too.
> Also add nvdimm_test compatibility string for recognizing the test module.
> 
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
>  ndctl/lib/libndctl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 26b9317..dd1a5fc 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -805,6 +805,8 @@ static void parse_papr_flags(struct ndctl_dimm *dimm, char *flags)
>  			dimm->flags.f_restore = 1;
>  		else if (strcmp(start, "smart_notify") == 0)
>  			dimm->flags.f_smart = 1;
> +		else if (strcmp(start, "save_fail") == 0)
> +			dimm->flags.f_save = 1;
>  		start = end + 1;
>  	}
>  	if (end != start)
> @@ -1035,7 +1037,8 @@ NDCTL_EXPORT int ndctl_bus_is_papr_scm(struct ndctl_bus *bus)
>  	if (sysfs_read_attr(bus->ctx, bus->bus_buf, buf) < 0)
>  		return 0;
> 
> -	return (strcmp(buf, "ibm,pmemory") == 0);
> +	return (strcmp(buf, "ibm,pmemory") == 0 ||
> +		strcmp(buf, "nvdimm_test") == 0);

I'm guessing this name comes from the kernel? It would be nice to make
it symmetrical with 'nfit_test' by calling the bus 'papr_test' maybe,
but no worries if it is too late for that.

>  }
> 
>  /**

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing

[Santosh Sivaraj]

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
>> For NFIT to be available ACPI is a must, so don't fail when nfit modules
>> are missing on a platform that doesn't support ACPI.
>> 
>> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
>> ---
>>  test.h                        |  2 +-
>>  test/ack-shutdown-count-set.c |  2 +-
>>  test/blk_namespaces.c         |  2 +-
>>  test/core.c                   | 23 +++++++++++++++++++++--
>>  test/dpa-alloc.c              |  2 +-
>>  test/dsm-fail.c               |  2 +-
>>  test/libndctl.c               |  2 +-
>>  test/multi-pmem.c             |  2 +-
>>  test/parent-uuid.c            |  2 +-
>>  test/pmem_namespaces.c        |  2 +-
>>  10 files changed, 30 insertions(+), 11 deletions(-)
>> 
>> diff --git a/test.h b/test.h
>> index cba8d41..7de13fe 100644
>> --- a/test.h
>> +++ b/test.h
>> @@ -20,7 +20,7 @@ void builtin_xaction_namespace_reset(void);
>>  
>> 
>>  struct kmod_ctx;
>>  struct kmod_module;
>> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>>  		struct ndctl_ctx *nd_ctx, int log_level,
>>  		struct ndctl_test *test);
>>  
>> 
>> diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
>> index fb1d82b..c561ff3 100644
>> --- a/test/ack-shutdown-count-set.c
>> +++ b/test/ack-shutdown-count-set.c
>> @@ -99,7 +99,7 @@ static int test_ack_shutdown_count_set(int loglevel, struct ndctl_test *test,
>>  	int result = EXIT_FAILURE, err;
>>  
>> 
>>  	ndctl_set_log_priority(ctx, loglevel);
>> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>>  	if (err < 0) {
>>  		result = 77;
>>  		ndctl_test_skip(test);
>> diff --git a/test/blk_namespaces.c b/test/blk_namespaces.c
>> index d7f00cb..f076e85 100644
>> --- a/test/blk_namespaces.c
>> +++ b/test/blk_namespaces.c
>> @@ -228,7 +228,7 @@ int test_blk_namespaces(int log_level, struct ndctl_test *test,
>>  
>> 
>>  	if (!bus) {
>>  		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
>> -		rc = nfit_test_init(&kmod_ctx, &mod, NULL, log_level, test);
>> +		rc = ndctl_test_init(&kmod_ctx, &mod, NULL, log_level, test);
>>  		ndctl_invalidate(ctx);
>>  		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
>>  		if (rc < 0 || !bus) {
>> diff --git a/test/core.c b/test/core.c
>> index cc7d8d9..903034a 100644
>> --- a/test/core.c
>> +++ b/test/core.c
>> @@ -11,6 +11,7 @@
>>  #include <util/log.h>
>>  #include <util/sysfs.h>
>>  #include <ndctl/libndctl.h>
>> +#include <ndctl/ndctl.h>
>>  #include <ccan/array_size/array_size.h>
>>  
>> 
>>  #define KVER_STRLEN 20
>> @@ -106,11 +107,11 @@ int ndctl_test_get_skipped(struct ndctl_test *test)
>>  	return test->skip;
>>  }
>>  
>> 
>> -int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>> +int ndctl_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>>  		struct ndctl_ctx *nd_ctx, int log_level,
>>  		struct ndctl_test *test)
>>  {
>> -	int rc;
>> +	int rc, family = NVDIMM_FAMILY_INTEL;
>>  	unsigned int i;
>>  	const char *name;
>>  	struct ndctl_bus *bus;
>> @@ -127,6 +128,19 @@ int nfit_test_init(struct kmod_ctx **ctx, struct kmod_module **mod,
>>  		"nd_e820",
>>  		"nd_pmem",
>>  	};
>> +	char *test_env;
>> +
>> +	/* Do we want to force test PAPR? */
>> +	test_env = getenv("NDCTL_TEST_FAMILY");
>> +	if (test_env && strcmp(test_env, "PAPR") == 0)
>> +		family = NVDIMM_FAMILY_PAPR;
>> +
>> +	/* ACPI is a must for nfit, so if ACPI is not available let's default to
>> +	 * PAPR */
>
> fix multi line comment to the right formatting:
> /*
>  * line 1, etc
>  */
>

Will fix that.

>> +	if (access("/sys/bus/acpi", F_OK) == -1) {
>> +		if (errno == ENOENT)
>> +			family = NVDIMM_FAMILY_PAPR;
>> +	}
>
> Instead of a blind default, can we perform a similar check for presence of
> PAPR too?
>

Yes, I wanted to do that, but there is no reliable way of check that; there is
no ofnode before module load, and there won't be any PAPR specific DT entries if
the platform is not Power.

I also test the 'ndtest' module on x86 with NDCTL_TEST_FAMILY environment
variable. I can let the default be nfit_test (NVDIMM_FAMILY_INTEL) and only load
PAPR module when the environment variable is set. Thoughts?

Thanks,
Santosh

>>  
>> 
>>  	log_init(&log_ctx, "test/init", "NDCTL_TEST");
>>  	log_ctx.log_priority = log_level;
>> @@ -185,6 +199,11 @@ retry:
>>  
>> 
>>  		path = kmod_module_get_path(*mod);
>>  		if (!path) {
>> +			if (family == NVDIMM_FAMILY_PAPR &&
>> +			    (strcmp(name, "nfit") == 0 ||
>> +			     strcmp(name, "nd_e820") == 0))
>> +				continue;
>> +
>>  			log_err(&log_ctx, "%s.ko: failed to get path\n", name);
>>  			break;
>>  		}
>> diff --git a/test/dpa-alloc.c b/test/dpa-alloc.c
>> index e922009..0b3bb7a 100644
>> --- a/test/dpa-alloc.c
>> +++ b/test/dpa-alloc.c
>> @@ -289,7 +289,7 @@ int test_dpa_alloc(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
>>  		return 77;
>>  
>> 
>>  	ndctl_set_log_priority(ctx, loglevel);
>> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>>  	if (err < 0) {
>>  		ndctl_test_skip(test);
>>  		fprintf(stderr, "nfit_test unavailable skipping tests\n");
>> diff --git a/test/dsm-fail.c b/test/dsm-fail.c
>> index 9dfd8b0..0a6383d 100644
>> --- a/test/dsm-fail.c
>> +++ b/test/dsm-fail.c
>> @@ -346,7 +346,7 @@ int test_dsm_fail(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
>>  	int result = EXIT_FAILURE, err;
>>  
>> 
>>  	ndctl_set_log_priority(ctx, loglevel);
>> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>>  	if (err < 0) {
>>  		result = 77;
>>  		ndctl_test_skip(test);
>> diff --git a/test/libndctl.c b/test/libndctl.c
>> index 24d72b3..0e88fce 100644
>> --- a/test/libndctl.c
>> +++ b/test/libndctl.c
>> @@ -2692,7 +2692,7 @@ int test_libndctl(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx)
>>  	daxctl_set_log_priority(daxctl_ctx, loglevel);
>>  	ndctl_set_private_data(ctx, test);
>>  
>> 
>> -	err = nfit_test_init(&kmod_ctx, &mod, ctx, loglevel, test);
>> +	err = ndctl_test_init(&kmod_ctx, &mod, ctx, loglevel, test);
>>  	if (err < 0) {
>>  		ndctl_test_skip(test);
>>  		fprintf(stderr, "nfit_test unavailable skipping tests\n");
>> diff --git a/test/multi-pmem.c b/test/multi-pmem.c
>> index 3d10952..3ea08cc 100644
>> --- a/test/multi-pmem.c
>> +++ b/test/multi-pmem.c
>> @@ -249,7 +249,7 @@ int test_multi_pmem(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ctx
>>  
>> 
>>  	ndctl_set_log_priority(ctx, loglevel);
>>  
>> 
>> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>>  	if (err < 0) {
>>  		result = 77;
>>  		ndctl_test_skip(test);
>> diff --git a/test/parent-uuid.c b/test/parent-uuid.c
>> index 6424e9f..bded33a 100644
>> --- a/test/parent-uuid.c
>> +++ b/test/parent-uuid.c
>> @@ -218,7 +218,7 @@ int test_parent_uuid(int loglevel, struct ndctl_test *test, struct ndctl_ctx *ct
>>  		return 77;
>>  
>> 
>>  	ndctl_set_log_priority(ctx, loglevel);
>> -	err = nfit_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>> +	err = ndctl_test_init(&kmod_ctx, &mod, NULL, loglevel, test);
>>  	if (err < 0) {
>>  		ndctl_test_skip(test);
>>  		fprintf(stderr, "nfit_test unavailable skipping tests\n");
>> diff --git a/test/pmem_namespaces.c b/test/pmem_namespaces.c
>> index f0f2edd..a4db1ae 100644
>> --- a/test/pmem_namespaces.c
>> +++ b/test/pmem_namespaces.c
>> @@ -191,7 +191,7 @@ int test_pmem_namespaces(int log_level, struct ndctl_test *test,
>>  
>> 
>>  	if (!bus) {
>>  		fprintf(stderr, "ACPI.NFIT unavailable falling back to nfit_test\n");
>> -		rc = nfit_test_init(&kmod_ctx, &mod, NULL, log_level, test);
>> +		rc = ndctl_test_init(&kmod_ctx, &mod, NULL, log_level, test);
>>  		ndctl_invalidate(ctx);
>>  		bus = ndctl_bus_get_by_provider(ctx, "nfit_test.0");
>>  		if (rc < 0 || !bus) {
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 1/4] libndctl: test enablement for non-nfit devices

[Santosh Sivaraj]

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

Hi Vishal,

> On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
>> Unify adding dimms for papr and nfit families, this will help in adding
>
> Minor nit, but it seems like the subject line and the first sentence in
> the body should be swapped. The one-line description of what's happening
> in this patch is "Unify adding dimms for papr and nfit families", and
> the body can go into more detail about why - i.e. in preparation for
> enabling tests on non-nfit devices.
>

Agree.

>> all attributes needed for the unit tests too. We don't fail adding a dimm
>> if some of the dimm attributes are missing, so this will work fine on PAPR
>> platforms where most dimm attributes are provided.
>
> Does this mean 'most - but not all'? Might be a good clarification to
> make here.

Yes, that's right. I will fix that up in the next version.

Thanks,
Santosh
>
>> 
>> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
>> ---
>>  ndctl/lib/libndctl.c | 103 ++++++++++++++++---------------------------
>>  1 file changed, 38 insertions(+), 65 deletions(-)
>> 
>> v3:
>> * Drop patch which skips SMART tests, smart test enablement will be posted
>>   soon.
>> 
>> v2:
>> * Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
>>   INTEL family instead. That change was there to test the same code on x86, but
>>   accidently committed. Now have a environment variable to force test PAPR
>>   family on x86.
>> 
>> * Patch 4: Remove stray code, artifact of refactoring in patch 1.
>> 
>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
>> index 36fb6fe..26b9317 100644
>> --- a/ndctl/lib/libndctl.c
>> +++ b/ndctl/lib/libndctl.c
>> @@ -1646,41 +1646,9 @@ static int ndctl_bind(struct ndctl_ctx *ctx, struct kmod_module *module,
>>  static int ndctl_unbind(struct ndctl_ctx *ctx, const char *devpath);
>>  static struct kmod_module *to_module(struct ndctl_ctx *ctx, const char *alias);
>>  
>> -static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>> -{
>> -	int rc = -ENODEV;
>> -	char buf[SYSFS_ATTR_SIZE];
>> -	struct ndctl_ctx *ctx = dimm->bus->ctx;
>> -	char *path = calloc(1, strlen(dimm_base) + 100);
>> -	const char * const devname = ndctl_dimm_get_devname(dimm);
>> -
>> -	dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
>> -
>> -	if (!path)
>> -		return -ENOMEM;
>> -
>> -	/* construct path to the papr compatible dimm flags file */
>> -	sprintf(path, "%s/papr/flags", dimm_base);
>> -
>> -	if (ndctl_bus_is_papr_scm(dimm->bus) &&
>> -	    sysfs_read_attr(ctx, path, buf) == 0) {
>> -
>> -		dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname, buf);
>> -		dimm->cmd_family = NVDIMM_FAMILY_PAPR;
>> -
>> -		/* Parse dimm flags */
>> -		parse_papr_flags(dimm, buf);
>> -
>> -		/* Allocate monitor mode fd */
>> -		dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
>> -		rc = 0;
>> -	}
>> -
>> -	free(path);
>> -	return rc;
>> -}
>> -
>> -static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>> +static int populate_dimm_attributes(struct ndctl_dimm *dimm,
>> +				    const char *dimm_base,
>> +				    const char *bus_prefix)
>>  {
>>  	int i, rc = -1;
>>  	char buf[SYSFS_ATTR_SIZE];
>> @@ -1694,7 +1662,7 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>>  	 * 'unique_id' may not be available on older kernels, so don't
>>  	 * fail if the read fails.
>>  	 */
>> -	sprintf(path, "%s/nfit/id", dimm_base);
>> +	sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0) {
>>  		unsigned int b[9];
>> 
>> @@ -1709,68 +1677,74 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>>  		}
>>  	}
>> 
>> -	sprintf(path, "%s/nfit/handle", dimm_base);
>> +	sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		goto err_read;
>>  	dimm->handle = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/phys_id", dimm_base);
>> +	sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		goto err_read;
>>  	dimm->phys_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/serial", dimm_base);
>> +	sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->serial = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/vendor", dimm_base);
>> +	sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->vendor_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/device", dimm_base);
>> +	sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->device_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/rev_id", dimm_base);
>> +	sprintf(path, "%s/%s/rev_id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->revision_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/dirty_shutdown", dimm_base);
>> +	sprintf(path, "%s/%s/dirty_shutdown", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->dirty_shutdown = strtoll(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/subsystem_vendor", dimm_base);
>> +	sprintf(path, "%s/%s/subsystem_vendor", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/subsystem_device", dimm_base);
>> +	sprintf(path, "%s/%s/subsystem_device", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->subsystem_device_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/subsystem_rev_id", dimm_base);
>> +	sprintf(path, "%s/%s/subsystem_rev_id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->subsystem_revision_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/family", dimm_base);
>> +	sprintf(path, "%s/%s/family", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->cmd_family = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
>> +	sprintf(path, "%s/%s/dsm_mask", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/format", dimm_base);
>> +	sprintf(path, "%s/%s/format", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->format[0] = strtoul(buf, NULL, 0);
>>  	for (i = 1; i < dimm->formats; i++) {
>> -		sprintf(path, "%s/nfit/format%d", dimm_base, i);
>> +		sprintf(path, "%s/%s/format%d", dimm_base, bus_prefix, i);
>>  		if (sysfs_read_attr(ctx, path, buf) == 0)
>>  			dimm->format[i] = strtoul(buf, NULL, 0);
>>  	}
>>  
>> -	sprintf(path, "%s/nfit/flags", dimm_base);
>> -	if (sysfs_read_attr(ctx, path, buf) == 0)
>> -		parse_nfit_mem_flags(dimm, buf);
>> +	sprintf(path, "%s/%s/flags", dimm_base, bus_prefix);
>> +	if (sysfs_read_attr(ctx, path, buf) == 0) {
>> +		if (ndctl_bus_has_nfit(dimm->bus))
>> +			parse_nfit_mem_flags(dimm, buf);
>> +		else if (ndctl_bus_is_papr_scm(dimm->bus)) {
>> +			dimm->cmd_family = NVDIMM_FAMILY_PAPR;
>> +			parse_papr_flags(dimm, buf);
>> +		}
>> +	}
>>  
>>  	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
>>  	rc = 0;
>> @@ -1792,7 +1766,8 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>>  	if (!path)
>>  		return NULL;
>>  
>> -	sprintf(path, "%s/nfit/formats", dimm_base);
>> +	sprintf(path, "%s/%s/formats", dimm_base,
>> +		ndctl_bus_has_nfit(bus) ? "nfit" : "papr");
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		formats = 1;
>>  	else
>> @@ -1866,13 +1841,12 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>>  	else
>>  		dimm->fwa_result = fwa_result_to_result(buf);
>>  
>> +	dimm->formats = formats;
>>  	/* Check if the given dimm supports nfit */
>>  	if (ndctl_bus_has_nfit(bus)) {
>> -		dimm->formats = formats;
>> -		rc = add_nfit_dimm(dimm, dimm_base);
>> -	} else if (ndctl_bus_has_of_node(bus)) {
>> -		rc = add_papr_dimm(dimm, dimm_base);
>> -	}
>> +		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
>> +	} else if (ndctl_bus_has_of_node(bus))
>> +		rc = populate_dimm_attributes(dimm, dimm_base, "papr");
>>  
>>  	if (rc == -ENODEV) {
>>  		/* Unprobed dimm with no family */
>> @@ -2531,13 +2505,12 @@ static void *add_region(void *parent, int id, const char *region_base)
>>  		goto err_read;
>>  	region->num_mappings = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/range_index", region_base);
>> -	if (ndctl_bus_has_nfit(bus)) {
>> -		if (sysfs_read_attr(ctx, path, buf) < 0)
>> -			goto err_read;
>> -		region->range_index = strtoul(buf, NULL, 0);
>> -	} else
>> +	sprintf(path, "%s/%s/range_index", region_base,
>> +		ndctl_bus_has_nfit(bus) ? "nfit": "papr");
>> +	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		region->range_index = -1;
>> +	else
>> +		region->range_index = strtoul(buf, NULL, 0);
>>  
>>  	sprintf(path, "%s/read_only", region_base);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing

[Verma, Vishal L]

On Fri, 2021-03-19 at 11:20 +0530, Santosh Sivaraj wrote:
> "Verma, Vishal L" <vishal.l.verma@intel.com> writes:

[..]

> > 
> > fix multi line comment to the right formatting:
> > /*
> >  * line 1, etc
> >  */
> > 
> 
> Will fix that.
> 
> > > +	if (access("/sys/bus/acpi", F_OK) == -1) {
> > > +		if (errno == ENOENT)
> > > +			family = NVDIMM_FAMILY_PAPR;
> > > +	}
> > 
> > Instead of a blind default, can we perform a similar check for presence of
> > PAPR too?
> > 
> 
> Yes, I wanted to do that, but there is no reliable way of check that; there is
> no ofnode before module load, and there won't be any PAPR specific DT entries if
> the platform is not Power.
> 
> I also test the 'ndtest' module on x86 with NDCTL_TEST_FAMILY environment
> variable. I can let the default be nfit_test (NVDIMM_FAMILY_INTEL) and only load
> PAPR module when the environment variable is set. Thoughts?
> 
The env variable seems reasonable to me. If there is ever a third
'family' adding tests, having an arbitrary default might be awkward.
I may suggest - if acpi is detected, use NFIT. If env has something that
is known, e.g. PAPR, use that. If env is unset or doesn't match anything
we know about, then bail with an error message. Does that sound
reasonable?
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v3 2/4] test: Don't skip tests if nfit modules are missing

[Santosh Sivaraj]

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Fri, 2021-03-19 at 11:20 +0530, Santosh Sivaraj wrote:
>> "Verma, Vishal L" <vishal.l.verma@intel.com> writes:
>
> [..]
>
>> > 
>> > fix multi line comment to the right formatting:
>> > /*
>> >  * line 1, etc
>> >  */
>> > 
>> 
>> Will fix that.
>> 
>> > > +	if (access("/sys/bus/acpi", F_OK) == -1) {
>> > > +		if (errno == ENOENT)
>> > > +			family = NVDIMM_FAMILY_PAPR;
>> > > +	}
>> > 
>> > Instead of a blind default, can we perform a similar check for presence of
>> > PAPR too?
>> > 
>> 
>> Yes, I wanted to do that, but there is no reliable way of check that; there is
>> no ofnode before module load, and there won't be any PAPR specific DT entries if
>> the platform is not Power.
>> 
>> I also test the 'ndtest' module on x86 with NDCTL_TEST_FAMILY environment
>> variable. I can let the default be nfit_test (NVDIMM_FAMILY_INTEL) and only load
>> PAPR module when the environment variable is set. Thoughts?
>> 
> The env variable seems reasonable to me. If there is ever a third
> 'family' adding tests, having an arbitrary default might be awkward.
> I may suggest - if acpi is detected, use NFIT. If env has something that
> is known, e.g. PAPR, use that. If env is unset or doesn't match anything
> we know about, then bail with an error message. Does that sound
> reasonable?

Yes, that sounds too to me. I will send in the next version soon.
Thanks for the review!

Thanks,
Santosh
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org