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

[Santosh Sivaraj <santosh@fossix.org>]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org