diff mbox series

[ndctl,v3,1/4] libndctl: test enablement for non-nfit devices

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

Commit Message

Santosh Sivaraj March 11, 2021, 7:46 a.m. UTC
Unify adding dimms for papr and nfit families, this will help in adding
all attributes needed for the unit tests too. We don't fail adding a dimm
if some of the dimm attributes are missing, so this will work fine on PAPR
platforms where most dimm attributes are provided.

Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
---
 ndctl/lib/libndctl.c | 103 ++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 65 deletions(-)

v3:
* Drop patch which skips SMART tests, smart test enablement will be posted
  soon.

v2:
* Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
  INTEL family instead. That change was there to test the same code on x86, but
  accidently committed. Now have a environment variable to force test PAPR
  family on x86.

* Patch 4: Remove stray code, artifact of refactoring in patch 1.

Comments

Verma, Vishal L March 17, 2021, 10:08 p.m. UTC | #1
On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
> Unify adding dimms for papr and nfit families, this will help in adding

Minor nit, but it seems like the subject line and the first sentence in
the body should be swapped. The one-line description of what's happening
in this patch is "Unify adding dimms for papr and nfit families", and
the body can go into more detail about why - i.e. in preparation for
enabling tests on non-nfit devices.

> all attributes needed for the unit tests too. We don't fail adding a dimm
> if some of the dimm attributes are missing, so this will work fine on PAPR
> platforms where most dimm attributes are provided.

Does this mean 'most - but not all'? Might be a good clarification to
make here.

> 
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
>  ndctl/lib/libndctl.c | 103 ++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 65 deletions(-)
> 
> v3:
> * Drop patch which skips SMART tests, smart test enablement will be posted
>   soon.
> 
> v2:
> * Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
>   INTEL family instead. That change was there to test the same code on x86, but
>   accidently committed. Now have a environment variable to force test PAPR
>   family on x86.
> 
> * Patch 4: Remove stray code, artifact of refactoring in patch 1.
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 36fb6fe..26b9317 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1646,41 +1646,9 @@ 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 int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> -{
> -	int rc = -ENODEV;
> -	char buf[SYSFS_ATTR_SIZE];
> -	struct ndctl_ctx *ctx = dimm->bus->ctx;
> -	char *path = calloc(1, strlen(dimm_base) + 100);
> -	const char * const devname = ndctl_dimm_get_devname(dimm);
> -
> -	dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
> -
> -	if (!path)
> -		return -ENOMEM;
> -
> -	/* construct path to the papr compatible dimm flags file */
> -	sprintf(path, "%s/papr/flags", dimm_base);
> -
> -	if (ndctl_bus_is_papr_scm(dimm->bus) &&
> -	    sysfs_read_attr(ctx, path, buf) == 0) {
> -
> -		dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname, buf);
> -		dimm->cmd_family = NVDIMM_FAMILY_PAPR;
> -
> -		/* Parse dimm flags */
> -		parse_papr_flags(dimm, buf);
> -
> -		/* Allocate monitor mode fd */
> -		dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
> -		rc = 0;
> -	}
> -
> -	free(path);
> -	return rc;
> -}
> -
> -static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
> +static int populate_dimm_attributes(struct ndctl_dimm *dimm,
> +				    const char *dimm_base,
> +				    const char *bus_prefix)
>  {
>  	int i, rc = -1;
>  	char buf[SYSFS_ATTR_SIZE];
> @@ -1694,7 +1662,7 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>  	 * 'unique_id' may not be available on older kernels, so don't
>  	 * fail if the read fails.
>  	 */
> -	sprintf(path, "%s/nfit/id", dimm_base);
> +	sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0) {
>  		unsigned int b[9];
> 
> @@ -1709,68 +1677,74 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>  		}
>  	}
> 
> -	sprintf(path, "%s/nfit/handle", dimm_base);
> +	sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		goto err_read;
>  	dimm->handle = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/phys_id", dimm_base);
> +	sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		goto err_read;
>  	dimm->phys_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/serial", dimm_base);
> +	sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->serial = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/vendor", dimm_base);
> +	sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->vendor_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/device", dimm_base);
> +	sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->device_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/rev_id", dimm_base);
> +	sprintf(path, "%s/%s/rev_id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->revision_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/dirty_shutdown", dimm_base);
> +	sprintf(path, "%s/%s/dirty_shutdown", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->dirty_shutdown = strtoll(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/subsystem_vendor", dimm_base);
> +	sprintf(path, "%s/%s/subsystem_vendor", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/subsystem_device", dimm_base);
> +	sprintf(path, "%s/%s/subsystem_device", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->subsystem_device_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/subsystem_rev_id", dimm_base);
> +	sprintf(path, "%s/%s/subsystem_rev_id", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->subsystem_revision_id = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/family", dimm_base);
> +	sprintf(path, "%s/%s/family", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->cmd_family = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
> +	sprintf(path, "%s/%s/dsm_mask", dimm_base, bus_prefix);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>  
> -	sprintf(path, "%s/nfit/format", dimm_base);
> +	sprintf(path, "%s/%s/format", dimm_base, bus_prefix);
>  	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/nfit/format%d", dimm_base, i);
> +		sprintf(path, "%s/%s/format%d", dimm_base, bus_prefix, i);
>  		if (sysfs_read_attr(ctx, path, buf) == 0)
>  			dimm->format[i] = strtoul(buf, NULL, 0);
>  	}
>  
> -	sprintf(path, "%s/nfit/flags", dimm_base);
> -	if (sysfs_read_attr(ctx, path, buf) == 0)
> -		parse_nfit_mem_flags(dimm, buf);
> +	sprintf(path, "%s/%s/flags", dimm_base, bus_prefix);
> +	if (sysfs_read_attr(ctx, path, buf) == 0) {
> +		if (ndctl_bus_has_nfit(dimm->bus))
> +			parse_nfit_mem_flags(dimm, buf);
> +		else if (ndctl_bus_is_papr_scm(dimm->bus)) {
> +			dimm->cmd_family = NVDIMM_FAMILY_PAPR;
> +			parse_papr_flags(dimm, buf);
> +		}
> +	}
>  
>  	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
>  	rc = 0;
> @@ -1792,7 +1766,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" : "papr");
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		formats = 1;
>  	else
> @@ -1866,13 +1841,12 @@ 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);
> -	}
> +		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
> +	} else if (ndctl_bus_has_of_node(bus))
> +		rc = populate_dimm_attributes(dimm, dimm_base, "papr");
>  
>  	if (rc == -ENODEV) {
>  		/* Unprobed dimm with no family */
> @@ -2531,13 +2505,12 @@ 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)
> -			goto err_read;
> -		region->range_index = strtoul(buf, NULL, 0);
> -	} else
> +	sprintf(path, "%s/%s/range_index", region_base,
> +		ndctl_bus_has_nfit(bus) ? "nfit": "papr");
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
>  		region->range_index = -1;
> +	else
> +		region->range_index = strtoul(buf, NULL, 0);
>  
>  	sprintf(path, "%s/read_only", region_base);
>  	if (sysfs_read_attr(ctx, path, buf) < 0)
Santosh Sivaraj March 19, 2021, 5:54 a.m. UTC | #2
"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

Hi Vishal,

> On Thu, 2021-03-11 at 13:16 +0530, Santosh Sivaraj wrote:
>> Unify adding dimms for papr and nfit families, this will help in adding
>
> Minor nit, but it seems like the subject line and the first sentence in
> the body should be swapped. The one-line description of what's happening
> in this patch is "Unify adding dimms for papr and nfit families", and
> the body can go into more detail about why - i.e. in preparation for
> enabling tests on non-nfit devices.
>

Agree.

>> all attributes needed for the unit tests too. We don't fail adding a dimm
>> if some of the dimm attributes are missing, so this will work fine on PAPR
>> platforms where most dimm attributes are provided.
>
> Does this mean 'most - but not all'? Might be a good clarification to
> make here.

Yes, that's right. I will fix that up in the next version.

Thanks,
Santosh
>
>> 
>> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
>> ---
>>  ndctl/lib/libndctl.c | 103 ++++++++++++++++---------------------------
>>  1 file changed, 38 insertions(+), 65 deletions(-)
>> 
>> v3:
>> * Drop patch which skips SMART tests, smart test enablement will be posted
>>   soon.
>> 
>> v2:
>> * Patch 2: Fix a bug, I skip erroring out if PAPR family, but condition had
>>   INTEL family instead. That change was there to test the same code on x86, but
>>   accidently committed. Now have a environment variable to force test PAPR
>>   family on x86.
>> 
>> * Patch 4: Remove stray code, artifact of refactoring in patch 1.
>> 
>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
>> index 36fb6fe..26b9317 100644
>> --- a/ndctl/lib/libndctl.c
>> +++ b/ndctl/lib/libndctl.c
>> @@ -1646,41 +1646,9 @@ 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 int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>> -{
>> -	int rc = -ENODEV;
>> -	char buf[SYSFS_ATTR_SIZE];
>> -	struct ndctl_ctx *ctx = dimm->bus->ctx;
>> -	char *path = calloc(1, strlen(dimm_base) + 100);
>> -	const char * const devname = ndctl_dimm_get_devname(dimm);
>> -
>> -	dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
>> -
>> -	if (!path)
>> -		return -ENOMEM;
>> -
>> -	/* construct path to the papr compatible dimm flags file */
>> -	sprintf(path, "%s/papr/flags", dimm_base);
>> -
>> -	if (ndctl_bus_is_papr_scm(dimm->bus) &&
>> -	    sysfs_read_attr(ctx, path, buf) == 0) {
>> -
>> -		dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname, buf);
>> -		dimm->cmd_family = NVDIMM_FAMILY_PAPR;
>> -
>> -		/* Parse dimm flags */
>> -		parse_papr_flags(dimm, buf);
>> -
>> -		/* Allocate monitor mode fd */
>> -		dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
>> -		rc = 0;
>> -	}
>> -
>> -	free(path);
>> -	return rc;
>> -}
>> -
>> -static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>> +static int populate_dimm_attributes(struct ndctl_dimm *dimm,
>> +				    const char *dimm_base,
>> +				    const char *bus_prefix)
>>  {
>>  	int i, rc = -1;
>>  	char buf[SYSFS_ATTR_SIZE];
>> @@ -1694,7 +1662,7 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>>  	 * 'unique_id' may not be available on older kernels, so don't
>>  	 * fail if the read fails.
>>  	 */
>> -	sprintf(path, "%s/nfit/id", dimm_base);
>> +	sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0) {
>>  		unsigned int b[9];
>> 
>> @@ -1709,68 +1677,74 @@ static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>>  		}
>>  	}
>> 
>> -	sprintf(path, "%s/nfit/handle", dimm_base);
>> +	sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		goto err_read;
>>  	dimm->handle = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/phys_id", dimm_base);
>> +	sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		goto err_read;
>>  	dimm->phys_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/serial", dimm_base);
>> +	sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->serial = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/vendor", dimm_base);
>> +	sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->vendor_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/device", dimm_base);
>> +	sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->device_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/rev_id", dimm_base);
>> +	sprintf(path, "%s/%s/rev_id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->revision_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/dirty_shutdown", dimm_base);
>> +	sprintf(path, "%s/%s/dirty_shutdown", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->dirty_shutdown = strtoll(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/subsystem_vendor", dimm_base);
>> +	sprintf(path, "%s/%s/subsystem_vendor", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/subsystem_device", dimm_base);
>> +	sprintf(path, "%s/%s/subsystem_device", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->subsystem_device_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/subsystem_rev_id", dimm_base);
>> +	sprintf(path, "%s/%s/subsystem_rev_id", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->subsystem_revision_id = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/family", dimm_base);
>> +	sprintf(path, "%s/%s/family", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->cmd_family = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
>> +	sprintf(path, "%s/%s/dsm_mask", dimm_base, bus_prefix);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>>  
>> -	sprintf(path, "%s/nfit/format", dimm_base);
>> +	sprintf(path, "%s/%s/format", dimm_base, bus_prefix);
>>  	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/nfit/format%d", dimm_base, i);
>> +		sprintf(path, "%s/%s/format%d", dimm_base, bus_prefix, i);
>>  		if (sysfs_read_attr(ctx, path, buf) == 0)
>>  			dimm->format[i] = strtoul(buf, NULL, 0);
>>  	}
>>  
>> -	sprintf(path, "%s/nfit/flags", dimm_base);
>> -	if (sysfs_read_attr(ctx, path, buf) == 0)
>> -		parse_nfit_mem_flags(dimm, buf);
>> +	sprintf(path, "%s/%s/flags", dimm_base, bus_prefix);
>> +	if (sysfs_read_attr(ctx, path, buf) == 0) {
>> +		if (ndctl_bus_has_nfit(dimm->bus))
>> +			parse_nfit_mem_flags(dimm, buf);
>> +		else if (ndctl_bus_is_papr_scm(dimm->bus)) {
>> +			dimm->cmd_family = NVDIMM_FAMILY_PAPR;
>> +			parse_papr_flags(dimm, buf);
>> +		}
>> +	}
>>  
>>  	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
>>  	rc = 0;
>> @@ -1792,7 +1766,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" : "papr");
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		formats = 1;
>>  	else
>> @@ -1866,13 +1841,12 @@ 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);
>> -	}
>> +		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
>> +	} else if (ndctl_bus_has_of_node(bus))
>> +		rc = populate_dimm_attributes(dimm, dimm_base, "papr");
>>  
>>  	if (rc == -ENODEV) {
>>  		/* Unprobed dimm with no family */
>> @@ -2531,13 +2505,12 @@ 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)
>> -			goto err_read;
>> -		region->range_index = strtoul(buf, NULL, 0);
>> -	} else
>> +	sprintf(path, "%s/%s/range_index", region_base,
>> +		ndctl_bus_has_nfit(bus) ? "nfit": "papr");
>> +	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		region->range_index = -1;
>> +	else
>> +		region->range_index = strtoul(buf, NULL, 0);
>>  
>>  	sprintf(path, "%s/read_only", region_base);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
diff mbox series

Patch

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 36fb6fe..26b9317 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1646,41 +1646,9 @@  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 int add_papr_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
-{
-	int rc = -ENODEV;
-	char buf[SYSFS_ATTR_SIZE];
-	struct ndctl_ctx *ctx = dimm->bus->ctx;
-	char *path = calloc(1, strlen(dimm_base) + 100);
-	const char * const devname = ndctl_dimm_get_devname(dimm);
-
-	dbg(ctx, "%s: Probing of_pmem dimm at %s\n", devname, dimm_base);
-
-	if (!path)
-		return -ENOMEM;
-
-	/* construct path to the papr compatible dimm flags file */
-	sprintf(path, "%s/papr/flags", dimm_base);
-
-	if (ndctl_bus_is_papr_scm(dimm->bus) &&
-	    sysfs_read_attr(ctx, path, buf) == 0) {
-
-		dbg(ctx, "%s: Adding papr-scm dimm flags:\"%s\"\n", devname, buf);
-		dimm->cmd_family = NVDIMM_FAMILY_PAPR;
-
-		/* Parse dimm flags */
-		parse_papr_flags(dimm, buf);
-
-		/* Allocate monitor mode fd */
-		dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
-		rc = 0;
-	}
-
-	free(path);
-	return rc;
-}
-
-static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
+static int populate_dimm_attributes(struct ndctl_dimm *dimm,
+				    const char *dimm_base,
+				    const char *bus_prefix)
 {
 	int i, rc = -1;
 	char buf[SYSFS_ATTR_SIZE];
@@ -1694,7 +1662,7 @@  static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
 	 * 'unique_id' may not be available on older kernels, so don't
 	 * fail if the read fails.
 	 */
-	sprintf(path, "%s/nfit/id", dimm_base);
+	sprintf(path, "%s/%s/id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0) {
 		unsigned int b[9];
 
@@ -1709,68 +1677,74 @@  static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
 		}
 	}
 
-	sprintf(path, "%s/nfit/handle", dimm_base);
+	sprintf(path, "%s/%s/handle", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		goto err_read;
 	dimm->handle = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/phys_id", dimm_base);
+	sprintf(path, "%s/%s/phys_id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		goto err_read;
 	dimm->phys_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/serial", dimm_base);
+	sprintf(path, "%s/%s/serial", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->serial = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/vendor", dimm_base);
+	sprintf(path, "%s/%s/vendor", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->vendor_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/device", dimm_base);
+	sprintf(path, "%s/%s/device", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->device_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/rev_id", dimm_base);
+	sprintf(path, "%s/%s/rev_id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->revision_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/dirty_shutdown", dimm_base);
+	sprintf(path, "%s/%s/dirty_shutdown", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->dirty_shutdown = strtoll(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/subsystem_vendor", dimm_base);
+	sprintf(path, "%s/%s/subsystem_vendor", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_vendor_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/subsystem_device", dimm_base);
+	sprintf(path, "%s/%s/subsystem_device", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_device_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/subsystem_rev_id", dimm_base);
+	sprintf(path, "%s/%s/subsystem_rev_id", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->subsystem_revision_id = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/family", dimm_base);
+	sprintf(path, "%s/%s/family", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->cmd_family = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
+	sprintf(path, "%s/%s/dsm_mask", dimm_base, bus_prefix);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
 
-	sprintf(path, "%s/nfit/format", dimm_base);
+	sprintf(path, "%s/%s/format", dimm_base, bus_prefix);
 	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/nfit/format%d", dimm_base, i);
+		sprintf(path, "%s/%s/format%d", dimm_base, bus_prefix, i);
 		if (sysfs_read_attr(ctx, path, buf) == 0)
 			dimm->format[i] = strtoul(buf, NULL, 0);
 	}
 
-	sprintf(path, "%s/nfit/flags", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) == 0)
-		parse_nfit_mem_flags(dimm, buf);
+	sprintf(path, "%s/%s/flags", dimm_base, bus_prefix);
+	if (sysfs_read_attr(ctx, path, buf) == 0) {
+		if (ndctl_bus_has_nfit(dimm->bus))
+			parse_nfit_mem_flags(dimm, buf);
+		else if (ndctl_bus_is_papr_scm(dimm->bus)) {
+			dimm->cmd_family = NVDIMM_FAMILY_PAPR;
+			parse_papr_flags(dimm, buf);
+		}
+	}
 
 	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
 	rc = 0;
@@ -1792,7 +1766,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" : "papr");
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		formats = 1;
 	else
@@ -1866,13 +1841,12 @@  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);
-	}
+		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
+	} else if (ndctl_bus_has_of_node(bus))
+		rc = populate_dimm_attributes(dimm, dimm_base, "papr");
 
 	if (rc == -ENODEV) {
 		/* Unprobed dimm with no family */
@@ -2531,13 +2505,12 @@  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)
-			goto err_read;
-		region->range_index = strtoul(buf, NULL, 0);
-	} else
+	sprintf(path, "%s/%s/range_index", region_base,
+		ndctl_bus_has_nfit(bus) ? "nfit": "papr");
+	if (sysfs_read_attr(ctx, path, buf) < 0)
 		region->range_index = -1;
+	else
+		region->range_index = strtoul(buf, NULL, 0);
 
 	sprintf(path, "%s/read_only", region_base);
 	if (sysfs_read_attr(ctx, path, buf) < 0)