diff mbox series

[ndctl,RFC,v4,1/3] libndctl: test enablement for non-nfit devices

Message ID 20201108122016.2090891-1-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
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(+)

Comments

Dan Williams Dec. 7, 2020, 11:07 p.m. UTC | #1
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.
Santosh Sivaraj Dec. 9, 2020, 2:09 a.m. UTC | #2
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 mbox series

Patch

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;
 		}