| Message ID | 20201108122016.2090891-1-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:21 AM Santosh Sivaraj <santosh@fossix.org> wrote: > > Don't fail is nfit module is missing, this will happen in > platforms that don't have ACPI support. Add attributes to > PAPR dimm family that are independent of platforms like the > test dimms. > > Signed-off-by: Santosh Sivaraj <santosh@fossix.org> > --- > ndctl/lib/libndctl.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ > test/core.c | 6 +++++ > 2 files changed, 58 insertions(+) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index ad521d3..d1f8e4e 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -1655,6 +1655,54 @@ 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 void populate_dimm_attributes(struct ndctl_dimm *dimm, > + const char *dimm_base) > +{ > + char buf[SYSFS_ATTR_SIZE]; > + struct ndctl_ctx *ctx = dimm->bus->ctx; > + char *path = calloc(1, strlen(dimm_base) + 100); > + > + sprintf(path, "%s/phys_id", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) < 0) > + goto err_read; > + dimm->phys_id = strtoul(buf, NULL, 0); > + > + sprintf(path, "%s/handle", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) < 0) > + goto err_read; > + dimm->handle = strtoul(buf, NULL, 0); > + > + sprintf(path, "%s/vendor", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) == 0) > + dimm->vendor_id = strtoul(buf, NULL, 0); > + > + sprintf(path, "%s/id", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) == 0) { > + unsigned int b[9]; > + > + dimm->unique_id = strdup(buf); > + if (!dimm->unique_id) > + goto err_read; > + if (sscanf(dimm->unique_id, "%02x%02x-%02x-%02x%02x-%02x%02x%02x%02x", > + &b[0], &b[1], &b[2], &b[3], &b[4], > + &b[5], &b[6], &b[7], &b[8]) == 9) { > + dimm->manufacturing_date = b[3] << 8 | b[4]; > + dimm->manufacturing_location = b[2]; > + } > + } > + sprintf(path, "%s/subsystem_vendor", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) == 0) > + dimm->subsystem_vendor_id = strtoul(buf, NULL, 0); > + > + > + sprintf(path, "%s/dirty_shutdown", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) == 0) > + dimm->dirty_shutdown = strtoll(buf, NULL, 0); These are fairly similar to the nfit ones... how about refactoring this into a routine that takes a bus prefix and shares it between "nfit" and "papr"... We might also consider unifying them into a standard set of attributes that both the nfit-bus-provider and the papr-bus-provider export. I.e. that nfit was wrong to place them under nfit/ and they should have went somewhere generic from the beginning. The nfit compatibility can be done with symlinks to the new common location. > + > +err_read: > + free(path); > +} > + > static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base) > { > int rc = -ENODEV; > @@ -1685,6 +1733,10 @@ static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base) > rc = 0; > } > > + /* add the available dimm attributes, the platform can override or add > + * additional attributes later */ > + populate_dimm_attributes(dimm, dimm_base); > + > free(path); > return rc; > } > diff --git a/test/core.c b/test/core.c > index 5118d86..0fd1011 100644 > --- a/test/core.c > +++ b/test/core.c > @@ -195,6 +195,12 @@ retry: > > path = kmod_module_get_path(*mod); > if (!path) { > + /* For non-nfit platforms it's ok if nfit module is > + * missing */ > + if (strcmp(name, "nfit") == 0 || > + strcmp(name, "nd_e820") == 0) > + continue; This breaks the safety it afforded on nfit platforms. Instead I think this needs a couple changes: - rename nfit_test_init to ndctl_test_init - add a parameter for whether the test is initializing for nfit_test.ko or ndtest.ko.
Dan Williams <dan.j.williams@intel.com> writes: > On Sun, Nov 8, 2020 at 4:21 AM Santosh Sivaraj <santosh@fossix.org> wrote: >> >> Don't fail is nfit module is missing, this will happen in >> platforms that don't have ACPI support. Add attributes to >> PAPR dimm family that are independent of platforms like the >> test dimms. >> >> Signed-off-by: Santosh Sivaraj <santosh@fossix.org> >> --- >> ndctl/lib/libndctl.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ >> test/core.c | 6 +++++ >> 2 files changed, 58 insertions(+) >> >> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c >> index ad521d3..d1f8e4e 100644 >> --- a/ndctl/lib/libndctl.c >> +++ b/ndctl/lib/libndctl.c >> @@ -1655,6 +1655,54 @@ 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 void populate_dimm_attributes(struct ndctl_dimm *dimm, >> + const char *dimm_base) >> +{ >> + char buf[SYSFS_ATTR_SIZE]; >> + struct ndctl_ctx *ctx = dimm->bus->ctx; >> + char *path = calloc(1, strlen(dimm_base) + 100); >> + >> + sprintf(path, "%s/phys_id", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) < 0) >> + goto err_read; >> + dimm->phys_id = strtoul(buf, NULL, 0); >> + >> + sprintf(path, "%s/handle", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) < 0) >> + goto err_read; >> + dimm->handle = strtoul(buf, NULL, 0); >> + >> + sprintf(path, "%s/vendor", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) == 0) >> + dimm->vendor_id = strtoul(buf, NULL, 0); >> + >> + sprintf(path, "%s/id", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) == 0) { >> + unsigned int b[9]; >> + >> + dimm->unique_id = strdup(buf); >> + if (!dimm->unique_id) >> + goto err_read; >> + if (sscanf(dimm->unique_id, "%02x%02x-%02x-%02x%02x-%02x%02x%02x%02x", >> + &b[0], &b[1], &b[2], &b[3], &b[4], >> + &b[5], &b[6], &b[7], &b[8]) == 9) { >> + dimm->manufacturing_date = b[3] << 8 | b[4]; >> + dimm->manufacturing_location = b[2]; >> + } >> + } >> + sprintf(path, "%s/subsystem_vendor", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) == 0) >> + dimm->subsystem_vendor_id = strtoul(buf, NULL, 0); >> + >> + >> + sprintf(path, "%s/dirty_shutdown", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) == 0) >> + dimm->dirty_shutdown = strtoll(buf, NULL, 0); > > These are fairly similar to the nfit ones... how about refactoring > this into a routine that takes a bus prefix and shares it between > "nfit" and "papr"... > > We might also consider unifying them into a standard set of attributes > that both the nfit-bus-provider and the papr-bus-provider export. I.e. > that nfit was wrong to place them under nfit/ and they should have > went somewhere generic from the beginning. The nfit compatibility can > be done with symlinks to the new common location. > >> + >> +err_read: >> + free(path); >> +} >> + >> static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base) >> { >> int rc = -ENODEV; >> @@ -1685,6 +1733,10 @@ static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base) >> rc = 0; >> } >> >> + /* add the available dimm attributes, the platform can override or add >> + * additional attributes later */ >> + populate_dimm_attributes(dimm, dimm_base); >> + >> free(path); >> return rc; >> } >> diff --git a/test/core.c b/test/core.c >> index 5118d86..0fd1011 100644 >> --- a/test/core.c >> +++ b/test/core.c >> @@ -195,6 +195,12 @@ retry: >> >> path = kmod_module_get_path(*mod); >> if (!path) { >> + /* For non-nfit platforms it's ok if nfit module is >> + * missing */ >> + if (strcmp(name, "nfit") == 0 || >> + strcmp(name, "nd_e820") == 0) >> + continue; > > This breaks the safety it afforded on nfit platforms. Instead I think > this needs a couple changes: > > - rename nfit_test_init to ndctl_test_init > - add a parameter for whether the test is initializing for > nfit_test.ko or ndtest.ko. Thanks for review Dan. I will make changes accordingly and send newer version without -ENOCOMMITMESSAGE error. Since I posted this only for the comments on the approach I took, I think I got how to proceed in terms on ndctl changes atleast. I will send a v5 soon enough. Thanks, Santosh
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index ad521d3..d1f8e4e 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -1655,6 +1655,54 @@ 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 void populate_dimm_attributes(struct ndctl_dimm *dimm, + const char *dimm_base) +{ + char buf[SYSFS_ATTR_SIZE]; + struct ndctl_ctx *ctx = dimm->bus->ctx; + char *path = calloc(1, strlen(dimm_base) + 100); + + sprintf(path, "%s/phys_id", dimm_base); + if (sysfs_read_attr(ctx, path, buf) < 0) + goto err_read; + dimm->phys_id = strtoul(buf, NULL, 0); + + sprintf(path, "%s/handle", dimm_base); + if (sysfs_read_attr(ctx, path, buf) < 0) + goto err_read; + dimm->handle = strtoul(buf, NULL, 0); + + sprintf(path, "%s/vendor", dimm_base); + if (sysfs_read_attr(ctx, path, buf) == 0) + dimm->vendor_id = strtoul(buf, NULL, 0); + + sprintf(path, "%s/id", dimm_base); + if (sysfs_read_attr(ctx, path, buf) == 0) { + unsigned int b[9]; + + dimm->unique_id = strdup(buf); + if (!dimm->unique_id) + goto err_read; + if (sscanf(dimm->unique_id, "%02x%02x-%02x-%02x%02x-%02x%02x%02x%02x", + &b[0], &b[1], &b[2], &b[3], &b[4], + &b[5], &b[6], &b[7], &b[8]) == 9) { + dimm->manufacturing_date = b[3] << 8 | b[4]; + dimm->manufacturing_location = b[2]; + } + } + sprintf(path, "%s/subsystem_vendor", dimm_base); + if (sysfs_read_attr(ctx, path, buf) == 0) + dimm->subsystem_vendor_id = strtoul(buf, NULL, 0); + + + sprintf(path, "%s/dirty_shutdown", dimm_base); + if (sysfs_read_attr(ctx, path, buf) == 0) + dimm->dirty_shutdown = strtoll(buf, NULL, 0); + +err_read: + free(path); +} + static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base) { int rc = -ENODEV; @@ -1685,6 +1733,10 @@ static int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base) rc = 0; } + /* add the available dimm attributes, the platform can override or add + * additional attributes later */ + populate_dimm_attributes(dimm, dimm_base); + free(path); return rc; } diff --git a/test/core.c b/test/core.c index 5118d86..0fd1011 100644 --- a/test/core.c +++ b/test/core.c @@ -195,6 +195,12 @@ retry: path = kmod_module_get_path(*mod); if (!path) { + /* For non-nfit platforms it's ok if nfit module is + * missing */ + if (strcmp(name, "nfit") == 0 || + strcmp(name, "nd_e820") == 0) + continue; + log_err(&log_ctx, "%s.ko: failed to get path\n", name); break; }
Don't fail is nfit module is missing, this will happen in platforms that don't have ACPI support. Add attributes to PAPR dimm family that are independent of platforms like the test dimms. Signed-off-by: Santosh Sivaraj <santosh@fossix.org> --- ndctl/lib/libndctl.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ test/core.c | 6 +++++ 2 files changed, 58 insertions(+)