[ndctl PATCH v5 0/6] Add support for reporting papr nvdimm health

[ndctl PATCH v5 0/6] Add support for reporting papr nvdimm health

[Vaibhav Jain]

Changes since v4 [1]:
* Updated proposed changes to remove usage of term 'SCM' due to
  ambiguity with 'PMEM' and 'NVDIMM'. [ Dan Williams ]
* Replaced the usage of term 'SCM' with 'PMEM' in most contexts.
  [ Aneesh ]
* Updates to various newly introduced identifiers in 'papr.c'
  removing the 'SCM' prefix from their names.
* Renamed NVDIMM_FAMILY_PAPR_SCM to NVDIMM_FAMILY_PAPR
* Renamed PAPR_SCM_PDSM_HEALTH PAPR_PDSM_HEALTH
* Renamed 'papr_scm.c' to 'papr.c'
* Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'

[1] https://lore.kernel.org/linux-nvdimm/20200527044737.40615-1-vaibhav@linux.ibm.com

---
This patch-set proposes changes to libndctl to add support for reporting
health for nvdimms that support the PAPR standard[2]. The standard defines
machenism (HCALL) through which a guest kernel can query and fetch health
and performance stats of an nvdimm attached to the hypervisor[3]. Until
now 'ndctl' was unable to report these stats for papr_scm dimms on PPC64
guests due to absence of ACPI/NFIT, a limitation which this patch-set tries
to address.

The patch-set introduces support for the new PAPR PDSM family
defined at [4] & [5] via a new dimm-ops named
'papr_dimm_ops'. Infrastructure to probe and distinguish papr-scm
dimms from other dimm families that may support ACPI/NFIT is
implemented by updating the 'struct ndctl_dimm' initialization
routines to bifurcate based on the nvdimm type. We also introduce two
new dimm-ops member for handling initialization of dimm specific data
for specific DSM families.

These changes coupled with proposed kernel changes located at Ref[1] should
provide a way for the user to retrieve NVDIMM health status using ndtcl for
pseries guests. Below is a sample output using proposed kernel + ndctl
changes:

 # ndctl list -DH
[
  {
    "dev":"nmem0",
    "flag_smart_event":true,
    "health":{
      "health_state":"fatal",
      "shutdown_state":"dirty"
    }
  }
]

Structure of the patchset
=========================

We start with a re-factoring patch that splits the 'add_dimm()' function
into two functions one that take care of allocating and initializing
'struct ndctl_dimm' and another that takes care of initializing nfit
specific dimm attributes.

Patch-2 introduces probe function of papr nvdimms and assigning
'papr_dimm_ops' defined in 'papr.c' to 'dimm->ops' if
needed. The patch also code to parse the dimm flags specific to
papr nvdimms

Patch-3 introduces new dimm ops 'dimm_init()' & 'dimm_uninit()' to handle
DSM family specific initialization of 'struct ndctl_dimm'.

Patches-4,5 implements scaffolding to add support for PAPR PDSM
requests and pull in their definitions from the kernel.

Finally Patch-6 add support for issuing and handling the result of
'struct ndctl_cmd' to request dimm health stats from papr_scm kernel module
and returning appropriate health status to libndctl for reporting.

References
==========
[2] "Power Architecture Platform Reference"
https://en.wikipedia.org/wiki/Power_Architecture_Platform_Reference

[3] "Hypercall Op-codes (hcalls)"
https://github.com/torvalds/linux/blob/master/Documentation/powerpc/papr_hcalls.rst

[4] "powerpc/papr_scm: Add support for reporting nvdimm health"
https://lore.kernel.org/linux-nvdimm/20200529214719.223344-1-vaibhav@linux.ibm.com

[5] "ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods"
https://lore.kernel.org/linux-nvdimm/20200529214719.223344-5-vaibhav@linux.ibm.com

Vaibhav Jain (6):
  libndctl: Refactor out add_dimm() to handle NFIT specific init
  libncdtl: Add initial support for NVDIMM_FAMILY_PAPR nvdimm family
  libndctl: Introduce new dimm-ops dimm_init() & dimm_uninit()
  libndctl,papr_scm: Add definitions for PAPR nvdimm specific methods
  papr: Add scaffolding to issue and handle PDSM requests
  libndctl,papr_scm: Implement support for PAPR_PDSM_HEALTH

 ndctl/lib/Makefile.am |   1 +
 ndctl/lib/libndctl.c  | 260 ++++++++++++++++++++++++++----------
 ndctl/lib/papr.c      | 301 ++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/papr_pdsm.h | 175 ++++++++++++++++++++++++
 ndctl/lib/private.h   |   9 ++
 ndctl/libndctl.h      |   1 +
 ndctl/ndctl.h         |   1 +
 7 files changed, 678 insertions(+), 70 deletions(-)
 create mode 100644 ndctl/lib/papr.c
 create mode 100644 ndctl/lib/papr_pdsm.h

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

[ndctl PATCH v5 1/6] libndctl: Refactor out add_dimm() to handle NFIT specific init

[Vaibhav Jain]

Presently add_dimm() only probes dimms that support NFIT/ACPI. Hence
this patch refactors this functionality into two functions namely
add_dimm() and add_nfit_dimm(). Function add_dimm() performs
allocation and common 'struct ndctl_dimm' initialization and depending
on whether the dimm-bus supports NIFT, calls add_nfit_dimm(). Once
the probe is completed based on the value of 'ndctl_dimm.cmd_family'
appropriate dimm-ops are assigned to the dimm.

In case dimm-bus is of unknown type or doesn't support NFIT the
initialization still continues, with no dimm-ops assigned to the
'struct ndctl_dimm' there-by limiting the functionality available.

This patch shouldn't introduce any behavioral change.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5:
* None

v3..v4:
* None

v2..v3:
* None

v1..v2:
* None
---
 ndctl/lib/libndctl.c | 193 +++++++++++++++++++++++++------------------
 1 file changed, 112 insertions(+), 81 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ee737cbbfe3e..d76dbf7e17de 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1441,82 +1441,15 @@ 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 *add_dimm(void *parent, int id, const char *dimm_base)
+static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
 {
-	int formats, i;
-	struct ndctl_dimm *dimm;
+	int i, rc = -1;
 	char buf[SYSFS_ATTR_SIZE];
-	struct ndctl_bus *bus = parent;
-	struct ndctl_ctx *ctx = bus->ctx;
+	struct ndctl_ctx *ctx = dimm->bus->ctx;
 	char *path = calloc(1, strlen(dimm_base) + 100);
 
 	if (!path)
-		return NULL;
-
-	sprintf(path, "%s/nfit/formats", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) < 0)
-		formats = 1;
-	else
-		formats = clamp(strtoul(buf, NULL, 0), 1UL, 2UL);
-
-	dimm = calloc(1, sizeof(*dimm) + sizeof(int) * formats);
-	if (!dimm)
-		goto err_dimm;
-	dimm->bus = bus;
-	dimm->id = id;
-
-	sprintf(path, "%s/dev", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) < 0)
-		goto err_read;
-	if (sscanf(buf, "%d:%d", &dimm->major, &dimm->minor) != 2)
-		goto err_read;
-
-	sprintf(path, "%s/commands", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) < 0)
-		goto err_read;
-	dimm->cmd_mask = parse_commands(buf, 1);
-
-	dimm->dimm_buf = calloc(1, strlen(dimm_base) + 50);
-	if (!dimm->dimm_buf)
-		goto err_read;
-	dimm->buf_len = strlen(dimm_base) + 50;
-
-	dimm->dimm_path = strdup(dimm_base);
-	if (!dimm->dimm_path)
-		goto err_read;
-
-	sprintf(path, "%s/modalias", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) < 0)
-		goto err_read;
-	dimm->module = to_module(ctx, buf);
-
-	dimm->handle = -1;
-	dimm->phys_id = -1;
-	dimm->serial = -1;
-	dimm->vendor_id = -1;
-	dimm->device_id = -1;
-	dimm->revision_id = -1;
-	dimm->health_eventfd = -1;
-	dimm->dirty_shutdown = -ENOENT;
-	dimm->subsystem_vendor_id = -1;
-	dimm->subsystem_device_id = -1;
-	dimm->subsystem_revision_id = -1;
-	dimm->manufacturing_date = -1;
-	dimm->manufacturing_location = -1;
-	dimm->cmd_family = -1;
-	dimm->nfit_dsm_mask = ULONG_MAX;
-	for (i = 0; i < formats; i++)
-		dimm->format[i] = -1;
-
-	sprintf(path, "%s/flags", dimm_base);
-	if (sysfs_read_attr(ctx, path, buf) < 0) {
-		dimm->locked = -1;
-		dimm->aliased = -1;
-	} else
-		parse_dimm_flags(dimm, buf);
-
-	if (!ndctl_bus_has_nfit(bus))
-		goto out;
+		return -1;
 
 	/*
 	 * 'unique_id' may not be available on older kernels, so don't
@@ -1582,24 +1515,15 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 	sprintf(path, "%s/nfit/family", dimm_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->cmd_family = strtoul(buf, NULL, 0);
-	if (dimm->cmd_family == NVDIMM_FAMILY_INTEL)
-		dimm->ops = intel_dimm_ops;
-	if (dimm->cmd_family == NVDIMM_FAMILY_HPE1)
-		dimm->ops = hpe1_dimm_ops;
-	if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
-		dimm->ops = msft_dimm_ops;
-	if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
-		dimm->ops = hyperv_dimm_ops;
 
 	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
 
-	dimm->formats = formats;
 	sprintf(path, "%s/nfit/format", dimm_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		dimm->format[0] = strtoul(buf, NULL, 0);
-	for (i = 1; i < formats; i++) {
+	for (i = 1; i < dimm->formats; i++) {
 		sprintf(path, "%s/nfit/format%d", dimm_base, i);
 		if (sysfs_read_attr(ctx, path, buf) == 0)
 			dimm->format[i] = strtoul(buf, NULL, 0);
@@ -1610,7 +1534,114 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 		parse_nfit_mem_flags(dimm, buf);
 
 	dimm->health_eventfd = open(path, O_RDONLY|O_CLOEXEC);
+	rc = 0;
+ err_read:
+
+	free(path);
+	return rc;
+}
+
+static void *add_dimm(void *parent, int id, const char *dimm_base)
+{
+	int formats, i, rc = -ENODEV;
+	struct ndctl_dimm *dimm = NULL;
+	char buf[SYSFS_ATTR_SIZE];
+	struct ndctl_bus *bus = parent;
+	struct ndctl_ctx *ctx = bus->ctx;
+	char *path = calloc(1, strlen(dimm_base) + 100);
+
+	if (!path)
+		return NULL;
+
+	sprintf(path, "%s/nfit/formats", dimm_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		formats = 1;
+	else
+		formats = clamp(strtoul(buf, NULL, 0), 1UL, 2UL);
+
+	dimm = calloc(1, sizeof(*dimm) + sizeof(int) * formats);
+	if (!dimm)
+		goto err_dimm;
+	dimm->bus = bus;
+	dimm->id = id;
+
+	sprintf(path, "%s/dev", dimm_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		goto err_read;
+	if (sscanf(buf, "%d:%d", &dimm->major, &dimm->minor) != 2)
+		goto err_read;
+
+	sprintf(path, "%s/commands", dimm_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		goto err_read;
+	dimm->cmd_mask = parse_commands(buf, 1);
+
+	dimm->dimm_buf = calloc(1, strlen(dimm_base) + 50);
+	if (!dimm->dimm_buf)
+		goto err_read;
+	dimm->buf_len = strlen(dimm_base) + 50;
+
+	dimm->dimm_path = strdup(dimm_base);
+	if (!dimm->dimm_path)
+		goto err_read;
+
+	sprintf(path, "%s/modalias", dimm_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		goto err_read;
+	dimm->module = to_module(ctx, buf);
+
+	dimm->handle = -1;
+	dimm->phys_id = -1;
+	dimm->serial = -1;
+	dimm->vendor_id = -1;
+	dimm->device_id = -1;
+	dimm->revision_id = -1;
+	dimm->health_eventfd = -1;
+	dimm->dirty_shutdown = -ENOENT;
+	dimm->subsystem_vendor_id = -1;
+	dimm->subsystem_device_id = -1;
+	dimm->subsystem_revision_id = -1;
+	dimm->manufacturing_date = -1;
+	dimm->manufacturing_location = -1;
+	dimm->cmd_family = -1;
+	dimm->nfit_dsm_mask = ULONG_MAX;
+	for (i = 0; i < formats; i++)
+		dimm->format[i] = -1;
+
+	sprintf(path, "%s/flags", dimm_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0) {
+		dimm->locked = -1;
+		dimm->aliased = -1;
+	} else
+		parse_dimm_flags(dimm, buf);
+
+	/* Check if the given dimm supports nfit */
+	if (ndctl_bus_has_nfit(bus)) {
+		dimm->formats = formats;
+		rc = add_nfit_dimm(dimm, dimm_base);
+	}
+
+	if (rc == -ENODEV) {
+		/* Unprobed dimm with no family */
+		rc = 0;
+		goto out;
+	}
+
+	/* Assign dimm-ops based on command family */
+	if (dimm->cmd_family == NVDIMM_FAMILY_INTEL)
+		dimm->ops = intel_dimm_ops;
+	if (dimm->cmd_family == NVDIMM_FAMILY_HPE1)
+		dimm->ops = hpe1_dimm_ops;
+	if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
+		dimm->ops = msft_dimm_ops;
+	if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
+		dimm->ops = hyperv_dimm_ops;
  out:
+	if (rc) {
+		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
+		goto err_read;
+	}
+
 	list_add(&bus->dimms, &dimm->list);
 	free(path);
 
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Vaibhav Jain]

Add necessary scaffolding in libndctl for dimms that support papr_scm
specification[1]. Since there can be platforms that support
Open-Firmware[2] but not the papr_scm specification, hence the changes
proposed first add support for probing if the dimm bus supports
Open-Firmware. This is done via querying for sysfs attribute 'of_node'
in dimm device sysfs directory. If available newly introduced member
'struct ndctl_bus.has_of_node' is set. During the probe of the dimm
and execution of add_dimm(), the newly introduced add_papr_dimm()
is called if dimm bus reports supports Open-Firmware.

Function add_papr_dimm() queries the 'compatible' device tree
attribute and based on its value assign NVDIMM_FAMILY_PAPR to the
dimm command family. In future, based on the contents of 'compatible'
attribute more of_pmem dimm families can be queried.

We also add support for parsing the dimm flags for
NVDIMM_FAMILY_PAPR supporting nvdimms as described at [3]. A newly
introduced function parse_papr_flags() reads the contents of this
flag file and sets appropriate flag bits in 'struct
ndctl_dimm.flags'.

Also we advertise support for monitor mode by allocating a file
descriptor to the dimm 'flags' file and assigning it to 'struct
ndctl_dimm.health_event_fd'.

The dimm-ops implementation for NVDIMM_FAMILY_PAPR is
available in global variable 'papr_dimm_ops' which points to
skeleton implementation in newly introduced file 'lib/papr.c'.

References:
[1] Documentation/powerpc/papr_hcalls.rst
https://lore.kernel.org/linux-nvdimm/20200529214719.223344-2-vaibhav@linux.ibm.com

[2] https://en.wikipedia.org/wiki/Open_Firmware

[3] Documentation/ABI/testing/sysfs-bus-papr-pmem
https://lore.kernel.org/linux-nvdimm/20200529214719.223344-4-vaibhav@linux.ibm.com

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5:
* Renamed file 'papr_scm.c' to 'papr.c'
* s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
* s/papr_scm_dimm_ops/papr_dimm_ops/g
* s/parse_papr_scm_flags()/parse_papr_flags()/g
* Updated patch description & title to reflect new command family name
  and file-names.

v3..v4:
* None

v2..v3:
* Renamed add_of_pmem() to add_papr_dimm() [ Aneesh ]

v1..v2:
* Squashed the patch to parse dimm flags
* Updated the dimm flags parsing to add case for 'restore_fail' and
  'flush_fail'.
* Renamed parse_of_pmem_flags() to parse_papr_scm_flags().
* Updated the path to dimm flags file to 'papr/flags'.
* Introduced 'papr_scm.c' file in this patch rather than later in the
  patch series.
* Update add_of_pmem_dimm() to parse dimm flags and enable monitoring
  only if 'ibm,pmemory' compatible nvdimm is found.
---
 ndctl/lib/Makefile.am |  1 +
 ndctl/lib/libndctl.c  | 78 +++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/papr.c      | 32 ++++++++++++++++++
 ndctl/lib/private.h   |  2 ++
 ndctl/libndctl.h      |  1 +
 ndctl/ndctl.h         |  1 +
 6 files changed, 115 insertions(+)
 create mode 100644 ndctl/lib/papr.c

diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index d6be5c3acd26..e15bb2288575 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -23,6 +23,7 @@ libndctl_la_SOURCES =\
 	hpe1.c \
 	msft.c \
 	hyperv.c \
+	papr.c \
 	ars.c \
 	firmware.c \
 	libndctl.c \
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);
+}
+
 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;
+}
 /**
  * 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;
+
+
+	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);
+		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;
+	}
+	/* Ignore dimm if unknown type */
+
+ out:
+	free(path);
+	return rc;
+}
+
 static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
 {
 	int i, rc = -1;
@@ -1619,6 +1692,8 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 	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);
 	}
 
 	if (rc == -ENODEV) {
@@ -1636,6 +1711,9 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 		dimm->ops = msft_dimm_ops;
 	if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
 		dimm->ops = hyperv_dimm_ops;
+	if (dimm->cmd_family == NVDIMM_FAMILY_PAPR)
+		dimm->ops = papr_dimm_ops;
+
  out:
 	if (rc) {
 		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
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.
+ */
+#include <stdint.h>
+#include <stdlib.h>
+#include <limits.h>
+#include <util/log.h>
+#include <ndctl.h>
+#include <ndctl/libndctl.h>
+#include <lib/private.h>
+
+static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
+{
+	/* Handle this separately to support monitor mode */
+	if (cmd == ND_CMD_SMART)
+		return true;
+
+	return !!(dimm->cmd_mask & (1ULL << cmd));
+}
+
+struct ndctl_dimm_ops * const papr_dimm_ops = &(struct ndctl_dimm_ops) {
+	.cmd_is_supported = papr_cmd_is_supported,
+};
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index 2e537f0a8649..d90236b1f98b 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -167,6 +167,7 @@ struct ndctl_bus {
 	int dimms_init;
 	int regions_init;
 	int has_nfit;
+	int has_of_node;
 	char *bus_path;
 	char *bus_buf;
 	size_t buf_len;
@@ -352,6 +353,7 @@ extern struct ndctl_dimm_ops * const intel_dimm_ops;
 extern struct ndctl_dimm_ops * const hpe1_dimm_ops;
 extern struct ndctl_dimm_ops * const msft_dimm_ops;
 extern struct ndctl_dimm_ops * const hyperv_dimm_ops;
+extern struct ndctl_dimm_ops * const papr_dimm_ops;
 
 static inline struct ndctl_bus *cmd_to_bus(struct ndctl_cmd *cmd)
 {
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 2580f433ade8..daf11b8ce4ea 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -119,6 +119,7 @@ struct ndctl_bus *ndctl_bus_get_next(struct ndctl_bus *bus);
              bus = ndctl_bus_get_next(bus))
 struct ndctl_ctx *ndctl_bus_get_ctx(struct ndctl_bus *bus);
 int ndctl_bus_has_nfit(struct ndctl_bus *bus);
+int ndctl_bus_has_of_node(struct ndctl_bus *bus);
 unsigned int ndctl_bus_get_major(struct ndctl_bus *bus);
 unsigned int ndctl_bus_get_minor(struct ndctl_bus *bus);
 const char *ndctl_bus_get_devname(struct ndctl_bus *bus);
diff --git a/ndctl/ndctl.h b/ndctl/ndctl.h
index 008f81cdeb9f..3b64f66d58cc 100644
--- a/ndctl/ndctl.h
+++ b/ndctl/ndctl.h
@@ -263,6 +263,7 @@ struct nd_cmd_pkg {
 #define NVDIMM_FAMILY_HPE2 2
 #define NVDIMM_FAMILY_MSFT 3
 #define NVDIMM_FAMILY_HYPERV 4
+#define NVDIMM_FAMILY_PAPR 5
 
 #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
 					struct nd_cmd_pkg)
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v5 3/6] libndctl: Introduce new dimm-ops dimm_init() & dimm_uninit()

[Vaibhav Jain]

There are scenarios when a dimm-provider need to allocate some
per-dimm data that can be quickly retrieved. This data can be used to
cache data that spans multiple 'struct ndctl_cmd' submissions.

Unfortunately currently in libnvdimm there is no easy way to implement
this. Even if this data is some how stored in some field of 'struct
ndctl_dimm', managing its lifetime is a challenge.

To solve this problem, the patch proposes a new member 'struct
ndctl_dimm.dimm_user_data' to store per-dimm data interpretation of
which is specific to a dimm-provider. Also two new dimm-ops namely
dimm_init() & dimm_uninit() are introduced that can be used to manage
the lifetime of this per-dimm data.

Semantics
=========
int (*dimm_init)(struct ndctl_dimm *):

This callback will be called just after dimm-probe inside add_dimm()
is completed. Dimm-providers should use this callback to allocate
per-dimm data and assign it to 'struct ndctl_dimm.dimm_user_data'
member. In case this function returns an error, dimm initialization is
halted and errors out.

void (*dimm_uninit)(struct ndctl_dimm *):

This callback will be called during free_dimm() and is only called if
previous call to 'dimm_ops->dimm_init()' had reported no
error. Dimm-providers should use this callback to unallocate and
cleanup 'dimm_user_data'.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5:
* None

v3..v4:
* None

v2..v3:
* None

v1..v2:
* Changed the patch order
---
 ndctl/lib/libndctl.c | 11 +++++++++++
 ndctl/lib/private.h  |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index a52c2e208fbe..8d226abf7495 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -598,6 +598,11 @@ static void free_dimm(struct ndctl_dimm *dimm)
 {
 	if (!dimm)
 		return;
+
+	/* If needed call the dimm uninitialization function */
+	if (dimm->ops && dimm->ops->dimm_uninit)
+		dimm->ops->dimm_uninit(dimm);
+
 	free(dimm->unique_id);
 	free(dimm->dimm_buf);
 	free(dimm->dimm_path);
@@ -1714,8 +1719,14 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 	if (dimm->cmd_family == NVDIMM_FAMILY_PAPR)
 		dimm->ops = papr_dimm_ops;
 
+	/* Call the dimm initialization function if needed */
+	if (!rc && dimm->ops && dimm->ops->dimm_init)
+		rc = dimm->ops->dimm_init(dimm);
+
  out:
 	if (rc) {
+		/* Ensure dimm_uninit() is not called during free_dimm() */
+		dimm->ops = NULL;
 		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
 		goto err_read;
 	}
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index d90236b1f98b..d0188a97d673 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -99,6 +99,7 @@ struct ndctl_dimm {
 	} flags;
 	int locked;
 	int aliased;
+	void *dimm_user_data;
 	struct list_node list;
 	int formats;
 	int format[0];
@@ -347,6 +348,10 @@ struct ndctl_dimm_ops {
 	int (*fw_update_supported)(struct ndctl_dimm *);
 	int (*xlat_firmware_status)(struct ndctl_cmd *);
 	u32 (*get_firmware_status)(struct ndctl_cmd *);
+	/* Called just after dimm is initialized and probed */
+	int (*dimm_init)(struct ndctl_dimm *);
+	/* Called just before struct ndctl_dimm is de-allocated */
+	void (*dimm_uninit)(struct ndctl_dimm *);
 };
 
 extern struct ndctl_dimm_ops * const intel_dimm_ops;
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v5 4/6] libndctl,papr_scm: Add definitions for PAPR nvdimm specific methods

[Vaibhav Jain]

Pull the kernel definition of PAPR nvdimm specific methods which is
located in the proposed kernel tree patches at Ref[1] & [2].

Also add instance of 'struct nd_pdsm_cmd_pkg' to 'struct ndctl_cmd'
named as 'papr'.

References:
[1] "powerpc/papr_scm: Add support for reporting nvdimm health"
https://lore.kernel.org/linux-nvdimm/20200529214719.223344-1-vaibhav@linux.ibm.com

[2] "ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods"
https://lore.kernel.org/linux-nvdimm/20200529214719.223344-5-vaibhav@linux.ibm.com

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5:
* Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
* Updated 'papr_pdsm.h' to recent kernel-uapi version.

v3..v4:
* Updated the definition of 'struct nd_pdsm_cmd_pkg' and
  pdsm_cmd_to_payload() to remove 'payload_offset' field. [ Aneesh ]

v2..v3:
* Added instance of 'struct nd_pdsm_cmd_pkg' to 'struct ndctl_cmd'
* Updated commit description.
* Updated the papr_scm_pdsm.h header to recently proposed kernel
  version.

v1..v2:
* Switched from obsolete papr_scm_dsm.h to recent papr_scm_pdsm.h that
  describes the ND_CMD_CALL interface between libndctl and papr_scm
  module.
---
 ndctl/lib/papr.c      |   1 +
 ndctl/lib/papr_pdsm.h | 175 ++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/private.h   |   2 +
 3 files changed, 178 insertions(+)
 create mode 100644 ndctl/lib/papr_pdsm.h

diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
index 5ddf2755a950..4a40979d228a 100644
--- a/ndctl/lib/papr.c
+++ b/ndctl/lib/papr.c
@@ -17,6 +17,7 @@
 #include <ndctl.h>
 #include <ndctl/libndctl.h>
 #include <lib/private.h>
+#include <papr_pdsm.h>
 
 static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
 {
diff --git a/ndctl/lib/papr_pdsm.h b/ndctl/lib/papr_pdsm.h
new file mode 100644
index 000000000000..cf871dabcb65
--- /dev/null
+++ b/ndctl/lib/papr_pdsm.h
@@ -0,0 +1,175 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
+ *
+ * (C) Copyright IBM 2020
+ *
+ * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
+ */
+
+#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
+#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
+
+#include <linux/types.h>
+
+/*
+ * PDSM Envelope:
+ *
+ * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
+ * envelope which consists of a header and user-defined payload sections.
+ * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
+ * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
+ * There is reserved field that can used to introduce new fields to the
+ * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
+ * lies at a 8-byte boundary.
+ *
+ *  +-------------+---------------------+---------------------------+
+ *  |   64-Bytes  |       16-Bytes      |       Max 176-Bytes       |
+ *  +-------------+---------------------+---------------------------+
+ *  |               nd_pdsm_cmd_pkg     |                           |
+ *  |-------------+                     |                           |
+ *  |  nd_cmd_pkg |                     |                           |
+ *  +-------------+---------------------+---------------------------+
+ *  | nd_family   |                     |                           |
+ *  | nd_size_out | cmd_status          |                           |
+ *  | nd_size_in  | payload_version     |     payload               |
+ *  | nd_command  | reserved            |                           |
+ *  | nd_fw_size  |                     |                           |
+ *  +-------------+---------------------+---------------------------+
+ *
+ * PDSM Header:
+ *
+ * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
+ * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
+ * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
+ * contained in 'struct nd_cmd_pkg', the header also has members following
+ * members:
+ *
+ * 'cmd_status'		: (Out) Errors if any encountered while servicing PDSM.
+ * 'payload_version'	: (In/Out) Version number associated with the payload.
+ * 'reserved'		: Not used and reserved for future.
+ *
+ * PDSM Payload:
+ *
+ * The layout of the PDSM Payload is defined by various structs shared between
+ * papr_scm and libndctl so that contents of payload can be interpreted. During
+ * servicing of a PDSM the papr_scm module will read input args from the payload
+ * field by casting its contents to an appropriate struct pointer based on the
+ * PDSM command. Similarly the output of servicing the PDSM command will be
+ * copied to the payload field using the same struct.
+ *
+ * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
+ * leaves around 176 bytes for the envelope payload (ignoring any padding that
+ * the compiler may silently introduce).
+ *
+ * Payload Version:
+ *
+ * A 'payload_version' field is present in PDSM header that indicates a specific
+ * version of the structure present in PDSM Payload for a given PDSM command.
+ * This provides backward compatibility in case the PDSM Payload structure
+ * evolves and different structures are supported by 'papr_scm' and 'libndctl'.
+ *
+ * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the version
+ * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
+ * module when servicing the PDSM envelope checks the 'payload_version' and then
+ * uses 'payload struct version' == MIN('payload_version field',
+ * 'max payload-struct-version supported by papr_scm') to service the PDSM.
+ * After servicing the PDSM, 'papr_scm' put the negotiated version of payload
+ * struct in returned 'payload_version' field.
+ *
+ * Libndctl on receiving the envelope back from papr_scm again checks the
+ * 'payload_version' field and based on it use the appropriate version dsm
+ * struct to parse the results.
+ *
+ * Backward Compatibility:
+ *
+ * Above scheme of exchanging different versioned PDSM struct between libndctl
+ * and papr_scm should provide backward compatibility until following two
+ * assumptions/conditions when defining new PDSM structs hold:
+ *
+ * Let T(X) = { set of attributes in PDSM struct 'T' versioned X }
+ *
+ * 1. T(X) is a proper subset of T(Y) if Y > X.
+ *    i.e Each new version of PDSM struct should retain existing struct
+ *    attributes from previous version
+ *
+ * 2. If an entity (libndctl or papr_scm) supports a PDSM struct T(X) then
+ *    it should also support T(1), T(2)...T(X - 1).
+ *    i.e When adding support for new version of a PDSM struct, libndctl
+ *    and papr_scm should retain support of the existing PDSM struct
+ *    version they support.
+ */
+
+/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
+struct nd_pdsm_cmd_pkg {
+	struct nd_cmd_pkg hdr;	/* Package header containing sub-cmd */
+	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
+	__u16 reserved[5];	/* Ignored and to be used in future */
+	__u16 payload_version;	/* In/Out: version of the payload */
+	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
+} __attribute__((packed));
+
+/*
+ * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
+ * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
+ */
+enum papr_pdsm {
+	PAPR_PDSM_MIN = 0x0,
+	PAPR_PDSM_HEALTH,
+	PAPR_PDSM_MAX,
+};
+
+/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
+static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
+{
+	return (struct nd_pdsm_cmd_pkg *) cmd;
+}
+
+/* Return the payload pointer for a given pcmd */
+static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
+{
+	if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
+		return NULL;
+	else
+		return (void *)(pcmd->payload);
+}
+
+/* Various nvdimm health indicators */
+#define PAPR_PDSM_DIMM_HEALTHY       0
+#define PAPR_PDSM_DIMM_UNHEALTHY     1
+#define PAPR_PDSM_DIMM_CRITICAL      2
+#define PAPR_PDSM_DIMM_FATAL         3
+
+/*
+ * Struct exchanged between kernel & ndctl in for PAPR_PDSM_HEALTH
+ * Various flags indicate the health status of the dimm.
+ *
+ * dimm_unarmed		: Dimm not armed. So contents wont persist.
+ * dimm_bad_shutdown	: Previous shutdown did not persist contents.
+ * dimm_bad_restore	: Contents from previous shutdown werent restored.
+ * dimm_scrubbed	: Contents of the dimm have been scrubbed.
+ * dimm_locked		: Contents of the dimm cant be modified until CEC reboot
+ * dimm_encrypted	: Contents of dimm are encrypted.
+ * dimm_health		: Dimm health indicator. One of PAPR_PDSM_DIMM_XXXX
+ */
+struct nd_papr_pdsm_health_v1 {
+	__u8 dimm_unarmed;
+	__u8 dimm_bad_shutdown;
+	__u8 dimm_bad_restore;
+	__u8 dimm_scrubbed;
+	__u8 dimm_locked;
+	__u8 dimm_encrypted;
+	__u16 dimm_health;
+} __attribute__((packed));
+
+/*
+ * Typedef the current struct for dimm_health so that any application
+ * or kernel recompiled after introducing a new version automatically
+ * supports the new version.
+ */
+#define nd_papr_pdsm_health nd_papr_pdsm_health_v1
+
+/* Current version number for the dimm health struct */
+#define ND_PAPR_PDSM_HEALTH_VERSION 1
+
+#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index d0188a97d673..9b40e0f8761c 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -32,6 +32,7 @@
 #include "hpe1.h"
 #include "msft.h"
 #include "hyperv.h"
+#include "papr_pdsm.h"
 #include "libndctl-nfit.h"
 
 struct nvdimm_data {
@@ -278,6 +279,7 @@ struct ndctl_cmd {
 		struct ndn_pkg_msft msft[0];
 		struct nd_pkg_hyperv hyperv[0];
 		struct nd_pkg_intel intel[0];
+		struct nd_pdsm_cmd_pkg papr[0];
 		struct nd_cmd_get_config_size get_size[0];
 		struct nd_cmd_get_config_data_hdr get_data[0];
 		struct nd_cmd_set_config_hdr set_data[0];
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v5 5/6] papr: Add scaffolding to issue and handle PDSM requests

[Vaibhav Jain]

This patch implement necessary infrastructure inside 'papr.c' to
issue and handle PDSM requests. Changes implemented are:

* Implement dimm initialization/un-initialization functions
  papr_dimm_init()/unint() to allocate a per-dimm 'struct dimm_priv'
  instance.

* New helper function allocate_cmd() to allocate command packages for
  a specific PDSM command and payload size.

* New function update_dimm_state() to parse a given command payload
  and update per dimm 'struct dimm_priv'.

* Provide an implementation of 'dimm_ops->smart_get_flags' to send the
  submitted instance of 'struct ndctl_cmd' to update_dimm_state().

* Logging helpers for papr_scm that use the underlying libndctl
  provided logging.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5:
* Updated papr_dbg/err macros to remove the 'scm' suffix.
* s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
* s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
* Minor changes to various comments and patch-description to remove
  usage of term 'scm'.

v3..v4:
* Remove the initialization of 'nd_pdsm_cmd_pkg.payload_offset' field
  from allocate_cmd(). [ Aneesh ]

v2..v3:
* None

v1..v2:
* Added new dimm callback 'papr_get_firmware_status'
* Switched to new papr_scm interface as described by papr_scm_dsm.h
* Changed the case of logging functions [ Santosh Sivaraj ]
* Removed redundant logging functions.
* Minor updates to patch description to s/DSM/PDSM/
---
 ndctl/lib/papr.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 184 insertions(+)

diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
index 4a40979d228a..1b7870beb631 100644
--- a/ndctl/lib/papr.c
+++ b/ndctl/lib/papr.c
@@ -19,6 +19,32 @@
 #include <lib/private.h>
 #include <papr_pdsm.h>
 
+/* Utility logging maros for simplify logging */
+#define papr_dbg(_dimm_, _format_str_, ...) dbg(_dimm_->bus->ctx,	\
+					      "papr:"#_format_str_,	\
+					      ##__VA_ARGS__)
+#define papr_err(_dimm_, _format_str_, ...) err(_dimm_->bus->ctx,	\
+					      "papr:"#_format_str_,	\
+					      ##__VA_ARGS__)
+
+/* Helpers to evaluate the size of PDSM envelope */
+/* Calculate the pdsm header size */
+#define ND_PDSM_ENVELOPE_CONTENT_HDR_SIZE \
+	(sizeof(struct nd_pdsm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
+
+/* Given a type calculate envelope-content size (papr_pdsm-header + payload) */
+#define ND_PDSM_ENVELOPE_CONTENT_SIZE(_type_)	\
+	(sizeof(_type_) + ND_PDSM_ENVELOPE_CONTENT_HDR_SIZE)
+
+/* Command flags to indicate if a given command is parsed of not */
+#define CMD_PKG_SUBMITTED 1
+#define CMD_PKG_PARSED 2
+
+/* Per dimm data. Holds per-dimm data parsed from the cmd_pkgs */
+struct dimm_priv {
+	/* Empty for now */
+};
+
 static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
 {
 	/* Handle this separately to support monitor mode */
@@ -28,6 +54,164 @@ static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
 	return !!(dimm->cmd_mask & (1ULL << cmd));
 }
 
+static __u64 pcmd_to_pdsm(const struct nd_pdsm_cmd_pkg *pcmd)
+{
+	return pcmd->hdr.nd_command;
+}
+
+static u32 papr_get_firmware_status(struct ndctl_cmd *cmd)
+{
+	const struct nd_pdsm_cmd_pkg *pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
+
+	return (u32) pcmd->cmd_status;
+}
+
+/* Verify if the given command is supported and valid */
+static bool cmd_is_valid(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
+{
+	const struct nd_pdsm_cmd_pkg *pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
+
+	if (dimm == NULL)
+		return false;
+
+	if (cmd == NULL) {
+		papr_err(dimm, "Invalid command\n");
+		return false;
+	}
+
+	/* Verify the command family */
+	if (pcmd->hdr.nd_family != NVDIMM_FAMILY_PAPR) {
+		papr_err(dimm, "Invalid command family:0x%016llx\n",
+			 pcmd->hdr.nd_family);
+		return false;
+	}
+
+	/* Verify the PDSM */
+	if (pcmd_to_pdsm(pcmd) <= PAPR_PDSM_MIN ||
+	    pcmd_to_pdsm(pcmd) >= PAPR_PDSM_MAX) {
+		papr_err(dimm, "Invalid command :0x%016llx\n",
+			 pcmd->hdr.nd_command);
+		return false;
+	}
+
+	return true;
+}
+
+/* Parse a command payload and update dimm flags/private data */
+static int update_dimm_stats(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
+{
+	const struct nd_pdsm_cmd_pkg *pcmd;
+
+	if (!cmd_is_valid(dimm, cmd))
+		return -EINVAL;
+
+	/*
+	 * Silently prevent parsing of an already parsed ndctl_cmd else
+	 * mark the command as parsed.
+	 */
+	if (cmd->status >= CMD_PKG_PARSED) {
+		return 0;
+	} else if (cmd->status < 0) {
+		papr_err(dimm, "Command error %d\n", cmd->status);
+		return -ENXIO;
+	}
+
+	/* Mark the command as parsed */
+	cmd->status = CMD_PKG_PARSED;
+
+	/* Get the pdsm request and handle it */
+	pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
+	switch (pcmd_to_pdsm(pcmd)) {
+	default:
+		papr_err(dimm, "Unhandled pdsm-request 0x%016llx\n",
+			 pcmd_to_pdsm(pcmd));
+		return -ENOENT;
+	}
+}
+
+/* Allocate a struct ndctl_cmd for given pdsm request with payload size */
+static struct ndctl_cmd *allocate_cmd(struct ndctl_dimm *dimm,
+				      __u64 pdsm_cmd, size_t payload_size,
+				      uint16_t payload_version)
+{
+	struct ndctl_cmd *cmd;
+	struct nd_pdsm_cmd_pkg *pcmd;
+	size_t size;
+
+	size = sizeof(struct ndctl_cmd) +
+		sizeof(struct nd_pdsm_cmd_pkg) + payload_size;
+	cmd = calloc(1, size);
+	if (!cmd)
+		return NULL;
+	pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
+
+	ndctl_cmd_ref(cmd);
+	cmd->dimm = dimm;
+	cmd->type = ND_CMD_CALL;
+	cmd->size = size;
+	cmd->status = CMD_PKG_SUBMITTED;
+	cmd->get_firmware_status = &papr_get_firmware_status;
+
+	/* Populate the nd_cmd_pkg contained in nd_pdsm_cmd_pkg */
+	pcmd->hdr.nd_family = NVDIMM_FAMILY_PAPR;
+	pcmd->hdr.nd_command = pdsm_cmd;
+
+	pcmd->payload_version = payload_version;
+
+	/* Keep payload size empty. To be populated by called */
+	pcmd->hdr.nd_fw_size = 0;
+	pcmd->hdr.nd_size_out = 0;
+	pcmd->hdr.nd_size_in = 0;
+
+	return cmd;
+}
+
+static unsigned int papr_smart_get_flags(struct ndctl_cmd *cmd)
+{
+	/* In case of error return empty flags * */
+	if (update_dimm_stats(cmd->dimm, cmd))
+		return 0;
+
+	/* Return empty flags for now as no DSM support */
+	return 0;
+}
+
+static int papr_dimm_init(struct ndctl_dimm *dimm)
+{
+	struct dimm_priv *p;
+
+	if (dimm->dimm_user_data) {
+		papr_dbg(dimm, "Dimm already initialized !!\n");
+		return 0;
+	}
+
+	p = calloc(1, sizeof(struct dimm_priv));
+	if (!p) {
+		papr_err(dimm, "Unable to allocate memory for dimm-private\n");
+		return -1;
+	}
+
+	dimm->dimm_user_data = p;
+	return 0;
+}
+
+static void papr_dimm_uninit(struct ndctl_dimm *dimm)
+{
+	struct dimm_priv *p = dimm->dimm_user_data;
+
+	if (!p) {
+		papr_dbg(dimm, "Dimm already un-initialized !!\n");
+		return;
+	}
+
+	dimm->dimm_user_data = NULL;
+	free(p);
+}
+
 struct ndctl_dimm_ops * const papr_dimm_ops = &(struct ndctl_dimm_ops) {
 	.cmd_is_supported = papr_cmd_is_supported,
+	.dimm_init = papr_dimm_init,
+	.dimm_uninit = papr_dimm_uninit,
+	.smart_get_flags = papr_smart_get_flags,
+	.get_firmware_status =  papr_get_firmware_status,
 };
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for PAPR_PDSM_HEALTH

[Vaibhav Jain]

Add support for reporting DIMM health and shutdown state by issuing
PAPR_PDSM_HEALTH request to papr_scm module. It returns an
instance of 'struct nd_papr_pdsm_health' as defined in
'papr_pdsm.h'. The patch provides support for dimm-ops
'new_smart', 'smart_get_health' & 'smart_get_shutdown_state' as newly
introduced functions papr_new_smart_health(), papr_smart_get_health()
& papr_smart_get_shutdown_state() respectively. These callbacks should
enable ndctl to report DIMM health.

Also a new member 'struct dimm_priv.health' is introduced which holds
the current health status of the dimm. This member is set inside newly
added function 'update_dimm_health_v1()' which parses the v1 payload
returned by the kernel after servicing PAPR_PDSM_HEALTH. The
function will also update dimm-flags viz 'struct ndctl_dimm.flags.f_*'
based on the flags set in the returned payload.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5:
* Updated patch description to reflect updated names of struct and
  defines that have the term 'scm' removed.

v3..v4:
* None

v2..v3:
* None

v1..v2:
* Squashed patch to report nvdimm bad shutdown state with this patch.
* Switched to new structs/enums as defined in papr_scm_pdsm.h
---
 ndctl/lib/papr.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c
index 1b7870beb631..cb7ff9e0d5bd 100644
--- a/ndctl/lib/papr.c
+++ b/ndctl/lib/papr.c
@@ -42,7 +42,9 @@
 
 /* Per dimm data. Holds per-dimm data parsed from the cmd_pkgs */
 struct dimm_priv {
-	/* Empty for now */
+
+	/* Cache the dimm health status */
+	struct nd_papr_pdsm_health health;
 };
 
 static bool papr_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
@@ -97,6 +99,43 @@ static bool cmd_is_valid(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
 	return true;
 }
 
+/*
+ * Parse the nd_papr_pdsm_health_v1 payload embedded in ndctl_cmd and
+ * update dimm health/flags
+ */
+static int update_dimm_health_v1(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
+{
+	struct nd_pdsm_cmd_pkg *pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
+	struct dimm_priv *p = dimm->dimm_user_data;
+	const struct nd_papr_pdsm_health_v1 *health =
+		pdsm_cmd_to_payload(pcmd);
+
+	/* Update the dimm flags */
+	dimm->flags.f_arm = health->dimm_unarmed;
+	dimm->flags.f_flush = health->dimm_bad_shutdown;
+	dimm->flags.f_restore = health->dimm_bad_restore;
+	dimm->flags.f_smart = (health->dimm_health != 0);
+
+	/* Cache the dimm health information */
+	memcpy(&p->health, health, sizeof(*health));
+	return 0;
+}
+
+/* Check payload version returned and pass the packet to appropriate handler */
+static int update_dimm_health(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
+{
+	const struct nd_pdsm_cmd_pkg *pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
+
+	if (pcmd->payload_version == 1)
+		return update_dimm_health_v1(dimm, cmd);
+
+	/* unknown version */
+	papr_err(dimm, "Unknown payload version for dimm_health.\n");
+	papr_dbg(dimm, "dimm_health payload Ver=%d, Supported=%d\n",
+		 pcmd->payload_version, ND_PAPR_PDSM_HEALTH_VERSION);
+	return -EINVAL;
+}
+
 /* Parse a command payload and update dimm flags/private data */
 static int update_dimm_stats(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
 {
@@ -122,6 +161,8 @@ static int update_dimm_stats(struct ndctl_dimm *dimm, struct ndctl_cmd *cmd)
 	/* Get the pdsm request and handle it */
 	pcmd = nd_to_pdsm_cmd_pkg(cmd->pkg);
 	switch (pcmd_to_pdsm(pcmd)) {
+	case PAPR_PDSM_HEALTH:
+		return update_dimm_health(dimm, cmd);
 	default:
 		papr_err(dimm, "Unhandled pdsm-request 0x%016llx\n",
 			 pcmd_to_pdsm(pcmd));
@@ -166,14 +207,54 @@ static struct ndctl_cmd *allocate_cmd(struct ndctl_dimm *dimm,
 	return cmd;
 }
 
+static struct ndctl_cmd *papr_new_smart_health(struct ndctl_dimm *dimm)
+{
+	struct ndctl_cmd *cmd_ret;
+
+	cmd_ret = allocate_cmd(dimm, PAPR_PDSM_HEALTH,
+			       sizeof(struct nd_papr_pdsm_health),
+			       ND_PAPR_PDSM_HEALTH_VERSION);
+	if (!cmd_ret) {
+		papr_err(dimm, "Unable to allocate smart_health command\n");
+		return NULL;
+	}
+
+	cmd_ret->pkg[0].nd_size_out = ND_PDSM_ENVELOPE_CONTENT_SIZE(
+		struct nd_papr_pdsm_health);
+
+	return cmd_ret;
+}
+
+static unsigned int papr_smart_get_health(struct ndctl_cmd *cmd)
+{
+	struct dimm_priv *p = cmd->dimm->dimm_user_data;
+
+	/*
+	 * Update the dimm stats and use some math to return one of
+	 * defined ND_SMART_*_HEALTH values
+	 */
+	if (update_dimm_stats(cmd->dimm, cmd) || !p->health.dimm_health)
+		return 0;
+	else
+		return 1 << (p->health.dimm_health - 1);
+}
+
+static unsigned int papr_smart_get_shutdown_state(struct ndctl_cmd *cmd)
+{
+	struct dimm_priv *p = cmd->dimm->dimm_user_data;
+
+	/* Update dimm state and return f_flush */
+	return update_dimm_stats(cmd->dimm, cmd) ?
+		0 : p->health.dimm_bad_shutdown;
+}
+
 static unsigned int papr_smart_get_flags(struct ndctl_cmd *cmd)
 {
 	/* In case of error return empty flags * */
 	if (update_dimm_stats(cmd->dimm, cmd))
 		return 0;
 
-	/* Return empty flags for now as no DSM support */
-	return 0;
+	return ND_SMART_HEALTH_VALID | ND_SMART_SHUTDOWN_VALID;
 }
 
 static int papr_dimm_init(struct ndctl_dimm *dimm)
@@ -214,4 +295,7 @@ struct ndctl_dimm_ops * const papr_dimm_ops = &(struct ndctl_dimm_ops) {
 	.dimm_uninit = papr_dimm_uninit,
 	.smart_get_flags = papr_smart_get_flags,
 	.get_firmware_status =  papr_get_firmware_status,
+	.new_smart = papr_new_smart_health,
+	.smart_get_health = papr_smart_get_health,
+	.smart_get_shutdown_state = papr_smart_get_shutdown_state,
 };
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v5 1/6] libndctl: Refactor out add_dimm() to handle NFIT specific init

[Verma, Vishal L]

On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
> Presently add_dimm() only probes dimms that support NFIT/ACPI. Hence
> this patch refactors this functionality into two functions namely
> add_dimm() and add_nfit_dimm(). Function add_dimm() performs
> allocation and common 'struct ndctl_dimm' initialization and depending
> on whether the dimm-bus supports NIFT, calls add_nfit_dimm(). Once
> the probe is completed based on the value of 'ndctl_dimm.cmd_family'
> appropriate dimm-ops are assigned to the dimm.
> 
> In case dimm-bus is of unknown type or doesn't support NFIT the
> initialization still continues, with no dimm-ops assigned to the
> 'struct ndctl_dimm' there-by limiting the functionality available.
> 
> This patch shouldn't introduce any behavioral change.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
> 
> v4..v5:
> * None
> 
> v3..v4:
> * None
> 
> v2..v3:
> * None
> 
> v1..v2:
> * None
> ---
>  ndctl/lib/libndctl.c | 193 +++++++++++++++++++++++++-----------------
> -
>  1 file changed, 112 insertions(+), 81 deletions(-)

Hi Vaibhav,

This mostly looks good, one minor nit below.

> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ee737cbbfe3e..d76dbf7e17de 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1441,82 +1441,15 @@ 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 *add_dimm(void *parent, int id, const char *dimm_base)
> +static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>  {
> -	int formats, i;
> -	struct ndctl_dimm *dimm;
> +	int i, rc = -1;
>  	char buf[SYSFS_ATTR_SIZE];
> -	struct ndctl_bus *bus = parent;
> -	struct ndctl_ctx *ctx = bus->ctx;
> +	struct ndctl_ctx *ctx = dimm->bus->ctx;
>  	char *path = calloc(1, strlen(dimm_base) + 100);
>  
>  	if (!path)

<snip>

> +		return -1;

You should generally avoid returning a plain '-1'. This really wants to
return an -ENOMEM.
>  
>  	/*
>  	 * 'unique_id' may not be available on older kernels, so don't
> @@ -1582,24 +1515,15 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>  	sprintf(path, "%s/nfit/family", dimm_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->cmd_family = strtoul(buf, NULL, 0);
> -	if (dimm->cmd_family == NVDIMM_FAMILY_INTEL)
> -		dimm->ops = intel_dimm_ops;
> -	if (dimm->cmd_family == NVDIMM_FAMILY_HPE1)
> -		dimm->ops = hpe1_dimm_ops;
> -	if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
> -		dimm->ops = msft_dimm_ops;
> -	if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
> -		dimm->ops = hyperv_dimm_ops;
>  
>  	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>  
> -	dimm->formats = formats;
>  	sprintf(path, "%s/nfit/format", dimm_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		dimm->format[0] = strtoul(buf, NULL, 0);
> -	for (i = 1; i < formats; i++) {
> +	for (i = 1; i < dimm->formats; i++) {
>  		sprintf(path, "%s/nfit/format%d", dimm_base, i);
>  		if (sysfs_read_attr(ctx, path, buf) == 0)
>  			dimm->format[i] = strtoul(buf, NULL, 0);
> @@ -1610,7 +1534,114 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)

<snip>

>   out:
> +	if (rc) {
> +		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);

Returning -1 being awkward is especially true when it might end up
getting printed as part of an error message like this. Indeed, you
shouldn't even print 'rc' as a number, which then needs to get looked up
- use strerror to turn it into a string.

Additionally, "dimm:<number>" is pretty nonstandard in ndctl, we
normally use the 'devname' for error messages. How about something like:

	err(ctx, "%s: probe failed: %s\n", ndctl_dimm_get_devname(dimm),
		strerror(-rc));

> +		goto err_read;
> +	}
> +
>  	list_add(&bus->dimms, &dimm->list);
>  	free(path);
>  
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Verma, Vishal L]

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"

> +}
> +
>  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).

>  /**
>   * 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.

> +
> +

Nit: double newline here 

> +	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

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

> 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.

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

Re: [ndctl PATCH v5 1/6] libndctl: Refactor out add_dimm() to handle NFIT specific init

[Vaibhav Jain]

Hi Vishal,

Thanks for reviewing this patch. My responses below:

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

> On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
>> Presently add_dimm() only probes dimms that support NFIT/ACPI. Hence
>> this patch refactors this functionality into two functions namely
>> add_dimm() and add_nfit_dimm(). Function add_dimm() performs
>> allocation and common 'struct ndctl_dimm' initialization and depending
>> on whether the dimm-bus supports NIFT, calls add_nfit_dimm(). Once
>> the probe is completed based on the value of 'ndctl_dimm.cmd_family'
>> appropriate dimm-ops are assigned to the dimm.
>> 
>> In case dimm-bus is of unknown type or doesn't support NFIT the
>> initialization still continues, with no dimm-ops assigned to the
>> 'struct ndctl_dimm' there-by limiting the functionality available.
>> 
>> This patch shouldn't introduce any behavioral change.
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>> 
>> v4..v5:
>> * None
>> 
>> v3..v4:
>> * None
>> 
>> v2..v3:
>> * None
>> 
>> v1..v2:
>> * None
>> ---
>>  ndctl/lib/libndctl.c | 193 +++++++++++++++++++++++++-----------------
>> -
>>  1 file changed, 112 insertions(+), 81 deletions(-)
>
> Hi Vaibhav,
>
> This mostly looks good, one minor nit below.
>
>> 
>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
>> index ee737cbbfe3e..d76dbf7e17de 100644
>> --- a/ndctl/lib/libndctl.c
>> +++ b/ndctl/lib/libndctl.c
>> @@ -1441,82 +1441,15 @@ 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 *add_dimm(void *parent, int id, const char *dimm_base)
>> +static int add_nfit_dimm(struct ndctl_dimm *dimm, const char *dimm_base)
>>  {
>> -	int formats, i;
>> -	struct ndctl_dimm *dimm;
>> +	int i, rc = -1;
>>  	char buf[SYSFS_ATTR_SIZE];
>> -	struct ndctl_bus *bus = parent;
>> -	struct ndctl_ctx *ctx = bus->ctx;
>> +	struct ndctl_ctx *ctx = dimm->bus->ctx;
>>  	char *path = calloc(1, strlen(dimm_base) + 100);
>>  
>>  	if (!path)
>
> <snip>
>
>> +		return -1;
>
> You should generally avoid returning a plain '-1'. This really wants to
> return an -ENOMEM.
If calloc fails, variable 'errno' will already be set to ENOMEM. Hence
didn't explicitly return -ENOMEM. But fair suggestion will update
this in v6.

>>  
>>  	/*
>>  	 * 'unique_id' may not be available on older kernels, so don't
>> @@ -1582,24 +1515,15 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>>  	sprintf(path, "%s/nfit/family", dimm_base);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->cmd_family = strtoul(buf, NULL, 0);
>> -	if (dimm->cmd_family == NVDIMM_FAMILY_INTEL)
>> -		dimm->ops = intel_dimm_ops;
>> -	if (dimm->cmd_family == NVDIMM_FAMILY_HPE1)
>> -		dimm->ops = hpe1_dimm_ops;
>> -	if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
>> -		dimm->ops = msft_dimm_ops;
>> -	if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
>> -		dimm->ops = hyperv_dimm_ops;
>>  
>>  	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->nfit_dsm_mask = strtoul(buf, NULL, 0);
>>  
>> -	dimm->formats = formats;
>>  	sprintf(path, "%s/nfit/format", dimm_base);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		dimm->format[0] = strtoul(buf, NULL, 0);
>> -	for (i = 1; i < formats; i++) {
>> +	for (i = 1; i < dimm->formats; i++) {
>>  		sprintf(path, "%s/nfit/format%d", dimm_base, i);
>>  		if (sysfs_read_attr(ctx, path, buf) == 0)
>>  			dimm->format[i] = strtoul(buf, NULL, 0);
>> @@ -1610,7 +1534,114 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>
> <snip>
>
>>   out:
>> +	if (rc) {
>> +		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
>
> Returning -1 being awkward is especially true when it might end up
> getting printed as part of an error message like this. Indeed, you    
> shouldn't even print 'rc' as a number, which then needs to get looked up
> - use strerror to turn it into a string.
>
> Additionally, "dimm:<number>" is pretty nonstandard in ndctl, we
> normally use the 'devname' for error messages. How about something
> like:
>
> 	err(ctx, "%s: probe failed: %s\n", ndctl_dimm_get_devname(dimm),
> 		strerror(-rc));
>
Fair suggestion. Will update this in v6. 
>> +		goto err_read;
>> +	}
>> +
>>  	list_add(&bus->dimms, &dimm->list);
>>  	free(path);
>>  

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

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

[Vaibhav Jain]

"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

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

[Verma, Vishal L]

On Wed, 2020-06-03 at 15:27 +0530, Vaibhav Jain wrote:
> > 
> > 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.
> 
Understood - one more question:

Would it be useful to wrap the 'compatible' check into an API similar to
_has_of_node - say ndctl_bus_is_papr_compatible()? I'm not too strongly
attached this, there is only one user so far after all, but it seemed
like an easy thing that might get copy-pasted around in the future.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Vaibhav Jain]

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

> On Wed, 2020-06-03 at 15:27 +0530, Vaibhav Jain wrote:
>> > 
>> > 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.
>> 
> Understood - one more question:
>
> Would it be useful to wrap the 'compatible' check into an API similar to
> _has_of_node - say ndctl_bus_is_papr_compatible()? I'm not too strongly
> attached this, there is only one user so far after all, but it seemed
> like an easy thing that might get copy-pasted around in the future.

Yes, sounds good to me. Should simplify the add_papr_dimm() function a
bit as I can just call ndctl_bus_is_papr_compatible() and if true then
setup the cmd_family.

Will roll out this change in v6 iteration.

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

RE: [ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for PAPR_PDSM_HEALTH

[Williams, Dan J]

> -----Original Message-----
> From: Vaibhav Jain <vaibhav@linux.ibm.com>
> Sent: Friday, May 29, 2020 3:06 PM
> To: linux-nvdimm@lists.01.org
> Cc: Vaibhav Jain <vaibhav@linux.ibm.com>; Williams, Dan J
> <dan.j.williams@intel.com>; Verma, Vishal L <vishal.l.verma@intel.com>;
> Aneesh Kumar K . V <aneesh.kumar@linux.ibm.com>; Jeff Moyer
> <jmoyer@redhat.com>; Oliver O'Halloran <oohall@gmail.com>; Santosh
> Sivaraj <santosh@fossix.org>; Weiny, Ira <ira.weiny@intel.com>
> Subject: [ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for
> PAPR_PDSM_HEALTH
> 
> Add support for reporting DIMM health and shutdown state by issuing
> PAPR_PDSM_HEALTH request to papr_scm module. It returns an instance of
> 'struct nd_papr_pdsm_health' as defined in 'papr_pdsm.h'. The patch
> provides support for dimm-ops 'new_smart', 'smart_get_health' &
> 'smart_get_shutdown_state' as newly introduced functions
> papr_new_smart_health(), papr_smart_get_health() &
> papr_smart_get_shutdown_state() respectively. These callbacks should
> enable ndctl to report DIMM health.
> 
> Also a new member 'struct dimm_priv.health' is introduced which holds the
> current health status of the dimm. This member is set inside newly added
> function 'update_dimm_health_v1()' which parses the v1 payload returned
> by the kernel after servicing PAPR_PDSM_HEALTH. The function will also
> update dimm-flags viz 'struct ndctl_dimm.flags.f_*'
> based on the flags set in the returned payload.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
> 
> v4..v5:
> * Updated patch description to reflect updated names of struct and
>   defines that have the term 'scm' removed.
> 
> v3..v4:
> * None
> 
> v2..v3:
> * None
> 
> v1..v2:
> * Squashed patch to report nvdimm bad shutdown state with this patch.
> * Switched to new structs/enums as defined in papr_scm_pdsm.h
> ---
>  ndctl/lib/papr.c | 90
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c index
> 1b7870beb631..cb7ff9e0d5bd 100644
> --- a/ndctl/lib/papr.c
> +++ b/ndctl/lib/papr.c
> @@ -42,7 +42,9 @@
> 
>  /* Per dimm data. Holds per-dimm data parsed from the cmd_pkgs */  struct
> dimm_priv {
> -	/* Empty for now */
> +
> +	/* Cache the dimm health status */
> +	struct nd_papr_pdsm_health health;

I don't understand this. The kernel is caching this, why does libndctl need to cache it?
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v5 3/6] libndctl: Introduce new dimm-ops dimm_init() & dimm_uninit()

[Verma, Vishal L]

On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
> There are scenarios when a dimm-provider need to allocate some
> per-dimm data that can be quickly retrieved. This data can be used to
> cache data that spans multiple 'struct ndctl_cmd' submissions.
> 
> Unfortunately currently in libnvdimm there is no easy way to implement
> this. Even if this data is some how stored in some field of 'struct
> ndctl_dimm', managing its lifetime is a challenge.
> 
> To solve this problem, the patch proposes a new member 'struct
> ndctl_dimm.dimm_user_data' to store per-dimm data interpretation of
> which is specific to a dimm-provider. Also two new dimm-ops namely
> dimm_init() & dimm_uninit() are introduced that can be used to manage
> the lifetime of this per-dimm data.
> 
> Semantics
> =========
> int (*dimm_init)(struct ndctl_dimm *):
> 
> This callback will be called just after dimm-probe inside add_dimm()
> is completed. Dimm-providers should use this callback to allocate
> per-dimm data and assign it to 'struct ndctl_dimm.dimm_user_data'
> member. In case this function returns an error, dimm initialization is
> halted and errors out.
> 
> void (*dimm_uninit)(struct ndctl_dimm *):
> 
> This callback will be called during free_dimm() and is only called if
> previous call to 'dimm_ops->dimm_init()' had reported no
> error. Dimm-providers should use this callback to unallocate and
> cleanup 'dimm_user_data'.

I'm not sure I fully understand the need for this whole paradigm - of
creating a private caching area in ndctl_dimm, and having these
init/uninit functions to set it up.

Looking ahead at subsequent patches, the data you're stashing there is
already cached in the kernel or the command payloads. It seems the
caching is maybe just avoiding some ioctl round trips - is that correct?

If so , why not just make all the data retrieval synchronous to the
operation that's requesting it? Health retrieval is generally not a fast
path of any sort, and doing everything synchronously seems much simpler,
and is also what existing nvdimm families do.

> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
> 
> v4..v5:
> * None
> 
> v3..v4:
> * None
> 
> v2..v3:
> * None
> 
> v1..v2:
> * Changed the patch order
> ---
>  ndctl/lib/libndctl.c | 11 +++++++++++
>  ndctl/lib/private.h  |  5 +++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index a52c2e208fbe..8d226abf7495 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -598,6 +598,11 @@ static void free_dimm(struct ndctl_dimm *dimm)
>  {
>  	if (!dimm)
>  		return;
> +
> +	/* If needed call the dimm uninitialization function */
> +	if (dimm->ops && dimm->ops->dimm_uninit)
> +		dimm->ops->dimm_uninit(dimm);
> +
>  	free(dimm->unique_id);
>  	free(dimm->dimm_buf);
>  	free(dimm->dimm_path);
> @@ -1714,8 +1719,14 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>  	if (dimm->cmd_family == NVDIMM_FAMILY_PAPR)
>  		dimm->ops = papr_dimm_ops;
>  
> +	/* Call the dimm initialization function if needed */
> +	if (!rc && dimm->ops && dimm->ops->dimm_init)
> +		rc = dimm->ops->dimm_init(dimm);
> +
>   out:
>  	if (rc) {
> +		/* Ensure dimm_uninit() is not called during free_dimm() */
> +		dimm->ops = NULL;
>  		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
>  		goto err_read;
>  	}
> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> index d90236b1f98b..d0188a97d673 100644
> --- a/ndctl/lib/private.h
> +++ b/ndctl/lib/private.h
> @@ -99,6 +99,7 @@ struct ndctl_dimm {
>  	} flags;
>  	int locked;
>  	int aliased;
> +	void *dimm_user_data;
>  	struct list_node list;
>  	int formats;
>  	int format[0];
> @@ -347,6 +348,10 @@ struct ndctl_dimm_ops {
>  	int (*fw_update_supported)(struct ndctl_dimm *);
>  	int (*xlat_firmware_status)(struct ndctl_cmd *);
>  	u32 (*get_firmware_status)(struct ndctl_cmd *);
> +	/* Called just after dimm is initialized and probed */
> +	int (*dimm_init)(struct ndctl_dimm *);
> +	/* Called just before struct ndctl_dimm is de-allocated */
> +	void (*dimm_uninit)(struct ndctl_dimm *);
>  };
>  
>  extern struct ndctl_dimm_ops * const intel_dimm_ops;
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH v5 3/6] libndctl: Introduce new dimm-ops dimm_init() & dimm_uninit()

[Vaibhav Jain]

Hi Vishal,

Thanks for reviewing this patch. My responses below:

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

> On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
>> There are scenarios when a dimm-provider need to allocate some
>> per-dimm data that can be quickly retrieved. This data can be used to
>> cache data that spans multiple 'struct ndctl_cmd' submissions.
>> 
>> Unfortunately currently in libnvdimm there is no easy way to implement
>> this. Even if this data is some how stored in some field of 'struct
>> ndctl_dimm', managing its lifetime is a challenge.
>> 
>> To solve this problem, the patch proposes a new member 'struct
>> ndctl_dimm.dimm_user_data' to store per-dimm data interpretation of
>> which is specific to a dimm-provider. Also two new dimm-ops namely
>> dimm_init() & dimm_uninit() are introduced that can be used to manage
>> the lifetime of this per-dimm data.
>> 
>> Semantics
>> =========
>> int (*dimm_init)(struct ndctl_dimm *):
>> 
>> This callback will be called just after dimm-probe inside add_dimm()
>> is completed. Dimm-providers should use this callback to allocate
>> per-dimm data and assign it to 'struct ndctl_dimm.dimm_user_data'
>> member. In case this function returns an error, dimm initialization is
>> halted and errors out.
>> 
>> void (*dimm_uninit)(struct ndctl_dimm *):
>> 
>> This callback will be called during free_dimm() and is only called if
>> previous call to 'dimm_ops->dimm_init()' had reported no
>> error. Dimm-providers should use this callback to unallocate and
>> cleanup 'dimm_user_data'.
<snip>
>
> I'm not sure I fully understand the need for this whole paradigm - of
> creating a private caching area in ndctl_dimm, and having these
> init/uninit functions to set it up.
>
> Looking ahead at subsequent patches, the data you're stashing there is
> already cached in the kernel or the command payloads. It seems the
> caching is maybe just avoiding some ioctl round trips - is that
> correct?
Yes, that was the real motivation behind introducing these new
dimm-ops. The primary problem was with fetching the
'life_used_percentage' which in case of papr_scm would have required a
separate ioctl apart from one to fetch nvdimm-health.

With per-dimm data to hold the dimm-health and 'life_used_percentage',
once the ndctl_cmd for fetching nvdimm health is complete I would issue
another ndctl_cmd to fetch the 'life_used_percentage' store these value
in per-dimm data and when 'smart_get_life_used' is called would return
cached value.

>
> If so , why not just make all the data retrieval synchronous to the
> operation that's requesting it? Health retrieval is generally not a fast
> path of any sort, and doing everything synchronously seems much simpler,
> and is also what existing nvdimm families do.
I can probably issue the ndctl_cmd to fetch 'life_used_percentage'
synchronously in 'smart_get_life_used' but introducing per-dimm data
seemed a cleaner approach as it may have wider usefulness.

>
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>> 
>> v4..v5:
>> * None
>> 
>> v3..v4:
>> * None
>> 
>> v2..v3:
>> * None
>> 
>> v1..v2:
>> * Changed the patch order
>> ---
>>  ndctl/lib/libndctl.c | 11 +++++++++++
>>  ndctl/lib/private.h  |  5 +++++
>>  2 files changed, 16 insertions(+)
>> 
>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
>> index a52c2e208fbe..8d226abf7495 100644
>> --- a/ndctl/lib/libndctl.c
>> +++ b/ndctl/lib/libndctl.c
>> @@ -598,6 +598,11 @@ static void free_dimm(struct ndctl_dimm *dimm)
>>  {
>>  	if (!dimm)
>>  		return;
>> +
>> +	/* If needed call the dimm uninitialization function */
>> +	if (dimm->ops && dimm->ops->dimm_uninit)
>> +		dimm->ops->dimm_uninit(dimm);
>> +
>>  	free(dimm->unique_id);
>>  	free(dimm->dimm_buf);
>>  	free(dimm->dimm_path);
>> @@ -1714,8 +1719,14 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>>  	if (dimm->cmd_family == NVDIMM_FAMILY_PAPR)
>>  		dimm->ops = papr_dimm_ops;
>>  
>> +	/* Call the dimm initialization function if needed */
>> +	if (!rc && dimm->ops && dimm->ops->dimm_init)
>> +		rc = dimm->ops->dimm_init(dimm);
>> +
>>   out:
>>  	if (rc) {
>> +		/* Ensure dimm_uninit() is not called during free_dimm() */
>> +		dimm->ops = NULL;
>>  		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
>>  		goto err_read;
>>  	}
>> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
>> index d90236b1f98b..d0188a97d673 100644
>> --- a/ndctl/lib/private.h
>> +++ b/ndctl/lib/private.h
>> @@ -99,6 +99,7 @@ struct ndctl_dimm {
>>  	} flags;
>>  	int locked;
>>  	int aliased;
>> +	void *dimm_user_data;
>>  	struct list_node list;
>>  	int formats;
>>  	int format[0];
>> @@ -347,6 +348,10 @@ struct ndctl_dimm_ops {
>>  	int (*fw_update_supported)(struct ndctl_dimm *);
>>  	int (*xlat_firmware_status)(struct ndctl_cmd *);
>>  	u32 (*get_firmware_status)(struct ndctl_cmd *);
>> +	/* Called just after dimm is initialized and probed */
>> +	int (*dimm_init)(struct ndctl_dimm *);
>> +	/* Called just before struct ndctl_dimm is de-allocated */
>> +	void (*dimm_uninit)(struct ndctl_dimm *);
>>  };
>>  
>>  extern struct ndctl_dimm_ops * const intel_dimm_ops;

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

RE: [ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for PAPR_PDSM_HEALTH

[Vaibhav Jain]

"Williams, Dan J" <dan.j.williams@intel.com> writes:

>> -----Original Message-----
>> From: Vaibhav Jain <vaibhav@linux.ibm.com>
>> Sent: Friday, May 29, 2020 3:06 PM
>> To: linux-nvdimm@lists.01.org
>> Cc: Vaibhav Jain <vaibhav@linux.ibm.com>; Williams, Dan J
>> <dan.j.williams@intel.com>; Verma, Vishal L <vishal.l.verma@intel.com>;
>> Aneesh Kumar K . V <aneesh.kumar@linux.ibm.com>; Jeff Moyer
>> <jmoyer@redhat.com>; Oliver O'Halloran <oohall@gmail.com>; Santosh
>> Sivaraj <santosh@fossix.org>; Weiny, Ira <ira.weiny@intel.com>
>> Subject: [ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for
>> PAPR_PDSM_HEALTH
>> 
>> Add support for reporting DIMM health and shutdown state by issuing
>> PAPR_PDSM_HEALTH request to papr_scm module. It returns an instance of
>> 'struct nd_papr_pdsm_health' as defined in 'papr_pdsm.h'. The patch
>> provides support for dimm-ops 'new_smart', 'smart_get_health' &
>> 'smart_get_shutdown_state' as newly introduced functions
>> papr_new_smart_health(), papr_smart_get_health() &
>> papr_smart_get_shutdown_state() respectively. These callbacks should
>> enable ndctl to report DIMM health.
>> 
>> Also a new member 'struct dimm_priv.health' is introduced which holds the
>> current health status of the dimm. This member is set inside newly added
>> function 'update_dimm_health_v1()' which parses the v1 payload returned
>> by the kernel after servicing PAPR_PDSM_HEALTH. The function will also
>> update dimm-flags viz 'struct ndctl_dimm.flags.f_*'
>> based on the flags set in the returned payload.
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>> 
>> v4..v5:
>> * Updated patch description to reflect updated names of struct and
>>   defines that have the term 'scm' removed.
>> 
>> v3..v4:
>> * None
>> 
>> v2..v3:
>> * None
>> 
>> v1..v2:
>> * Squashed patch to report nvdimm bad shutdown state with this patch.
>> * Switched to new structs/enums as defined in papr_scm_pdsm.h
>> ---
>>  ndctl/lib/papr.c | 90
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 87 insertions(+), 3 deletions(-)
>> 
>> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c index
>> 1b7870beb631..cb7ff9e0d5bd 100644
>> --- a/ndctl/lib/papr.c
>> +++ b/ndctl/lib/papr.c
>> @@ -42,7 +42,9 @@
>> 
>>  /* Per dimm data. Holds per-dimm data parsed from the cmd_pkgs */  struct
>> dimm_priv {
>> -	/* Empty for now */
>> +
>> +	/* Cache the dimm health status */
>> +	struct nd_papr_pdsm_health health;
>
[.]
> I don't understand this. The kernel is caching this, why does libndctl
> need to cache it?

Was caching it here as the returned nvdimm health payload from kernel
might be of different version than what was requested and reconstituting
'struct nd_papr_pdsm_health' from 'struct ndctl_cmd' in each dimm-ops
callback would have be costly. 

However with payload versioning scheme not used any more this caching
wont be needed anymore.

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