| Message ID | 20201108122016.2090891-2-santosh@fossix.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndctl,RFC,v4,1/3] libndctl: test enablement for non-nfit devices | expand |
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 --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));
Signed-off-by: Santosh Sivaraj <santosh@fossix.org> --- ndctl/lib/libndctl.c | 34 ++++++++++++++++++++++++++-------- test/libndctl.c | 3 ++- 2 files changed, 28 insertions(+), 9 deletions(-)