Re: [ndctl PATCH v5 2/6] libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family

[Vaibhav Jain <vaibhav@linux.ibm.com>]

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
>
>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
>> index d76dbf7e17de..a52c2e208fbe 100644
>> --- a/ndctl/lib/libndctl.c
>> +++ b/ndctl/lib/libndctl.c
>> @@ -799,6 +799,28 @@ static void parse_nfit_mem_flags(struct ndctl_dimm *dimm, char *flags)
>>  				ndctl_dimm_get_devname(dimm), flags);
>>  }
>>  
>> +static void parse_papr_flags(struct ndctl_dimm *dimm, char *flags)
>> +{
>> +	char *start, *end;
>> +
>> +	start = flags;
>> +	while ((end = strchr(start, ' '))) {
>> +		*end = '\0';
>> +		if (strcmp(start, "not_armed") == 0)
>> +			dimm->flags.f_arm = 1;
>> +		else if (strcmp(start, "flush_fail") == 0)
>> +			dimm->flags.f_flush = 1;
>> +		else if (strcmp(start, "restore_fail") == 0)
>> +			dimm->flags.f_restore = 1;
>> +		else if (strcmp(start, "smart_notify") == 0)
>> +			dimm->flags.f_smart = 1;
>> +		start = end + 1;
>> +	}
>> +	if (end != start)
>> +		dbg(ndctl_dimm_get_ctx(dimm), "%s: %s\n",
>> +				ndctl_dimm_get_devname(dimm), flags);
>
> I think this would be more readable if ctx was obtained and saved in a
> 'ctx' variable at the start. Also, it might be better if 'flags' was
> included in the string - something like "%s flags: %s\n"
Sure will get this updated in v6. This function definition was derived
from parse_dimm_flags() hence looks this way.

>
>> +}
>> +
>>  static void parse_dimm_flags(struct ndctl_dimm *dimm, char *flags)
>>  {
>>  	char *start, *end;
>> @@ -856,6 +878,12 @@ static void *add_bus(void *parent, int id, const char *ctl_base)
>>  		bus->revision = strtoul(buf, NULL, 0);
>>  	}
>>  
>> +	sprintf(path, "%s/device/of_node/compatible", ctl_base);
>> +	if (sysfs_read_attr(ctx, path, buf) < 0)
>> +		bus->has_of_node = 0;
>> +	else
>> +		bus->has_of_node = 1;
>> +
>>  	sprintf(path, "%s/device/nfit/dsm_mask", ctl_base);
>>  	if (sysfs_read_attr(ctx, path, buf) < 0)
>>  		bus->nfit_dsm_mask = 0;
>> @@ -964,6 +992,10 @@ NDCTL_EXPORT int ndctl_bus_has_nfit(struct ndctl_bus *bus)
>>  	return bus->has_nfit;
>>  }
>>  
>> +NDCTL_EXPORT int ndctl_bus_has_of_node(struct ndctl_bus *bus)
>> +{
>> +	return bus->has_of_node;
>> +}
>
> New libndctl APIs need to update the 'symbol script' -
> ndctl/lib/libndctl.sym. Create a new 'section' at the bottom, and add
> all new symbols to that section. The new section would be 'LIBNDCTL_24'
> (Everything going into a given release gets a new section in that file,
> so any subsequent patches - throughout the release cycle - that add new
> APIs can add them to this new section).
Sure and thanks for pointing this out. Will add this symbol to the library
version script in v6

>
>>  /**
>>   * ndctl_bus_get_major - nd bus character device major number
>>   * @bus: ndctl_bus instance returned from ndctl_bus_get_{first|next}
>> @@ -1441,6 +1473,47 @@ 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);
>> +
>> +	dbg(ctx, "Probing of_pmem dimm %d at %s\n", dimm->id, dimm_base);
>> +
>> +	if (!path)
>> +		return -ENOMEM;
>> +
>> +	sprintf(path, "%s/../of_node/compatible", dimm_base);
>> +	if (sysfs_read_attr(ctx, path, buf) < 0)
>> +		goto out;
>
> Two things here:
> 1. Why not use the new ndctl_bus_has_of_node helper here? and
> 2. This looks redundant. add_papr_dimm() is only called if
> ndctl_bus_has_of_node() during add_dimm.
Presently we have two different nvdimm implementations:

* papr-scm: handled by arch/powerpc/platforms/pseries/papr_scm kernel module.
* nvdimm-n: handled by drivers/nvdimm/of_pmem kernel module.

Both nvdimms are exposed to the kernel via device tree nodes but different
'compatible' properties. This patchset only adds support for 'papr-scm'
compatible nvdimms.

'ndctl_bus_has_of_node()' simply indicates if the nvdimm has an
open-firmware compatible devicetree node associated with it and doesnt
necessarily indicate if its papr-scm compliant.

Hence validating the 'compatible' attribute value is necessary here.
Please see a more detailed info below regarding the 'compatible' sysfs
attribute.

>
>> +
>> +
>
> Nit: double newline here
Sure. Will fix this in v6
>
>> +	dbg(ctx, "Compatible of_pmem dimm %d at %s\n", dimm->id, buf);
>> +	/* construct path to the papr compatible dimm flags file */
>> +	sprintf(path, "%s/papr/flags", dimm_base);
>> +
>> +	if (strcmp(buf, "ibm,pmemory") == 0 &&
>> +	    sysfs_read_attr(ctx, path, buf) == 0) {
>> +
>> +		dbg(ctx, "Found papr dimm %d at %s\n", dimm->id, buf);
>
> Throughout the series - it would be better to print messages such as:
>
> 	"%s: adding papr dimm with flags: %s\n", devname, buf
>
> This would result in the canonical dimm names - nmem1, nmem2 and so on
> being used instead of "dimm 1" which can be confusing just from a
> convention standpoint.
>
> Similarly for the print a few lines above this:
>
> 	"%s: of_pmem compatible dimm: %s\n", devname, buf
>
Sure will address this in v6.

> (In this case, what does the 'compatible' sysfs attrubute contain? Is it
> useful to print here? - I havent't looked, just asking).

For papr-scm compatible nvdimms:
# cat /sys/class/nd/ndctl0/device/of_node/compatible
ibm,pmemory

This devicetree property is usually in format "<manufacturer>,<model>" that
tells the which kernel module to bind to the device. The papr_scm
kernel module indicates support for such device value in its device
match table ( https://git.io/JfPzF ) so that kernel can call its
probe-function.

Dumping this value here will be useful in debugging probe failures of nvdimms
that are exposed via open-firmware devicetree nodes but are not
necessarily papr_scm specification compliant.

>
>> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
>> new file mode 100644
>> index 000000000000..5ddf2755a950
>> --- /dev/null
>> +++ b/ndctl/lib/papr.c
>> @@ -0,0 +1,32 @@
>> +/*
>> + * Copyright (C) 2020 IBM Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU Lesser General Public License,
>> + * version 2.1, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT ANY
>> + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
>> + * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
>> + * more details.
>> + */
>
> Prefer SPDX license header instead fo the long form text for new files.
>
Sure will address this in v6.

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org