diff mbox series

[ndctl,RFC,v4,2/3] Skip smart tests when non nfit devices present

Message ID 20201108122016.2090891-2-santosh@fossix.org (mailing list archive)
State New
Headers show
Series [ndctl,RFC,v4,1/3] libndctl: test enablement for non-nfit devices | expand

Commit Message

Santosh Sivaraj Nov. 8, 2020, 12:20 p.m. UTC
Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
---
 ndctl/lib/libndctl.c | 34 ++++++++++++++++++++++++++--------
 test/libndctl.c      |  3 ++-
 2 files changed, 28 insertions(+), 9 deletions(-)

Comments

Dan Williams Dec. 7, 2020, 11:24 p.m. UTC | #1
On Sun, Nov 8, 2020 at 4:20 AM Santosh Sivaraj <santosh@fossix.org> wrote:
>

-ENOCOMMITMESSAGE

Also this patch is doing more than just skipping SMART tests, which
would have been noticed by writing a commit message, and which
probably would have prompted those changes to move to their own
patch... with their own commit message of course.

> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
>  ndctl/lib/libndctl.c | 34 ++++++++++++++++++++++++++--------
>  test/libndctl.c      |  3 ++-
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index d1f8e4e..26fc14c 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -815,8 +815,11 @@ 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)
>                 dbg(ctx, "%s: Flags:%s\n", ndctl_dimm_get_devname(dimm), flags);
>  }
> @@ -1044,7 +1047,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);
>  }
>
>  /**
> @@ -1661,6 +1665,7 @@ static void populate_dimm_attributes(struct ndctl_dimm *dimm,
>         char buf[SYSFS_ATTR_SIZE];
>         struct ndctl_ctx *ctx = dimm->bus->ctx;
>         char *path = calloc(1, strlen(dimm_base) + 100);
> +       int i;
>
>         sprintf(path, "%s/phys_id", dimm_base);
>         if (sysfs_read_attr(ctx, path, buf) < 0)
> @@ -1690,6 +1695,16 @@ static void populate_dimm_attributes(struct ndctl_dimm *dimm,
>                         dimm->manufacturing_location = b[2];
>                 }
>         }
> +
> +       sprintf(path, "%s/format", dimm_base);
> +       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/format%d", dimm_base, i);
> +               if (sysfs_read_attr(ctx, path, buf) == 0)
> +                       dimm->format[i] = strtoul(buf, NULL, 0);
> +       }

The "format" attribute is this strange artifact of the ACPI NFIT
definition. You've already done the work to emulate it, but I wish I
could have saved you from that diversion because I don't see the point
for papr, especially just to make a unit test happy.

The "format" was related to a JEDEC designation IIRC, but it has no
practical implication on the kernel or ndctl. It was only 3rd party
management software that thought they needed this, but even then I
think the justification was dubious.

> +
>         sprintf(path, "%s/subsystem_vendor", dimm_base);
>         if (sysfs_read_attr(ctx, path, buf) == 0)
>                 dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
> @@ -1853,7 +1868,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" : "");
>         if (sysfs_read_attr(ctx, path, buf) < 0)
>                 formats = 1;
>         else
> @@ -1927,9 +1943,9 @@ 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);
> @@ -2592,13 +2608,15 @@ 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)
> +       sprintf(path, "%s%s/range_index", region_base,
> +               ndctl_bus_has_nfit(bus) ? "/nfit" : "");
> +       if (sysfs_read_attr(ctx, path, buf) < 0) {
> +               if (ndctl_bus_has_nfit(bus))
>                         goto err_read;
> -               region->range_index = strtoul(buf, NULL, 0);
> +               else
> +                       region->range_index = -1;
>         } else
> -               region->range_index = -1;
> +               region->range_index = strtoul(buf, NULL, 0);
>
>         sprintf(path, "%s/read_only", region_base);
>         if (sysfs_read_attr(ctx, path, buf) < 0)
> diff --git a/test/libndctl.c b/test/libndctl.c
> index 994e0fa..b7e7b68 100644
> --- a/test/libndctl.c
> +++ b/test/libndctl.c
> @@ -2427,7 +2427,8 @@ static int check_commands(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
>          * The kernel did not start emulating v1.2 namespace spec smart data
>          * until 4.9.
>          */
> -       if (!ndctl_test_attempt(test, KERNEL_VERSION(4, 9, 0)))
> +       if (!ndctl_test_attempt(test, KERNEL_VERSION(4, 9, 0))
> +           || !ndctl_bus_has_nfit(bus))
>                 dimm_commands &= ~((1 << ND_CMD_SMART)
>                                 | (1 << ND_CMD_SMART_THRESHOLD));


I was going to say split test/libndctl.c into a generic test and and
nfit-specific test/libndctl-nfit.c, but if the vast majority of the
tests runs with just small tweaks like this I think I can do that
refactoring later.
diff mbox series

Patch

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index d1f8e4e..26fc14c 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -815,8 +815,11 @@  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)
 		dbg(ctx, "%s: Flags:%s\n", ndctl_dimm_get_devname(dimm), flags);
 }
@@ -1044,7 +1047,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);
 }
 
 /**
@@ -1661,6 +1665,7 @@  static void populate_dimm_attributes(struct ndctl_dimm *dimm,
 	char buf[SYSFS_ATTR_SIZE];
 	struct ndctl_ctx *ctx = dimm->bus->ctx;
 	char *path = calloc(1, strlen(dimm_base) + 100);
+	int i;
 
 	sprintf(path, "%s/phys_id", dimm_base);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
@@ -1690,6 +1695,16 @@  static void populate_dimm_attributes(struct ndctl_dimm *dimm,
 			dimm->manufacturing_location = b[2];
 		}
 	}
+
+	sprintf(path, "%s/format", dimm_base);
+	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/format%d", dimm_base, i);
+		if (sysfs_read_attr(ctx, path, buf) == 0)
+			dimm->format[i] = strtoul(buf, NULL, 0);
+	}
+
 	sprintf(path, "%s/subsystem_vendor", dimm_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
@@ -1853,7 +1868,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" : "");
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		formats = 1;
 	else
@@ -1927,9 +1943,9 @@  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);
@@ -2592,13 +2608,15 @@  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)
+	sprintf(path, "%s%s/range_index", region_base,
+		ndctl_bus_has_nfit(bus) ? "/nfit" : "");
+	if (sysfs_read_attr(ctx, path, buf) < 0) {
+		if (ndctl_bus_has_nfit(bus))
 			goto err_read;
-		region->range_index = strtoul(buf, NULL, 0);
+		else
+			region->range_index = -1;
 	} else
-		region->range_index = -1;
+		region->range_index = strtoul(buf, NULL, 0);
 
 	sprintf(path, "%s/read_only", region_base);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
diff --git a/test/libndctl.c b/test/libndctl.c
index 994e0fa..b7e7b68 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -2427,7 +2427,8 @@  static int check_commands(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	 * The kernel did not start emulating v1.2 namespace spec smart data
 	 * until 4.9.
 	 */
-	if (!ndctl_test_attempt(test, KERNEL_VERSION(4, 9, 0)))
+	if (!ndctl_test_attempt(test, KERNEL_VERSION(4, 9, 0))
+	    || !ndctl_bus_has_nfit(bus))
 		dimm_commands &= ~((1 << ND_CMD_SMART)
 				| (1 << ND_CMD_SMART_THRESHOLD));