[ndctl PATCH v4 0/5] ndctl: show broken dimm info with translate SPA feature.

[ndctl PATCH v4 0/5] ndctl: show broken dimm info with translate SPA feature.

[Yasunori Goto]

Hi,

I wrote v4 patch set to show broken NVDIMM for ndctl list command.
Please check it.

---
Change log since v3 [1]:
  - Return dimm info when dimm is not interleaved.
    Translate SPA is not necessary for its case.
  - Since ndctl_region is needed to get # of ways of interleave,
    ndctl_region_get_by_physical_address() is created,
    and sanity check (is_valid_spa) use it.
  - rename libndctl-nfit.c to nfit.c
  - move libndctl-nfit.h from ndctl/lib to ndctl/ .
  - not to make libndctl-nfit.la.
  - rename to ndctl_bus_is_nfit_cmd_supported()
  - add annotations.

Change log since v2 [2]:
 - Make libndctl-nfit.h and libndctl-nfit.c and define new interfaces
   which use translate spa on them.
 - Add sanity checks for new interfaces.
 - Fix some names
      o trans_spa -> translate_spa
      o sub_cmd -> passthru_cmd
   

Change log since v1 [3]:
 - Use ND_CMD_CALL to call translate SPA feature.
 - Separate patch set of ndctl from kernel patch set.
 - Add a interface to check what feature can call via ND_CMD_CALL by reading
   /device/nfit/dsm_mask 
 - Get only one nvdimm handle and DPA via ioctl() for the time being.
 - Bug fix which i found
     fix calculation of SPA from bad block at dev_badblocks_to_json().


[1] https://www.mail-archive.com/linux-nvdimm@lists.01.org/msg05857.html
[2] https://www.mail-archive.com/linux-nvdimm@lists.01.org/msg05577.html
[3] https://www.mail-archive.com/linux-nvdimm@lists.01.org/msg05287.html


----
This patch set is to show information of broken NVDIMM on ndctl.

When a region has a broken block, user needs to replace
the NVDIMM which includes the block.
However there is no information to find which DIMM module have the block.
Not only ndctl does not have such information, nvdimm driver can not
find it if the region is interleaved.


Fortunately, ACPI 6.2 has new specification of _DSM.
It is "translate spa" which can get NVDIMM handle
and DPA(Dimm Physical Address) from SPA(system Physical Addreess).
It helps for ndctl command to find broken NVDIMM when the region is
interleaved.

This patch set includes followings.
 - introduce libndctl-nfit.h
 - some preparations to call Translate SPA via ND_CMD_CALL.
 - make a function to get dimm by System physical address, and
   other helper functions.
 - ndctl list command show bad DIMM.


Thanks,
---
Yasunori Goto




_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 1/5] ndctl: Introduce libndctl-nfit.h

[Yasunori Goto]

This patch introduces libndctl-nfit.h.

Since these command can be executed via ND_CMD_CALL,
libndctl.h which is shared between ndctl command and kernel does not
have to include these defintions.

So, libndctl-nfit.h, which is defined for only ndctl, is created instead,
and move definitions from ndctl.h to it.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
---
 ndctl/libndctl-nfit.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
 ndctl/ndctl.h         | 37 -------------------------
 2 files changed, 76 insertions(+), 37 deletions(-)

diff --git a/ndctl/libndctl-nfit.h b/ndctl/libndctl-nfit.h
new file mode 100644
index 0000000..1258d2e
--- /dev/null
+++ b/ndctl/libndctl-nfit.h
@@ -0,0 +1,76 @@
+/*
+ * 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.
+ */
+
+#ifndef __LIBNDCTL_NFIT_H__
+#define __LIBNDCTL_NFIT_H__
+
+/*
+ * libndctl-nfit.h : definitions for NFIT related commands/functions.
+ */
+
+/* nfit command numbers which are called via ND_CMD_CALL */
+enum {
+	NFIT_CMD_TRANSLATE_SPA = 5,
+	NFIT_CMD_ARS_INJECT_SET = 7,
+	NFIT_CMD_ARS_INJECT_CLEAR = 8,
+	NFIT_CMD_ARS_INJECT_GET = 9,
+};
+
+/* error number of Translate SPA by firmware  */
+#define ND_TRANSLATE_SPA_STATUS_INVALID_SPA  2
+
+/*
+ * The following structures are command packages which are
+ * defined by ACPI 6.2 (or later).
+ */
+
+/* For Translate SPA */
+struct nd_cmd_translate_spa {
+	__u64 spa;
+	__u32 status;
+	__u8  flags;
+	__u8  _reserved[3];
+	__u64 translate_length;
+	__u32 num_nvdimms;
+	struct nd_nvdimm_device {
+		__u32 nfit_device_handle;
+		__u32 _reserved;
+		__u64 dpa;
+	} devices[0];
+
+}; /* naturally packed */
+
+/* For ARS Error Inject */
+struct nd_cmd_ars_err_inj {
+	__u64 err_inj_spa_range_base;
+	__u64 err_inj_spa_range_length;
+	__u8  err_inj_options;
+	__u32 status;
+}; /* naturally packed */
+
+/* For ARS Error Inject Clear */
+struct nd_cmd_ars_err_inj_clr {
+	__u64 err_inj_clr_spa_range_base;
+	__u64 err_inj_clr_spa_range_length;
+	__u32 status;
+}; /* naturally packed */
+
+/* For ARS Error Inject Status Query */
+struct nd_cmd_ars_err_inj_stat {
+	__u32 status;
+	__u32 inj_err_rec_count;
+	struct nd_error_stat_query_record {
+		__u64 err_inj_stat_spa_range_base;
+		__u64 err_inj_stat_spa_range_length;
+	} record[0];
+}; /* naturally packed */
+
+#endif /* __LIBNDCTL_NFIT_H__ */
diff --git a/ndctl/ndctl.h b/ndctl/ndctl.h
index d70b97d..2dd461b 100644
--- a/ndctl/ndctl.h
+++ b/ndctl/ndctl.h
@@ -145,43 +145,6 @@ struct nd_cmd_clear_error {
 	__u64 cleared;
 } __attribute__((packed));
 
-struct nd_cmd_trans_spa {
-	__u64 spa;
-	__u32 status;
-	__u8  flags;
-	__u8  _reserved[3];
-	__u64 trans_length;
-	__u32 num_nvdimms;
-	struct nd_nvdimm_device {
-		__u32 nfit_device_handle;
-		__u32 _reserved;
-		__u64 dpa;
-	} __attribute__((packed)) devices[0];
-
-} __attribute__((packed));
-
-struct nd_cmd_ars_err_inj {
-	__u64 err_inj_spa_range_base;
-	__u64 err_inj_spa_range_length;
-	__u8  err_inj_options;
-	__u32 status;
-} __attribute__((packed));
-
-struct nd_cmd_ars_err_inj_clr {
-	__u64 err_inj_clr_spa_range_base;
-	__u64 err_inj_clr_spa_range_length;
-	__u32 status;
-} __attribute__((packed));
-
-struct nd_cmd_ars_err_inj_stat {
-	__u32 status;
-	__u32 inj_err_rec_count;
-	struct nd_error_stat_query_record {
-		__u64 err_inj_stat_spa_range_base;
-		__u64 err_inj_stat_spa_range_length;
-	} __attribute__((packed)) record[0];
-} __attribute__((packed));
-
 enum {
 	ND_CMD_IMPLEMENTED = 0,
 



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 2/5] ndctl: make interface to read device/nfit/dsm_mask

[Yasunori Goto]

To check what feature can be called via ND_CMD_CALL, ndctl need to read
device/nfit/dsm_mask.
Since direct pointer access is not permitted,
ndctl_bus_get_nfit_dsm_mask() is created to access nfit_dsm_mask.
It is used later patch.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
---
 ndctl/lib/libndctl.c   | 16 ++++++++++++++++
 ndctl/lib/libndctl.sym |  1 +
 ndctl/libndctl.h.in    |  1 +
 3 files changed, 18 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index f52ecfe..ea37ca4 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -104,6 +104,7 @@ struct ndctl_bus {
 	size_t buf_len;
 	char *wait_probe_path;
 	unsigned long dsm_mask;
+	unsigned long nfit_dsm_mask;
 };
 
 /**
@@ -793,6 +794,12 @@ static void *add_bus(void *parent, int id, const char *ctl_base)
 		bus->revision = strtoul(buf, NULL, 0);
 	}
 
+	sprintf(path, "%s/device/nfit/dsm_mask", ctl_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		bus->nfit_dsm_mask = 0;
+	else
+		bus->nfit_dsm_mask = strtoul(buf, NULL, 0);
+
 	sprintf(path, "%s/device/provider", ctl_base);
 	if (sysfs_read_attr(ctx, path, buf) < 0)
 		goto err_read;
@@ -1048,6 +1055,15 @@ NDCTL_EXPORT int ndctl_bus_is_cmd_supported(struct ndctl_bus *bus,
 	return !!(bus->dsm_mask & (1ULL << cmd));
 }
 
+/*
+ * This function is exported for only nfit.c.
+ * Please use ndctl_bus_is_nfit_cmd_supported() in nfit.c instead.
+ */
+NDCTL_EXPORT unsigned long ndctl_bus_get_nfit_dsm_mask(struct ndctl_bus *bus)
+{
+	return bus->nfit_dsm_mask;
+}
+
 NDCTL_EXPORT unsigned int ndctl_bus_get_revision(struct ndctl_bus *bus)
 {
 	return bus->revision;
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 4c1773d..3f9bf09 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -30,6 +30,7 @@ global:
 	ndctl_bus_get_by_provider;
 	ndctl_bus_get_cmd_name;
 	ndctl_bus_is_cmd_supported;
+	ndctl_bus_get_nfit_dsm_mask;
 	ndctl_bus_has_nfit;
 	ndctl_bus_get_revision;
 	ndctl_bus_get_id;
diff --git a/ndctl/libndctl.h.in b/ndctl/libndctl.h.in
index 2bbda04..bfee693 100644
--- a/ndctl/libndctl.h.in
+++ b/ndctl/libndctl.h.in
@@ -109,6 +109,7 @@ struct ndctl_bus *ndctl_bus_get_by_provider(struct ndctl_ctx *ctx,
 		const char *provider);
 const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd);
 int ndctl_bus_is_cmd_supported(struct ndctl_bus *bus, int cmd);
+unsigned long ndctl_bus_get_nfit_dsm_mask(struct ndctl_bus *bus);
 unsigned int ndctl_bus_get_revision(struct ndctl_bus *bus);
 unsigned int ndctl_bus_get_id(struct ndctl_bus *bus);
 const char *ndctl_bus_get_provider(struct ndctl_bus *bus);



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 3/5] ndctl: allow ND_CMD_CALL for bus

[Yasunori Goto]

[ndctl PATCH 3/5] ndctl: allow ND_CMD_CALL for bus

Currently ndctl supports ND_CMD_CALL only for DIMM,
but Translate SPA is the feature of bus.
So ND_CMD_CALL must be allowed bus's ioctl().

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
---
 ndctl/lib/libndctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ea37ca4..740b6f1 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2228,6 +2228,7 @@ static int to_ioctl_cmd(int cmd, int dimm)
 #ifdef HAVE_NDCTL_CLEAR_ERROR
 		case ND_CMD_CLEAR_ERROR:     return ND_IOCTL_CLEAR_ERROR;
 #endif
+		case ND_CMD_CALL:            return ND_IOCTL_CALL;
 		default:
 						       return 0;
 		};



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 4/5] ndctl: Make new interfaces to get information by Physical Address

[Yasunori Goto]

This patch makes new interfaces :
  - Confirm NFIT command is supported or not. (nfit.c)
  - Call translate SPA featture of ACPI 6.2. (nfit.c)
  - Find region by System Physical Address (libndctl.c)
  - Find DIMM by System Physical Address. (libndctl.c)

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
---
 ndctl/lib/Makefile.am  |   2 +
 ndctl/lib/libndctl.c   |  69 +++++++++++++++++++++++
 ndctl/lib/libndctl.sym |   2 +
 ndctl/lib/nfit.c       | 147 +++++++++++++++++++++++++++++++++++++++++++++++++
 ndctl/libndctl-nfit.h  |   4 ++
 ndctl/libndctl.h.in    |   4 ++
 6 files changed, 228 insertions(+)

diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index d8df87d..9a7734d 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -36,6 +36,8 @@ libndctl_la_SOURCES += hpe1.c
 libndctl_la_SOURCES += msft.c
 endif
 
+libndctl_la_SOURCES += nfit.c
+
 EXTRA_DIST += libndctl.sym
 
 libndctl_la_LDFLAGS = $(AM_LDFLAGS) \
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 740b6f1..6cc8e1c 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -38,6 +38,7 @@
 #include <ndctl/libndctl.h>
 #include <ndctl/namespace.h>
 #include <daxctl/libdaxctl.h>
+#include <ndctl/libndctl-nfit.h>
 #include "private.h"
 
 static uuid_t null_uuid;
@@ -1570,6 +1571,74 @@ static struct ndctl_dimm *ndctl_dimm_get_by_id(struct ndctl_bus *bus, unsigned i
 	return NULL;
 }
 
+/**
+ * ndctl_region_get_by_physical_address - get region by physical address
+ * @bus: ndctl_bus instance
+ * @address: (System) Physical Address
+ *
+ * If @bus and @address is valid, returns a region address, which
+ * physical address belongs to.
+ */
+NDCTL_EXPORT struct ndctl_region *ndctl_region_get_by_physical_address(struct ndctl_bus *bus,
+		unsigned long long address)
+{
+	unsigned long long region_start, region_end;
+	struct ndctl_region *region;
+
+	ndctl_region_foreach(bus, region) {
+		region_start = ndctl_region_get_resource(region);
+		region_end = region_start + ndctl_region_get_size(region);
+		if (region_start <= address && address < region_end)
+			return region;
+	}
+
+	return NULL;
+}
+
+/**
+ * ndctl_dimm_get_by_physical_address - get ndctl_dimm pointer by physical address
+ * @bus: ndctl_bus instance
+ * @address: (System) Physical Address
+ *
+ * Returns address of ndctl_dimm on success.
+ */
+NDCTL_EXPORT struct ndctl_dimm *ndctl_dimm_get_by_physical_address(struct ndctl_bus *bus,
+	        unsigned long long address)
+{
+	int rc;
+        unsigned int handle;
+	unsigned long long dpa;
+	struct ndctl_region *region;
+
+	if (!bus)
+		return NULL;
+
+	region = ndctl_region_get_by_physical_address(bus, address);
+	if (!region)
+		return NULL;
+
+	if (ndctl_region_get_interleave_ways(region) == 1) {
+		/*
+		 * No need to ask firmware. The first dimm is iff.
+		 */
+		struct ndctl_mapping *mapping = ndctl_mapping_get_first(region);
+		if (!mapping)
+			return NULL;
+		return ndctl_mapping_get_dimm(mapping);
+	}
+
+	/*
+	 * Since the region is interleaved, we need to ask firmware about it.
+	 * If it supports Translate SPA, the dimm is returned.
+	 */
+        rc = ndctl_bus_cmd_translate_spa(bus, address, &handle, &dpa);
+        if (rc)
+                return NULL;
+
+        return ndctl_dimm_get_by_handle(bus, handle);
+}
+
+
 static int region_set_type(struct ndctl_region *region, char *path)
 {
 	struct ndctl_ctx *ctx = ndctl_region_get_ctx(region);
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 3f9bf09..888108d 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -77,6 +77,7 @@ global:
 	ndctl_dimm_get_bus;
 	ndctl_dimm_get_ctx;
 	ndctl_dimm_get_by_handle;
+	ndctl_dimm_get_by_physical_address;
 	ndctl_dimm_is_active;
 	ndctl_dimm_is_enabled;
 	ndctl_dimm_disable;
@@ -136,6 +137,7 @@ global:
 	ndctl_region_get_ctx;
 	ndctl_region_get_first_dimm;
 	ndctl_region_get_next_dimm;
+	ndctl_region_get_by_physical_address;
 	ndctl_region_is_enabled;
 	ndctl_region_enable;
 	ndctl_region_disable_invalidate;
diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
new file mode 100644
index 0000000..79d966d
--- /dev/null
+++ b/ndctl/lib/nfit.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright (c) 2017, FUJITSU LIMITED. All rights reserved.
+ *
+ * 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 <stdlib.h>
+#include <ndctl/libndctl.h>
+#include "private.h"
+#include <ndctl/libndctl-nfit.h>
+
+struct ndctl_bus;
+
+/**
+ * ndctl_bus_is_nfit_cmd_supported - ask nfit command is supported on @bus.
+ * @bus: ndctl_bus instance
+ * @cmd: nfit command number (defined as NFIT_CMD_XXX in libndctl-nfit.h)
+ *
+ * Return 1: command is supported. Return 0: command is not supported.
+ *
+ */
+NDCTL_EXPORT int ndctl_bus_is_nfit_cmd_supported(struct ndctl_bus *bus,
+                int cmd)
+{
+        return !!(ndctl_bus_get_nfit_dsm_mask(bus) & (1ULL << cmd));
+}
+
+static int bus_has_translate_spa(struct ndctl_bus *bus)
+{
+	if (!ndctl_bus_has_nfit(bus))
+		return 0;
+
+	return ndctl_bus_is_nfit_cmd_supported(bus, NFIT_CMD_TRANSLATE_SPA);
+}
+
+static struct ndctl_cmd *ndctl_bus_cmd_new_translate_spa(struct ndctl_bus *bus)
+{
+	struct ndctl_cmd *cmd;
+	struct nd_cmd_pkg *pkg;
+	struct nd_cmd_translate_spa *translate_spa;
+	size_t size, spa_length;
+
+	spa_length = sizeof(struct nd_cmd_translate_spa)
+		+ sizeof(struct nd_nvdimm_device);
+	size = sizeof(*cmd) + sizeof(*pkg) + spa_length;
+	cmd = calloc(1, size);
+	if (!cmd)
+		return NULL;
+
+	cmd->bus = bus;
+	ndctl_cmd_ref(cmd);
+	cmd->type = ND_CMD_CALL;
+	cmd->size = size;
+	cmd->status = 1;
+	pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
+	pkg->nd_command = NFIT_CMD_TRANSLATE_SPA;
+	pkg->nd_size_in = sizeof(unsigned long long);
+	pkg->nd_size_out = spa_length;
+	pkg->nd_fw_size = spa_length;
+	translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
+	cmd->firmware_status = &translate_spa->status;
+	translate_spa->translate_length = spa_length;
+
+	return cmd;
+}
+
+static int ndctl_bus_cmd_get_translate_spa(struct ndctl_cmd *cmd,
+					unsigned int *handle, unsigned long long *dpa)
+{
+	struct nd_cmd_pkg *pkg;
+	struct nd_cmd_translate_spa *translate_spa;
+
+	pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
+	translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
+
+	if (translate_spa->status == ND_TRANSLATE_SPA_STATUS_INVALID_SPA)
+		return -EINVAL;
+
+	/*
+	 * XXX: Currently NVDIMM mirroring is not supported.
+	 * Even if ACPI returned plural dimms due to mirroring,
+	 * this function returns just the first dimm.
+	 */
+
+	*handle = translate_spa->devices[0].nfit_device_handle;
+	*dpa = translate_spa->devices[0].dpa;
+
+	return 0;
+}
+
+static int is_valid_spa(struct ndctl_bus *bus, unsigned long long spa)
+{
+	return !!ndctl_region_get_by_physical_address(bus, spa);
+}
+
+/**
+ * ndctl_bus_cmd_translate_spa - call translate spa.
+ * @bus: bus which belongs to.
+ * @address: address (System Physical Address)
+ * @handle: pointer to return dimm handle
+ * @dpa: pointer to return Dimm Physical address
+ *
+ * If success, returns zero, store dimm's @handle, and @dpa.
+ */
+NDCTL_EXPORT int ndctl_bus_cmd_translate_spa(struct ndctl_bus *bus,
+	unsigned long long address, unsigned int *handle, unsigned long long *dpa)
+{
+
+	struct ndctl_cmd *cmd;
+	struct nd_cmd_pkg *pkg;
+	struct nd_cmd_translate_spa *translate_spa;
+	int rc;
+
+	if (!bus || !handle || !dpa)
+		return -EINVAL;
+
+	if (!bus_has_translate_spa(bus))
+		return -ENOTTY;
+
+	if (!is_valid_spa(bus, address))
+		return -EINVAL;
+
+	cmd = ndctl_bus_cmd_new_translate_spa(bus);
+	if (!cmd)
+		return -ENOMEM;
+
+	pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
+	translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
+	translate_spa->spa = address;
+
+	rc = ndctl_cmd_submit(cmd);
+	if (rc) {
+		ndctl_cmd_unref(cmd);
+		return rc;
+	}
+
+	rc = ndctl_bus_cmd_get_translate_spa(cmd, handle, dpa);
+	ndctl_cmd_unref(cmd);
+
+	return rc;
+}
diff --git a/ndctl/libndctl-nfit.h b/ndctl/libndctl-nfit.h
index 1258d2e..fbeebb0 100644
--- a/ndctl/libndctl-nfit.h
+++ b/ndctl/libndctl-nfit.h
@@ -73,4 +73,8 @@ struct nd_cmd_ars_err_inj_stat {
 	} record[0];
 }; /* naturally packed */
 
+int ndctl_bus_is_nfit_cmd_supported(struct ndctl_bus *bus, int cmd);
+int ndctl_bus_cmd_translate_spa(struct ndctl_bus *bus,
+	unsigned long long addr, unsigned int *handle, unsigned long long *dpa);
+
 #endif /* __LIBNDCTL_NFIT_H__ */
diff --git a/ndctl/libndctl.h.in b/ndctl/libndctl.h.in
index bfee693..bbfb14c 100644
--- a/ndctl/libndctl.h.in
+++ b/ndctl/libndctl.h.in
@@ -162,6 +162,8 @@ struct ndctl_smart_ops *ndctl_dimm_get_smart_ops(struct ndctl_dimm *dimm);
 struct ndctl_ctx *ndctl_dimm_get_ctx(struct ndctl_dimm *dimm);
 struct ndctl_dimm *ndctl_dimm_get_by_handle(struct ndctl_bus *bus,
 		unsigned int handle);
+struct ndctl_dimm *ndctl_dimm_get_by_physical_address(struct ndctl_bus *bus,
+		unsigned long long address);
 int ndctl_dimm_is_active(struct ndctl_dimm *dimm);
 int ndctl_dimm_is_enabled(struct ndctl_dimm *dimm);
 int ndctl_dimm_disable(struct ndctl_dimm *dimm);
@@ -422,6 +424,8 @@ struct ndctl_ctx *ndctl_region_get_ctx(struct ndctl_region *region);
 struct ndctl_dimm *ndctl_region_get_first_dimm(struct ndctl_region *region);
 struct ndctl_dimm *ndctl_region_get_next_dimm(struct ndctl_region *region,
 		struct ndctl_dimm *dimm);
+struct ndctl_region *ndctl_region_get_by_physical_address(struct ndctl_bus *bus,
+		unsigned long long address);
 #define ndctl_dimm_foreach_in_region(region, dimm) \
         for (dimm = ndctl_region_get_first_dimm(region); \
              dimm != NULL; \



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 5/5] ndctl: show bad dimm's name by ndctl list command.

[Yasunori Goto]

Show dimm's name which has badblock by ndctl list command.
This patch uses translate SPA interface to get bad dimm info.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
---
 util/json.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/util/json.c b/util/json.c
index 98165b7..a9b6259 100644
--- a/util/json.c
+++ b/util/json.c
@@ -366,6 +366,15 @@ struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
 	return NULL;
 }
 
+static struct ndctl_dimm *badblock_to_dimm(struct ndctl_region *region,
+		unsigned long long spa)
+{
+	struct ndctl_bus *bus;
+
+	bus = ndctl_region_get_bus(region);
+	return ndctl_dimm_get_by_physical_address(bus, spa);
+}
+
 struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 		unsigned int *bb_count, unsigned long flags)
 {
@@ -381,6 +390,18 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 
 	ndctl_region_badblock_foreach(region, bb) {
 		if (flags & UTIL_JSON_MEDIA_ERRORS) {
+			struct ndctl_dimm *dimm = NULL;
+			unsigned long long spa;
+
+			/* get start address of region */
+			spa = ndctl_region_get_resource(region);
+			if (spa == ULONG_MAX)
+				goto err_array;
+
+			/* get address of bad block */
+			spa += bb->offset << 9;
+			dimm = badblock_to_dimm(region, spa);
+
 			jbb = json_object_new_object();
 			if (!jbb)
 				goto err_array;
@@ -395,6 +416,12 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 				goto err;
 			json_object_object_add(jbb, "length", jobj);
 
+			if (dimm) {
+				jobj = json_object_new_string(ndctl_dimm_get_devname(dimm));
+				if (!jobj)
+					goto err;
+				json_object_object_add(jbb, "dimm", jobj);
+			}
 			json_object_array_add(jbbs, jbb);
 		}
 
@@ -436,6 +463,7 @@ static struct json_object *dev_badblocks_to_json(struct ndctl_region *region,
 
 	ndctl_region_badblock_foreach(region, bb) {
 		unsigned long long bb_begin, bb_end, begin, end;
+		struct ndctl_dimm *dimm = NULL;
 
 		bb_begin = region_begin + (bb->offset << 9);
 		bb_end = bb_begin + (bb->len << 9) - 1;
@@ -456,6 +484,8 @@ static struct json_object *dev_badblocks_to_json(struct ndctl_region *region,
 		offset = (begin - dev_begin) >> 9;
 		len = (end - begin + 1) >> 9;
 
+		dimm = badblock_to_dimm(region, begin);
+
 		if (flags & UTIL_JSON_MEDIA_ERRORS) {
 			/* add to json */
 			jbb = json_object_new_object();
@@ -472,6 +502,13 @@ static struct json_object *dev_badblocks_to_json(struct ndctl_region *region,
 				goto err;
 			json_object_object_add(jbb, "length", jobj);
 
+			if (dimm) {
+				jobj = json_object_new_string(ndctl_dimm_get_devname(dimm));
+				if (!jobj)
+					goto err;
+				json_object_object_add(jbb, "dimm", jobj);
+			}
+
 			json_object_array_add(jbbs, jbb);
 		}
 		bbs += len;



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 1/5] ndctl: Introduce libndctl-nfit.h

[Dan Williams]

On Fri, Sep 1, 2017 at 6:16 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
>
> This patch introduces libndctl-nfit.h.
>
> Since these command can be executed via ND_CMD_CALL,
> libndctl.h which is shared between ndctl command and kernel does not
> have to include these defintions.
>
> So, libndctl-nfit.h, which is defined for only ndctl, is created instead,
> and move definitions from ndctl.h to it.
>
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> ---
>  ndctl/libndctl-nfit.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  ndctl/ndctl.h         | 37 -------------------------
>  2 files changed, 76 insertions(+), 37 deletions(-)
>
> diff --git a/ndctl/libndctl-nfit.h b/ndctl/libndctl-nfit.h
> new file mode 100644
> index 0000000..1258d2e
> --- /dev/null
> +++ b/ndctl/libndctl-nfit.h
> @@ -0,0 +1,76 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef __LIBNDCTL_NFIT_H__
> +#define __LIBNDCTL_NFIT_H__
> +
> +/*
> + * libndctl-nfit.h : definitions for NFIT related commands/functions.
> + */
> +
> +/* nfit command numbers which are called via ND_CMD_CALL */
> +enum {
> +       NFIT_CMD_TRANSLATE_SPA = 5,
> +       NFIT_CMD_ARS_INJECT_SET = 7,
> +       NFIT_CMD_ARS_INJECT_CLEAR = 8,
> +       NFIT_CMD_ARS_INJECT_GET = 9,
> +};
> +
> +/* error number of Translate SPA by firmware  */
> +#define ND_TRANSLATE_SPA_STATUS_INVALID_SPA  2
> +
> +/*
> + * The following structures are command packages which are
> + * defined by ACPI 6.2 (or later).
> + */
> +
> +/* For Translate SPA */
> +struct nd_cmd_translate_spa {
> +       __u64 spa;
> +       __u32 status;
> +       __u8  flags;
> +       __u8  _reserved[3];
> +       __u64 translate_length;
> +       __u32 num_nvdimms;
> +       struct nd_nvdimm_device {
> +               __u32 nfit_device_handle;
> +               __u32 _reserved;
> +               __u64 dpa;
> +       } devices[0];
> +
> +}; /* naturally packed */
> +

Sorry, I wasn't clear. Only "nd_cmd_ars_err_inj_stat" is naturally
packed the others are not. I can fix this up when I apply it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 2/5] ndctl: make interface to read device/nfit/dsm_mask

[Dan Williams]

On Fri, Sep 1, 2017 at 6:17 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
>
> To check what feature can be called via ND_CMD_CALL, ndctl need to read
> device/nfit/dsm_mask.
> Since direct pointer access is not permitted,
> ndctl_bus_get_nfit_dsm_mask() is created to access nfit_dsm_mask.
> It is used later patch.
>
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> ---
>  ndctl/lib/libndctl.c   | 16 ++++++++++++++++
>  ndctl/lib/libndctl.sym |  1 +
>  ndctl/libndctl.h.in    |  1 +
>  3 files changed, 18 insertions(+)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index f52ecfe..ea37ca4 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -104,6 +104,7 @@ struct ndctl_bus {
>         size_t buf_len;
>         char *wait_probe_path;
>         unsigned long dsm_mask;
> +       unsigned long nfit_dsm_mask;
>  };
>
>  /**
> @@ -793,6 +794,12 @@ static void *add_bus(void *parent, int id, const char *ctl_base)
>                 bus->revision = strtoul(buf, NULL, 0);
>         }
>
> +       sprintf(path, "%s/device/nfit/dsm_mask", ctl_base);
> +       if (sysfs_read_attr(ctx, path, buf) < 0)
> +               bus->nfit_dsm_mask = 0;
> +       else
> +               bus->nfit_dsm_mask = strtoul(buf, NULL, 0);
> +
>         sprintf(path, "%s/device/provider", ctl_base);
>         if (sysfs_read_attr(ctx, path, buf) < 0)
>                 goto err_read;
> @@ -1048,6 +1055,15 @@ NDCTL_EXPORT int ndctl_bus_is_cmd_supported(struct ndctl_bus *bus,
>         return !!(bus->dsm_mask & (1ULL << cmd));
>  }
>
> +/*
> + * This function is exported for only nfit.c.
> + * Please use ndctl_bus_is_nfit_cmd_supported() in nfit.c instead.
> + */
> +NDCTL_EXPORT unsigned long ndctl_bus_get_nfit_dsm_mask(struct ndctl_bus *bus)
> +{
> +       return bus->nfit_dsm_mask;
> +}

I don't want to export the mask directly from library since we'll also
have ndctl_bus_is_nfit_cmd_supported(). Let's just move struct
ndctl_bus into private.h.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 4/5] ndctl: Make new interfaces to get information by Physical Address

[Dan Williams]

On Fri, Sep 1, 2017 at 6:21 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
>
> This patch makes new interfaces :
>   - Confirm NFIT command is supported or not. (nfit.c)
>   - Call translate SPA featture of ACPI 6.2. (nfit.c)
>   - Find region by System Physical Address (libndctl.c)
>   - Find DIMM by System Physical Address. (libndctl.c)
>
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> ---
>  ndctl/lib/Makefile.am  |   2 +
>  ndctl/lib/libndctl.c   |  69 +++++++++++++++++++++++
>  ndctl/lib/libndctl.sym |   2 +
>  ndctl/lib/nfit.c       | 147 +++++++++++++++++++++++++++++++++++++++++++++++++
>  ndctl/libndctl-nfit.h  |   4 ++
>  ndctl/libndctl.h.in    |   4 ++
>  6 files changed, 228 insertions(+)
>
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> index d8df87d..9a7734d 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -36,6 +36,8 @@ libndctl_la_SOURCES += hpe1.c
>  libndctl_la_SOURCES += msft.c
>  endif
>
> +libndctl_la_SOURCES += nfit.c
> +
>  EXTRA_DIST += libndctl.sym
>
>  libndctl_la_LDFLAGS = $(AM_LDFLAGS) \
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 740b6f1..6cc8e1c 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -38,6 +38,7 @@
>  #include <ndctl/libndctl.h>
>  #include <ndctl/namespace.h>
>  #include <daxctl/libdaxctl.h>
> +#include <ndctl/libndctl-nfit.h>
>  #include "private.h"
>
>  static uuid_t null_uuid;
> @@ -1570,6 +1571,74 @@ static struct ndctl_dimm *ndctl_dimm_get_by_id(struct ndctl_bus *bus, unsigned i
>         return NULL;
>  }
>
> +/**
> + * ndctl_region_get_by_physical_address - get region by physical address
> + * @bus: ndctl_bus instance
> + * @address: (System) Physical Address
> + *
> + * If @bus and @address is valid, returns a region address, which
> + * physical address belongs to.
> + */
> +NDCTL_EXPORT struct ndctl_region *ndctl_region_get_by_physical_address(struct ndctl_bus *bus,
> +               unsigned long long address)

This should be named ndctl_bus_get_region_by_physical_address() since
it is operating on a bus.

> +{
> +       unsigned long long region_start, region_end;
> +       struct ndctl_region *region;
> +
> +       ndctl_region_foreach(bus, region) {
> +               region_start = ndctl_region_get_resource(region);
> +               region_end = region_start + ndctl_region_get_size(region);
> +               if (region_start <= address && address < region_end)
> +                       return region;
> +       }
> +
> +       return NULL;
> +}
> +
> +/**
> + * ndctl_dimm_get_by_physical_address - get ndctl_dimm pointer by physical address
> + * @bus: ndctl_bus instance
> + * @address: (System) Physical Address
> + *
> + * Returns address of ndctl_dimm on success.
> + */
> +NDCTL_EXPORT struct ndctl_dimm *ndctl_dimm_get_by_physical_address(struct ndctl_bus *bus,
> +               unsigned long long address)
> +{

...and for the same reason this should be
ndctl_bus_get_dimm_by_physical_address()

> +       int rc;
> +        unsigned int handle;
> +       unsigned long long dpa;
> +       struct ndctl_region *region;
> +
> +       if (!bus)
> +               return NULL;
> +
> +       region = ndctl_region_get_by_physical_address(bus, address);
> +       if (!region)
> +               return NULL;
> +
> +       if (ndctl_region_get_interleave_ways(region) == 1) {
> +               /*
> +                * No need to ask firmware. The first dimm is iff.
> +                */
> +               struct ndctl_mapping *mapping = ndctl_mapping_get_first(region);
> +               if (!mapping)
> +                       return NULL;
> +               return ndctl_mapping_get_dimm(mapping);
> +       }
> +
> +       /*
> +        * Since the region is interleaved, we need to ask firmware about it.
> +        * If it supports Translate SPA, the dimm is returned.
> +        */
> +        rc = ndctl_bus_cmd_translate_spa(bus, address, &handle, &dpa);
> +        if (rc)
> +                return NULL;

Since this does not return an ndctl_cmd and a "handle" is an NFIT
specific attribute, let's change this to:

        if (ndctl_bus_has_nfit(bus))
                dimm = ndctl_bus_nfit_translate_spa(bus, address);

...in the future if a new bus type comes along we can add:

         else if (ndctl_bus_has_<type>(bus))
                dimm = ndctl_bus_<type>_translate_spa(bus, address);



> +
> +        return ndctl_dimm_get_by_handle(bus, handle);
> +}
> +
> +
>  static int region_set_type(struct ndctl_region *region, char *path)
>  {
>         struct ndctl_ctx *ctx = ndctl_region_get_ctx(region);
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index 3f9bf09..888108d 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -77,6 +77,7 @@ global:
>         ndctl_dimm_get_bus;
>         ndctl_dimm_get_ctx;
>         ndctl_dimm_get_by_handle;
> +       ndctl_dimm_get_by_physical_address;
>         ndctl_dimm_is_active;
>         ndctl_dimm_is_enabled;
>         ndctl_dimm_disable;
> @@ -136,6 +137,7 @@ global:
>         ndctl_region_get_ctx;
>         ndctl_region_get_first_dimm;
>         ndctl_region_get_next_dimm;
> +       ndctl_region_get_by_physical_address;
>         ndctl_region_is_enabled;
>         ndctl_region_enable;
>         ndctl_region_disable_invalidate;
> diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
> new file mode 100644
> index 0000000..79d966d
> --- /dev/null
> +++ b/ndctl/lib/nfit.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (c) 2017, FUJITSU LIMITED. All rights reserved.
> + *
> + * 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 <stdlib.h>
> +#include <ndctl/libndctl.h>
> +#include "private.h"
> +#include <ndctl/libndctl-nfit.h>
> +
> +struct ndctl_bus;
> +
> +/**
> + * ndctl_bus_is_nfit_cmd_supported - ask nfit command is supported on @bus.
> + * @bus: ndctl_bus instance
> + * @cmd: nfit command number (defined as NFIT_CMD_XXX in libndctl-nfit.h)
> + *
> + * Return 1: command is supported. Return 0: command is not supported.
> + *
> + */
> +NDCTL_EXPORT int ndctl_bus_is_nfit_cmd_supported(struct ndctl_bus *bus,
> +                int cmd)
> +{
> +        return !!(ndctl_bus_get_nfit_dsm_mask(bus) & (1ULL << cmd));
> +}
> +
> +static int bus_has_translate_spa(struct ndctl_bus *bus)
> +{
> +       if (!ndctl_bus_has_nfit(bus))
> +               return 0;
> +
> +       return ndctl_bus_is_nfit_cmd_supported(bus, NFIT_CMD_TRANSLATE_SPA);
> +}
> +
> +static struct ndctl_cmd *ndctl_bus_cmd_new_translate_spa(struct ndctl_bus *bus)
> +{
> +       struct ndctl_cmd *cmd;
> +       struct nd_cmd_pkg *pkg;
> +       struct nd_cmd_translate_spa *translate_spa;
> +       size_t size, spa_length;
> +
> +       spa_length = sizeof(struct nd_cmd_translate_spa)
> +               + sizeof(struct nd_nvdimm_device);
> +       size = sizeof(*cmd) + sizeof(*pkg) + spa_length;
> +       cmd = calloc(1, size);
> +       if (!cmd)
> +               return NULL;
> +
> +       cmd->bus = bus;
> +       ndctl_cmd_ref(cmd);
> +       cmd->type = ND_CMD_CALL;
> +       cmd->size = size;
> +       cmd->status = 1;
> +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> +       pkg->nd_command = NFIT_CMD_TRANSLATE_SPA;
> +       pkg->nd_size_in = sizeof(unsigned long long);
> +       pkg->nd_size_out = spa_length;
> +       pkg->nd_fw_size = spa_length;
> +       translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
> +       cmd->firmware_status = &translate_spa->status;
> +       translate_spa->translate_length = spa_length;
> +
> +       return cmd;
> +}
> +
> +static int ndctl_bus_cmd_get_translate_spa(struct ndctl_cmd *cmd,
> +                                       unsigned int *handle, unsigned long long *dpa)
> +{
> +       struct nd_cmd_pkg *pkg;
> +       struct nd_cmd_translate_spa *translate_spa;
> +
> +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> +       translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
> +
> +       if (translate_spa->status == ND_TRANSLATE_SPA_STATUS_INVALID_SPA)
> +               return -EINVAL;
> +
> +       /*
> +        * XXX: Currently NVDIMM mirroring is not supported.
> +        * Even if ACPI returned plural dimms due to mirroring,
> +        * this function returns just the first dimm.
> +        */
> +
> +       *handle = translate_spa->devices[0].nfit_device_handle;
> +       *dpa = translate_spa->devices[0].dpa;
> +
> +       return 0;
> +}
> +
> +static int is_valid_spa(struct ndctl_bus *bus, unsigned long long spa)
> +{
> +       return !!ndctl_region_get_by_physical_address(bus, spa);
> +}
> +
> +/**
> + * ndctl_bus_cmd_translate_spa - call translate spa.
> + * @bus: bus which belongs to.
> + * @address: address (System Physical Address)
> + * @handle: pointer to return dimm handle
> + * @dpa: pointer to return Dimm Physical address
> + *
> + * If success, returns zero, store dimm's @handle, and @dpa.
> + */
> +NDCTL_EXPORT int ndctl_bus_cmd_translate_spa(struct ndctl_bus *bus,
> +       unsigned long long address, unsigned int *handle, unsigned long long *dpa)
> +{
> +
> +       struct ndctl_cmd *cmd;
> +       struct nd_cmd_pkg *pkg;
> +       struct nd_cmd_translate_spa *translate_spa;
> +       int rc;
> +
> +       if (!bus || !handle || !dpa)
> +               return -EINVAL;
> +
> +       if (!bus_has_translate_spa(bus))
> +               return -ENOTTY;
> +
> +       if (!is_valid_spa(bus, address))
> +               return -EINVAL;
> +
> +       cmd = ndctl_bus_cmd_new_translate_spa(bus);
> +       if (!cmd)
> +               return -ENOMEM;
> +
> +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> +       translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
> +       translate_spa->spa = address;
> +
> +       rc = ndctl_cmd_submit(cmd);
> +       if (rc) {
> +               ndctl_cmd_unref(cmd);
> +               return rc;
> +       }
> +
> +       rc = ndctl_bus_cmd_get_translate_spa(cmd, handle, dpa);
> +       ndctl_cmd_unref(cmd);
> +
> +       return rc;

Since the "handle" is not generic I don't think we can make this
publicly exported. Let's hide this detail behind
ndctl_bus_nfit_translate_spa() that is called by
ndctl_bus_get_dimm_by_physical_address().
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 1/5] ndctl: Introduce libndctl-nfit.h

[Dan Williams]

On Tue, Sep 5, 2017 at 11:13 AM, Dan Williams <dan.j.williams@intel.com>
wrote:

> On Fri, Sep 1, 2017 at 6:16 AM, Yasunori Goto <y-goto@jp.fujitsu.com>
> wrote:
> >
> > This patch introduces libndctl-nfit.h.
> >
> > Since these command can be executed via ND_CMD_CALL,
> > libndctl.h which is shared between ndctl command and kernel does not
> > have to include these defintions.
> >
> > So, libndctl-nfit.h, which is defined for only ndctl, is created instead,
> > and move definitions from ndctl.h to it.
> >
> > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> > ---
> >  ndctl/libndctl-nfit.h | 76 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++
> >  ndctl/ndctl.h         | 37 -------------------------
> >  2 files changed, 76 insertions(+), 37 deletions(-)
> >
> > diff --git a/ndctl/libndctl-nfit.h b/ndctl/libndctl-nfit.h
> > new file mode 100644
> > index 0000000..1258d2e
> > --- /dev/null
> > +++ b/ndctl/libndctl-nfit.h
> > @@ -0,0 +1,76 @@
> > +/*
> > + * 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.
> > + */
> > +
> > +#ifndef __LIBNDCTL_NFIT_H__
> > +#define __LIBNDCTL_NFIT_H__
> > +
> > +/*
> > + * libndctl-nfit.h : definitions for NFIT related commands/functions.
> > + */
> > +
> > +/* nfit command numbers which are called via ND_CMD_CALL */
> > +enum {
> > +       NFIT_CMD_TRANSLATE_SPA = 5,
> > +       NFIT_CMD_ARS_INJECT_SET = 7,
> > +       NFIT_CMD_ARS_INJECT_CLEAR = 8,
> > +       NFIT_CMD_ARS_INJECT_GET = 9,
> > +};
> > +
> > +/* error number of Translate SPA by firmware  */
> > +#define ND_TRANSLATE_SPA_STATUS_INVALID_SPA  2
> > +
> > +/*
> > + * The following structures are command packages which are
> > + * defined by ACPI 6.2 (or later).
> > + */
> > +
> > +/* For Translate SPA */
> > +struct nd_cmd_translate_spa {
> > +       __u64 spa;
> > +       __u32 status;
> > +       __u8  flags;
> > +       __u8  _reserved[3];
> > +       __u64 translate_length;
> > +       __u32 num_nvdimms;
> > +       struct nd_nvdimm_device {
> > +               __u32 nfit_device_handle;
> > +               __u32 _reserved;
> > +               __u64 dpa;
> > +       } devices[0];
> > +
> > +}; /* naturally packed */
> > +
>
> Sorry, I wasn't clear. Only "nd_cmd_ars_err_inj_stat" is naturally
> packed the others are not. I can fix this up when I apply it.
>

Actually, since there's some other fixups needed on this series, I'll let
you resend this with the "attribute packed" statements added back in. I'll
follow on later with kernel and ndctl patches to remove the ones that are
not needed.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 1/5] ndctl: Introduce libndctl-nfit.h

[Yasunori Goto]

> Actually, since there's some other fixups needed on this series, I'll let
> you resend this with the "attribute packed" statements added back in. I'll
> follow on later with kernel and ndctl patches to remove the ones that are
> not needed.

Hmm, since I may still misunderstand yet, let me confirm it.

In my next patch, "attribute packed" statements should be added 
to ALL of structures again.

(that is, the followings should have "attribute packed" again.
  - nd_cmd_translate_spa
  - nd_cmd_ars_err_inj
  - nd_cmd_ars_err_inj_clear
  - nd_cmd_ars_err_inj_stat
)

Is this correct?




_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 1/5] ndctl: Introduce libndctl-nfit.h

[Dan Williams]

On Tue, Sep 5, 2017 at 6:28 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> > Actually, since there's some other fixups needed on this series, I'll let
> > you resend this with the "attribute packed" statements added back in.
> I'll
> > follow on later with kernel and ndctl patches to remove the ones that are
> > not needed.
>
> Hmm, since I may still misunderstand yet, let me confirm it.
>
> In my next patch, "attribute packed" statements should be added
> to ALL of structures again.
>
> (that is, the followings should have "attribute packed" again.
>   - nd_cmd_translate_spa
>   - nd_cmd_ars_err_inj
>   - nd_cmd_ars_err_inj_clear
>   - nd_cmd_ars_err_inj_stat
> )
>
> Is this correct?
>

Yes.

Then I can address all the places where the cleanup can happen in one patch
rather than just doing this one change in isolation. For example, some of
the existing ones in ndctl.h need conversion, as well as some of the
payloads in hpe.h.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 4/5] ndctl: Make new interfaces to get information by Physical Address

[Yasunori Goto]

> On Fri, Sep 1, 2017 at 6:21 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> >
> > This patch makes new interfaces :
> >   - Confirm NFIT command is supported or not. (nfit.c)
> >   - Call translate SPA featture of ACPI 6.2. (nfit.c)
> >   - Find region by System Physical Address (libndctl.c)
> >   - Find DIMM by System Physical Address. (libndctl.c)
> >
> > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> > ---
> >  ndctl/lib/Makefile.am  |   2 +
> >  ndctl/lib/libndctl.c   |  69 +++++++++++++++++++++++
> >  ndctl/lib/libndctl.sym |   2 +
> >  ndctl/lib/nfit.c       | 147 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  ndctl/libndctl-nfit.h  |   4 ++
> >  ndctl/libndctl.h.in    |   4 ++
> >  6 files changed, 228 insertions(+)
> >
> > diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> > index d8df87d..9a7734d 100644
> > --- a/ndctl/lib/Makefile.am
> > +++ b/ndctl/lib/Makefile.am
> > @@ -36,6 +36,8 @@ libndctl_la_SOURCES += hpe1.c
> >  libndctl_la_SOURCES += msft.c
> >  endif
> >
> > +libndctl_la_SOURCES += nfit.c
> > +
> >  EXTRA_DIST += libndctl.sym
> >
> >  libndctl_la_LDFLAGS = $(AM_LDFLAGS) \
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 740b6f1..6cc8e1c 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -38,6 +38,7 @@
> >  #include <ndctl/libndctl.h>
> >  #include <ndctl/namespace.h>
> >  #include <daxctl/libdaxctl.h>
> > +#include <ndctl/libndctl-nfit.h>
> >  #include "private.h"
> >
> >  static uuid_t null_uuid;
> > @@ -1570,6 +1571,74 @@ static struct ndctl_dimm *ndctl_dimm_get_by_id(struct ndctl_bus *bus, unsigned i
> >         return NULL;
> >  }
> >
> > +/**
> > + * ndctl_region_get_by_physical_address - get region by physical address
> > + * @bus: ndctl_bus instance
> > + * @address: (System) Physical Address
> > + *
> > + * If @bus and @address is valid, returns a region address, which
> > + * physical address belongs to.
> > + */
> > +NDCTL_EXPORT struct ndctl_region *ndctl_region_get_by_physical_address(struct ndctl_bus *bus,
> > +               unsigned long long address)
> 
> This should be named ndctl_bus_get_region_by_physical_address() since
> it is operating on a bus.
> 
> > +{
> > +       unsigned long long region_start, region_end;
> > +       struct ndctl_region *region;
> > +
> > +       ndctl_region_foreach(bus, region) {
> > +               region_start = ndctl_region_get_resource(region);
> > +               region_end = region_start + ndctl_region_get_size(region);
> > +               if (region_start <= address && address < region_end)
> > +                       return region;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> > +/**
> > + * ndctl_dimm_get_by_physical_address - get ndctl_dimm pointer by physical address
> > + * @bus: ndctl_bus instance
> > + * @address: (System) Physical Address
> > + *
> > + * Returns address of ndctl_dimm on success.
> > + */
> > +NDCTL_EXPORT struct ndctl_dimm *ndctl_dimm_get_by_physical_address(struct ndctl_bus *bus,
> > +               unsigned long long address)
> > +{
> 
> ...and for the same reason this should be
> ndctl_bus_get_dimm_by_physical_address()
> 
> > +       int rc;
> > +        unsigned int handle;
> > +       unsigned long long dpa;
> > +       struct ndctl_region *region;
> > +
> > +       if (!bus)
> > +               return NULL;
> > +
> > +       region = ndctl_region_get_by_physical_address(bus, address);
> > +       if (!region)
> > +               return NULL;
> > +
> > +       if (ndctl_region_get_interleave_ways(region) == 1) {
> > +               /*
> > +                * No need to ask firmware. The first dimm is iff.
> > +                */
> > +               struct ndctl_mapping *mapping = ndctl_mapping_get_first(region);
> > +               if (!mapping)
> > +                       return NULL;
> > +               return ndctl_mapping_get_dimm(mapping);
> > +       }
> > +
> > +       /*
> > +        * Since the region is interleaved, we need to ask firmware about it.
> > +        * If it supports Translate SPA, the dimm is returned.
> > +        */
> > +        rc = ndctl_bus_cmd_translate_spa(bus, address, &handle, &dpa);
> > +        if (rc)
> > +                return NULL;
> 
> Since this does not return an ndctl_cmd and a "handle" is an NFIT
> specific attribute, let's change this to:
> 
>         if (ndctl_bus_has_nfit(bus))
>                 dimm = ndctl_bus_nfit_translate_spa(bus, address);
> 
> ...in the future if a new bus type comes along we can add:
> 
>          else if (ndctl_bus_has_<type>(bus))
>                 dimm = ndctl_bus_<type>_translate_spa(bus, address);
> 
> 
> 
> > +
> > +        return ndctl_dimm_get_by_handle(bus, handle);
> > +}
> > +
> > +
> >  static int region_set_type(struct ndctl_region *region, char *path)
> >  {
> >         struct ndctl_ctx *ctx = ndctl_region_get_ctx(region);
> > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > index 3f9bf09..888108d 100644
> > --- a/ndctl/lib/libndctl.sym
> > +++ b/ndctl/lib/libndctl.sym
> > @@ -77,6 +77,7 @@ global:
> >         ndctl_dimm_get_bus;
> >         ndctl_dimm_get_ctx;
> >         ndctl_dimm_get_by_handle;
> > +       ndctl_dimm_get_by_physical_address;
> >         ndctl_dimm_is_active;
> >         ndctl_dimm_is_enabled;
> >         ndctl_dimm_disable;
> > @@ -136,6 +137,7 @@ global:
> >         ndctl_region_get_ctx;
> >         ndctl_region_get_first_dimm;
> >         ndctl_region_get_next_dimm;
> > +       ndctl_region_get_by_physical_address;
> >         ndctl_region_is_enabled;
> >         ndctl_region_enable;
> >         ndctl_region_disable_invalidate;
> > diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
> > new file mode 100644
> > index 0000000..79d966d
> > --- /dev/null
> > +++ b/ndctl/lib/nfit.c
> > @@ -0,0 +1,147 @@
> > +/*
> > + * Copyright (c) 2017, FUJITSU LIMITED. All rights reserved.
> > + *
> > + * 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 <stdlib.h>
> > +#include <ndctl/libndctl.h>
> > +#include "private.h"
> > +#include <ndctl/libndctl-nfit.h>
> > +
> > +struct ndctl_bus;
> > +
> > +/**
> > + * ndctl_bus_is_nfit_cmd_supported - ask nfit command is supported on @bus.
> > + * @bus: ndctl_bus instance
> > + * @cmd: nfit command number (defined as NFIT_CMD_XXX in libndctl-nfit.h)
> > + *
> > + * Return 1: command is supported. Return 0: command is not supported.
> > + *
> > + */
> > +NDCTL_EXPORT int ndctl_bus_is_nfit_cmd_supported(struct ndctl_bus *bus,
> > +                int cmd)
> > +{
> > +        return !!(ndctl_bus_get_nfit_dsm_mask(bus) & (1ULL << cmd));
> > +}
> > +
> > +static int bus_has_translate_spa(struct ndctl_bus *bus)
> > +{
> > +       if (!ndctl_bus_has_nfit(bus))
> > +               return 0;
> > +
> > +       return ndctl_bus_is_nfit_cmd_supported(bus, NFIT_CMD_TRANSLATE_SPA);
> > +}
> > +
> > +static struct ndctl_cmd *ndctl_bus_cmd_new_translate_spa(struct ndctl_bus *bus)
> > +{
> > +       struct ndctl_cmd *cmd;
> > +       struct nd_cmd_pkg *pkg;
> > +       struct nd_cmd_translate_spa *translate_spa;
> > +       size_t size, spa_length;
> > +
> > +       spa_length = sizeof(struct nd_cmd_translate_spa)
> > +               + sizeof(struct nd_nvdimm_device);
> > +       size = sizeof(*cmd) + sizeof(*pkg) + spa_length;
> > +       cmd = calloc(1, size);
> > +       if (!cmd)
> > +               return NULL;
> > +
> > +       cmd->bus = bus;
> > +       ndctl_cmd_ref(cmd);
> > +       cmd->type = ND_CMD_CALL;
> > +       cmd->size = size;
> > +       cmd->status = 1;
> > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > +       pkg->nd_command = NFIT_CMD_TRANSLATE_SPA;
> > +       pkg->nd_size_in = sizeof(unsigned long long);
> > +       pkg->nd_size_out = spa_length;
> > +       pkg->nd_fw_size = spa_length;
> > +       translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
> > +       cmd->firmware_status = &translate_spa->status;
> > +       translate_spa->translate_length = spa_length;
> > +
> > +       return cmd;
> > +}
> > +
> > +static int ndctl_bus_cmd_get_translate_spa(struct ndctl_cmd *cmd,
> > +                                       unsigned int *handle, unsigned long long *dpa)
> > +{
> > +       struct nd_cmd_pkg *pkg;
> > +       struct nd_cmd_translate_spa *translate_spa;
> > +
> > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > +       translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
> > +
> > +       if (translate_spa->status == ND_TRANSLATE_SPA_STATUS_INVALID_SPA)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * XXX: Currently NVDIMM mirroring is not supported.
> > +        * Even if ACPI returned plural dimms due to mirroring,
> > +        * this function returns just the first dimm.
> > +        */
> > +
> > +       *handle = translate_spa->devices[0].nfit_device_handle;
> > +       *dpa = translate_spa->devices[0].dpa;
> > +
> > +       return 0;
> > +}
> > +
> > +static int is_valid_spa(struct ndctl_bus *bus, unsigned long long spa)
> > +{
> > +       return !!ndctl_region_get_by_physical_address(bus, spa);
> > +}
> > +
> > +/**
> > + * ndctl_bus_cmd_translate_spa - call translate spa.
> > + * @bus: bus which belongs to.
> > + * @address: address (System Physical Address)
> > + * @handle: pointer to return dimm handle
> > + * @dpa: pointer to return Dimm Physical address
> > + *
> > + * If success, returns zero, store dimm's @handle, and @dpa.
> > + */
> > +NDCTL_EXPORT int ndctl_bus_cmd_translate_spa(struct ndctl_bus *bus,
> > +       unsigned long long address, unsigned int *handle, unsigned long long *dpa)
> > +{
> > +
> > +       struct ndctl_cmd *cmd;
> > +       struct nd_cmd_pkg *pkg;
> > +       struct nd_cmd_translate_spa *translate_spa;
> > +       int rc;
> > +
> > +       if (!bus || !handle || !dpa)
> > +               return -EINVAL;
> > +
> > +       if (!bus_has_translate_spa(bus))
> > +               return -ENOTTY;
> > +
> > +       if (!is_valid_spa(bus, address))
> > +               return -EINVAL;
> > +
> > +       cmd = ndctl_bus_cmd_new_translate_spa(bus);
> > +       if (!cmd)
> > +               return -ENOMEM;
> > +
> > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > +       translate_spa = (struct nd_cmd_translate_spa *)&pkg->nd_payload[0];
> > +       translate_spa->spa = address;
> > +
> > +       rc = ndctl_cmd_submit(cmd);
> > +       if (rc) {
> > +               ndctl_cmd_unref(cmd);
> > +               return rc;
> > +       }
> > +
> > +       rc = ndctl_bus_cmd_get_translate_spa(cmd, handle, dpa);
> > +       ndctl_cmd_unref(cmd);
> > +
> > +       return rc;
> 
> Since the "handle" is not generic I don't think we can make this
> publicly exported. Let's hide this detail behind
> ndctl_bus_nfit_translate_spa() that is called by
> ndctl_bus_get_dimm_by_physical_address().
> 

Hmmmmm.

ndctl_bus_cmd_translate_spa() is defined at ndctl/lib/nfit.c, 
but ndctl_bus_get_dimm_by_physical_address() is defined at ndctl/lib/libndctl.c,

So, even if ndctl_bus_nfit_translate_spa() becomes local definition,
another function must be created and exported yet.

Current my idea is...

- ndctl_bus_get_dimm_by_physical_address() is defined at ndctl/lib/libndctl.c.
- ndctl_bus_cmd_translate_spa() is defined at ndctl/lib/nfit.c, and becomes local definition.
- ndctl_bus_nfit_get_dimm_by_physical_address() is created at ndctl/lib/nfit.c, and
  it is exported.

How do you think?





_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 4/5] ndctl: Make new interfaces to get information by Physical Address

[Dan Williams]

On Tue, Sep 5, 2017 at 7:21 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> > On Fri, Sep 1, 2017 at 6:21 AM, Yasunori Goto <y-goto@jp.fujitsu.com>
> wrote:
> > >
> > > This patch makes new interfaces :
> > >   - Confirm NFIT command is supported or not. (nfit.c)
> > >   - Call translate SPA featture of ACPI 6.2. (nfit.c)
> > >   - Find region by System Physical Address (libndctl.c)
> > >   - Find DIMM by System Physical Address. (libndctl.c)
> > >
> > > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> > > ---
> > >  ndctl/lib/Makefile.am  |   2 +
> > >  ndctl/lib/libndctl.c   |  69 +++++++++++++++++++++++
> > >  ndctl/lib/libndctl.sym |   2 +
> > >  ndctl/lib/nfit.c       | 147 ++++++++++++++++++++++++++++++
> +++++++++++++++++++
> > >  ndctl/libndctl-nfit.h  |   4 ++
> > >  ndctl/libndctl.h.in    |   4 ++
> > >  6 files changed, 228 insertions(+)
> > >
> > > diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> > > index d8df87d..9a7734d 100644
> > > --- a/ndctl/lib/Makefile.am
> > > +++ b/ndctl/lib/Makefile.am
> > > @@ -36,6 +36,8 @@ libndctl_la_SOURCES += hpe1.c
> > >  libndctl_la_SOURCES += msft.c
> > >  endif
> > >
> > > +libndctl_la_SOURCES += nfit.c
> > > +
> > >  EXTRA_DIST += libndctl.sym
> > >
> > >  libndctl_la_LDFLAGS = $(AM_LDFLAGS) \
> > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > > index 740b6f1..6cc8e1c 100644
> > > --- a/ndctl/lib/libndctl.c
> > > +++ b/ndctl/lib/libndctl.c
> > > @@ -38,6 +38,7 @@
> > >  #include <ndctl/libndctl.h>
> > >  #include <ndctl/namespace.h>
> > >  #include <daxctl/libdaxctl.h>
> > > +#include <ndctl/libndctl-nfit.h>
> > >  #include "private.h"
> > >
> > >  static uuid_t null_uuid;
> > > @@ -1570,6 +1571,74 @@ static struct ndctl_dimm
> *ndctl_dimm_get_by_id(struct ndctl_bus *bus, unsigned i
> > >         return NULL;
> > >  }
> > >
> > > +/**
> > > + * ndctl_region_get_by_physical_address - get region by physical
> address
> > > + * @bus: ndctl_bus instance
> > > + * @address: (System) Physical Address
> > > + *
> > > + * If @bus and @address is valid, returns a region address, which
> > > + * physical address belongs to.
> > > + */
> > > +NDCTL_EXPORT struct ndctl_region *ndctl_region_get_by_physical_address(struct
> ndctl_bus *bus,
> > > +               unsigned long long address)
> >
> > This should be named ndctl_bus_get_region_by_physical_address() since
> > it is operating on a bus.
> >
> > > +{
> > > +       unsigned long long region_start, region_end;
> > > +       struct ndctl_region *region;
> > > +
> > > +       ndctl_region_foreach(bus, region) {
> > > +               region_start = ndctl_region_get_resource(region);
> > > +               region_end = region_start +
> ndctl_region_get_size(region);
> > > +               if (region_start <= address && address < region_end)
> > > +                       return region;
> > > +       }
> > > +
> > > +       return NULL;
> > > +}
> > > +
> > > +/**
> > > + * ndctl_dimm_get_by_physical_address - get ndctl_dimm pointer by
> physical address
> > > + * @bus: ndctl_bus instance
> > > + * @address: (System) Physical Address
> > > + *
> > > + * Returns address of ndctl_dimm on success.
> > > + */
> > > +NDCTL_EXPORT struct ndctl_dimm *ndctl_dimm_get_by_physical_address(struct
> ndctl_bus *bus,
> > > +               unsigned long long address)
> > > +{
> >
> > ...and for the same reason this should be
> > ndctl_bus_get_dimm_by_physical_address()
> >
> > > +       int rc;
> > > +        unsigned int handle;
> > > +       unsigned long long dpa;
> > > +       struct ndctl_region *region;
> > > +
> > > +       if (!bus)
> > > +               return NULL;
> > > +
> > > +       region = ndctl_region_get_by_physical_address(bus, address);
> > > +       if (!region)
> > > +               return NULL;
> > > +
> > > +       if (ndctl_region_get_interleave_ways(region) == 1) {
> > > +               /*
> > > +                * No need to ask firmware. The first dimm is iff.
> > > +                */
> > > +               struct ndctl_mapping *mapping =
> ndctl_mapping_get_first(region);
> > > +               if (!mapping)
> > > +                       return NULL;
> > > +               return ndctl_mapping_get_dimm(mapping);
> > > +       }
> > > +
> > > +       /*
> > > +        * Since the region is interleaved, we need to ask firmware
> about it.
> > > +        * If it supports Translate SPA, the dimm is returned.
> > > +        */
> > > +        rc = ndctl_bus_cmd_translate_spa(bus, address, &handle,
> &dpa);
> > > +        if (rc)
> > > +                return NULL;
> >
> > Since this does not return an ndctl_cmd and a "handle" is an NFIT
> > specific attribute, let's change this to:
> >
> >         if (ndctl_bus_has_nfit(bus))
> >                 dimm = ndctl_bus_nfit_translate_spa(bus, address);
> >
> > ...in the future if a new bus type comes along we can add:
> >
> >          else if (ndctl_bus_has_<type>(bus))
> >                 dimm = ndctl_bus_<type>_translate_spa(bus, address);
> >
> >
> >
> > > +
> > > +        return ndctl_dimm_get_by_handle(bus, handle);
> > > +}
> > > +
> > > +
> > >  static int region_set_type(struct ndctl_region *region, char *path)
> > >  {
> > >         struct ndctl_ctx *ctx = ndctl_region_get_ctx(region);
> > > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > > index 3f9bf09..888108d 100644
> > > --- a/ndctl/lib/libndctl.sym
> > > +++ b/ndctl/lib/libndctl.sym
> > > @@ -77,6 +77,7 @@ global:
> > >         ndctl_dimm_get_bus;
> > >         ndctl_dimm_get_ctx;
> > >         ndctl_dimm_get_by_handle;
> > > +       ndctl_dimm_get_by_physical_address;
> > >         ndctl_dimm_is_active;
> > >         ndctl_dimm_is_enabled;
> > >         ndctl_dimm_disable;
> > > @@ -136,6 +137,7 @@ global:
> > >         ndctl_region_get_ctx;
> > >         ndctl_region_get_first_dimm;
> > >         ndctl_region_get_next_dimm;
> > > +       ndctl_region_get_by_physical_address;
> > >         ndctl_region_is_enabled;
> > >         ndctl_region_enable;
> > >         ndctl_region_disable_invalidate;
> > > diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
> > > new file mode 100644
> > > index 0000000..79d966d
> > > --- /dev/null
> > > +++ b/ndctl/lib/nfit.c
> > > @@ -0,0 +1,147 @@
> > > +/*
> > > + * Copyright (c) 2017, FUJITSU LIMITED. All rights reserved.
> > > + *
> > > + * 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 <stdlib.h>
> > > +#include <ndctl/libndctl.h>
> > > +#include "private.h"
> > > +#include <ndctl/libndctl-nfit.h>
> > > +
> > > +struct ndctl_bus;
> > > +
> > > +/**
> > > + * ndctl_bus_is_nfit_cmd_supported - ask nfit command is supported
> on @bus.
> > > + * @bus: ndctl_bus instance
> > > + * @cmd: nfit command number (defined as NFIT_CMD_XXX in
> libndctl-nfit.h)
> > > + *
> > > + * Return 1: command is supported. Return 0: command is not supported.
> > > + *
> > > + */
> > > +NDCTL_EXPORT int ndctl_bus_is_nfit_cmd_supported(struct ndctl_bus
> *bus,
> > > +                int cmd)
> > > +{
> > > +        return !!(ndctl_bus_get_nfit_dsm_mask(bus) & (1ULL << cmd));
> > > +}
> > > +
> > > +static int bus_has_translate_spa(struct ndctl_bus *bus)
> > > +{
> > > +       if (!ndctl_bus_has_nfit(bus))
> > > +               return 0;
> > > +
> > > +       return ndctl_bus_is_nfit_cmd_supported(bus,
> NFIT_CMD_TRANSLATE_SPA);
> > > +}
> > > +
> > > +static struct ndctl_cmd *ndctl_bus_cmd_new_translate_spa(struct
> ndctl_bus *bus)
> > > +{
> > > +       struct ndctl_cmd *cmd;
> > > +       struct nd_cmd_pkg *pkg;
> > > +       struct nd_cmd_translate_spa *translate_spa;
> > > +       size_t size, spa_length;
> > > +
> > > +       spa_length = sizeof(struct nd_cmd_translate_spa)
> > > +               + sizeof(struct nd_nvdimm_device);
> > > +       size = sizeof(*cmd) + sizeof(*pkg) + spa_length;
> > > +       cmd = calloc(1, size);
> > > +       if (!cmd)
> > > +               return NULL;
> > > +
> > > +       cmd->bus = bus;
> > > +       ndctl_cmd_ref(cmd);
> > > +       cmd->type = ND_CMD_CALL;
> > > +       cmd->size = size;
> > > +       cmd->status = 1;
> > > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > > +       pkg->nd_command = NFIT_CMD_TRANSLATE_SPA;
> > > +       pkg->nd_size_in = sizeof(unsigned long long);
> > > +       pkg->nd_size_out = spa_length;
> > > +       pkg->nd_fw_size = spa_length;
> > > +       translate_spa = (struct nd_cmd_translate_spa
> *)&pkg->nd_payload[0];
> > > +       cmd->firmware_status = &translate_spa->status;
> > > +       translate_spa->translate_length = spa_length;
> > > +
> > > +       return cmd;
> > > +}
> > > +
> > > +static int ndctl_bus_cmd_get_translate_spa(struct ndctl_cmd *cmd,
> > > +                                       unsigned int *handle, unsigned
> long long *dpa)
> > > +{
> > > +       struct nd_cmd_pkg *pkg;
> > > +       struct nd_cmd_translate_spa *translate_spa;
> > > +
> > > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > > +       translate_spa = (struct nd_cmd_translate_spa
> *)&pkg->nd_payload[0];
> > > +
> > > +       if (translate_spa->status == ND_TRANSLATE_SPA_STATUS_
> INVALID_SPA)
> > > +               return -EINVAL;
> > > +
> > > +       /*
> > > +        * XXX: Currently NVDIMM mirroring is not supported.
> > > +        * Even if ACPI returned plural dimms due to mirroring,
> > > +        * this function returns just the first dimm.
> > > +        */
> > > +
> > > +       *handle = translate_spa->devices[0].nfit_device_handle;
> > > +       *dpa = translate_spa->devices[0].dpa;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int is_valid_spa(struct ndctl_bus *bus, unsigned long long spa)
> > > +{
> > > +       return !!ndctl_region_get_by_physical_address(bus, spa);
> > > +}
> > > +
> > > +/**
> > > + * ndctl_bus_cmd_translate_spa - call translate spa.
> > > + * @bus: bus which belongs to.
> > > + * @address: address (System Physical Address)
> > > + * @handle: pointer to return dimm handle
> > > + * @dpa: pointer to return Dimm Physical address
> > > + *
> > > + * If success, returns zero, store dimm's @handle, and @dpa.
> > > + */
> > > +NDCTL_EXPORT int ndctl_bus_cmd_translate_spa(struct ndctl_bus *bus,
> > > +       unsigned long long address, unsigned int *handle, unsigned
> long long *dpa)
> > > +{
> > > +
> > > +       struct ndctl_cmd *cmd;
> > > +       struct nd_cmd_pkg *pkg;
> > > +       struct nd_cmd_translate_spa *translate_spa;
> > > +       int rc;
> > > +
> > > +       if (!bus || !handle || !dpa)
> > > +               return -EINVAL;
> > > +
> > > +       if (!bus_has_translate_spa(bus))
> > > +               return -ENOTTY;
> > > +
> > > +       if (!is_valid_spa(bus, address))
> > > +               return -EINVAL;
> > > +
> > > +       cmd = ndctl_bus_cmd_new_translate_spa(bus);
> > > +       if (!cmd)
> > > +               return -ENOMEM;
> > > +
> > > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > > +       translate_spa = (struct nd_cmd_translate_spa
> *)&pkg->nd_payload[0];
> > > +       translate_spa->spa = address;
> > > +
> > > +       rc = ndctl_cmd_submit(cmd);
> > > +       if (rc) {
> > > +               ndctl_cmd_unref(cmd);
> > > +               return rc;
> > > +       }
> > > +
> > > +       rc = ndctl_bus_cmd_get_translate_spa(cmd, handle, dpa);
> > > +       ndctl_cmd_unref(cmd);
> > > +
> > > +       return rc;
> >
> > Since the "handle" is not generic I don't think we can make this
> > publicly exported. Let's hide this detail behind
> > ndctl_bus_nfit_translate_spa() that is called by
> > ndctl_bus_get_dimm_by_physical_address().
> >
>
> Hmmmmm.
>
> ndctl_bus_cmd_translate_spa() is defined at ndctl/lib/nfit.c,
> but ndctl_bus_get_dimm_by_physical_address() is defined at
> ndctl/lib/libndctl.c,
>
> So, even if ndctl_bus_nfit_translate_spa() becomes local definition,
> another function must be created and exported yet.
>
> Current my idea is...
>
> - ndctl_bus_get_dimm_by_physical_address() is defined at
> ndctl/lib/libndctl.c.
> - ndctl_bus_cmd_translate_spa() is defined at ndctl/lib/nfit.c, and
> becomes local definition.
> - ndctl_bus_nfit_get_dimm_by_physical_address() is created at
> ndctl/lib/nfit.c, and
>   it is exported.
>
> How do you think?
>

You don't need to NDCTL_EXPORT a routine to share it between library source
files. NDCTL_EXPORT is only for declaring symbols that will become library
interfaces. For example, see the definition of region_flag_refresh() in the
latest state of the pending branch. It is defined in ndctl/lib/libndctl.c,
but called from ndctl/lib/dimm.c.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 4/5] ndctl: Make new interfaces to get information by Physical Address

[Yasunori Goto]

> On Tue, Sep 5, 2017 at 7:21 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> 
> > > On Fri, Sep 1, 2017 at 6:21 AM, Yasunori Goto <y-goto@jp.fujitsu.com>
> > wrote:
> > > >
> > > > This patch makes new interfaces :
> > > >   - Confirm NFIT command is supported or not. (nfit.c)
> > > >   - Call translate SPA featture of ACPI 6.2. (nfit.c)
> > > >   - Find region by System Physical Address (libndctl.c)
> > > >   - Find DIMM by System Physical Address. (libndctl.c)
> > > >
> > > > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> > > > ---
> > > >  ndctl/lib/Makefile.am  |   2 +
> > > >  ndctl/lib/libndctl.c   |  69 +++++++++++++++++++++++
> > > >  ndctl/lib/libndctl.sym |   2 +
> > > >  ndctl/lib/nfit.c       | 147 ++++++++++++++++++++++++++++++
> > +++++++++++++++++++
> > > >  ndctl/libndctl-nfit.h  |   4 ++
> > > >  ndctl/libndctl.h.in    |   4 ++
> > > >  6 files changed, 228 insertions(+)
> > > >
> > > > diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> > > > index d8df87d..9a7734d 100644
> > > > --- a/ndctl/lib/Makefile.am
> > > > +++ b/ndctl/lib/Makefile.am
> > > > @@ -36,6 +36,8 @@ libndctl_la_SOURCES += hpe1.c
> > > >  libndctl_la_SOURCES += msft.c
> > > >  endif
> > > >
> > > > +libndctl_la_SOURCES += nfit.c
> > > > +
> > > >  EXTRA_DIST += libndctl.sym
> > > >
> > > >  libndctl_la_LDFLAGS = $(AM_LDFLAGS) \
> > > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > > > index 740b6f1..6cc8e1c 100644
> > > > --- a/ndctl/lib/libndctl.c
> > > > +++ b/ndctl/lib/libndctl.c
> > > > @@ -38,6 +38,7 @@
> > > >  #include <ndctl/libndctl.h>
> > > >  #include <ndctl/namespace.h>
> > > >  #include <daxctl/libdaxctl.h>
> > > > +#include <ndctl/libndctl-nfit.h>
> > > >  #include "private.h"
> > > >
> > > >  static uuid_t null_uuid;
> > > > @@ -1570,6 +1571,74 @@ static struct ndctl_dimm
> > *ndctl_dimm_get_by_id(struct ndctl_bus *bus, unsigned i
> > > >         return NULL;
> > > >  }
> > > >
> > > > +/**
> > > > + * ndctl_region_get_by_physical_address - get region by physical
> > address
> > > > + * @bus: ndctl_bus instance
> > > > + * @address: (System) Physical Address
> > > > + *
> > > > + * If @bus and @address is valid, returns a region address, which
> > > > + * physical address belongs to.
> > > > + */
> > > > +NDCTL_EXPORT struct ndctl_region *ndctl_region_get_by_physical_address(struct
> > ndctl_bus *bus,
> > > > +               unsigned long long address)
> > >
> > > This should be named ndctl_bus_get_region_by_physical_address() since
> > > it is operating on a bus.
> > >
> > > > +{
> > > > +       unsigned long long region_start, region_end;
> > > > +       struct ndctl_region *region;
> > > > +
> > > > +       ndctl_region_foreach(bus, region) {
> > > > +               region_start = ndctl_region_get_resource(region);
> > > > +               region_end = region_start +
> > ndctl_region_get_size(region);
> > > > +               if (region_start <= address && address < region_end)
> > > > +                       return region;
> > > > +       }
> > > > +
> > > > +       return NULL;
> > > > +}
> > > > +
> > > > +/**
> > > > + * ndctl_dimm_get_by_physical_address - get ndctl_dimm pointer by
> > physical address
> > > > + * @bus: ndctl_bus instance
> > > > + * @address: (System) Physical Address
> > > > + *
> > > > + * Returns address of ndctl_dimm on success.
> > > > + */
> > > > +NDCTL_EXPORT struct ndctl_dimm *ndctl_dimm_get_by_physical_address(struct
> > ndctl_bus *bus,
> > > > +               unsigned long long address)
> > > > +{
> > >
> > > ...and for the same reason this should be
> > > ndctl_bus_get_dimm_by_physical_address()
> > >
> > > > +       int rc;
> > > > +        unsigned int handle;
> > > > +       unsigned long long dpa;
> > > > +       struct ndctl_region *region;
> > > > +
> > > > +       if (!bus)
> > > > +               return NULL;
> > > > +
> > > > +       region = ndctl_region_get_by_physical_address(bus, address);
> > > > +       if (!region)
> > > > +               return NULL;
> > > > +
> > > > +       if (ndctl_region_get_interleave_ways(region) == 1) {
> > > > +               /*
> > > > +                * No need to ask firmware. The first dimm is iff.
> > > > +                */
> > > > +               struct ndctl_mapping *mapping =
> > ndctl_mapping_get_first(region);
> > > > +               if (!mapping)
> > > > +                       return NULL;
> > > > +               return ndctl_mapping_get_dimm(mapping);
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * Since the region is interleaved, we need to ask firmware
> > about it.
> > > > +        * If it supports Translate SPA, the dimm is returned.
> > > > +        */
> > > > +        rc = ndctl_bus_cmd_translate_spa(bus, address, &handle,
> > &dpa);
> > > > +        if (rc)
> > > > +                return NULL;
> > >
> > > Since this does not return an ndctl_cmd and a "handle" is an NFIT
> > > specific attribute, let's change this to:
> > >
> > >         if (ndctl_bus_has_nfit(bus))
> > >                 dimm = ndctl_bus_nfit_translate_spa(bus, address);
> > >
> > > ...in the future if a new bus type comes along we can add:
> > >
> > >          else if (ndctl_bus_has_<type>(bus))
> > >                 dimm = ndctl_bus_<type>_translate_spa(bus, address);
> > >
> > >
> > >
> > > > +
> > > > +        return ndctl_dimm_get_by_handle(bus, handle);
> > > > +}
> > > > +
> > > > +
> > > >  static int region_set_type(struct ndctl_region *region, char *path)
> > > >  {
> > > >         struct ndctl_ctx *ctx = ndctl_region_get_ctx(region);
> > > > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > > > index 3f9bf09..888108d 100644
> > > > --- a/ndctl/lib/libndctl.sym
> > > > +++ b/ndctl/lib/libndctl.sym
> > > > @@ -77,6 +77,7 @@ global:
> > > >         ndctl_dimm_get_bus;
> > > >         ndctl_dimm_get_ctx;
> > > >         ndctl_dimm_get_by_handle;
> > > > +       ndctl_dimm_get_by_physical_address;
> > > >         ndctl_dimm_is_active;
> > > >         ndctl_dimm_is_enabled;
> > > >         ndctl_dimm_disable;
> > > > @@ -136,6 +137,7 @@ global:
> > > >         ndctl_region_get_ctx;
> > > >         ndctl_region_get_first_dimm;
> > > >         ndctl_region_get_next_dimm;
> > > > +       ndctl_region_get_by_physical_address;
> > > >         ndctl_region_is_enabled;
> > > >         ndctl_region_enable;
> > > >         ndctl_region_disable_invalidate;
> > > > diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
> > > > new file mode 100644
> > > > index 0000000..79d966d
> > > > --- /dev/null
> > > > +++ b/ndctl/lib/nfit.c
> > > > @@ -0,0 +1,147 @@
> > > > +/*
> > > > + * Copyright (c) 2017, FUJITSU LIMITED. All rights reserved.
> > > > + *
> > > > + * 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 <stdlib.h>
> > > > +#include <ndctl/libndctl.h>
> > > > +#include "private.h"
> > > > +#include <ndctl/libndctl-nfit.h>
> > > > +
> > > > +struct ndctl_bus;
> > > > +
> > > > +/**
> > > > + * ndctl_bus_is_nfit_cmd_supported - ask nfit command is supported
> > on @bus.
> > > > + * @bus: ndctl_bus instance
> > > > + * @cmd: nfit command number (defined as NFIT_CMD_XXX in
> > libndctl-nfit.h)
> > > > + *
> > > > + * Return 1: command is supported. Return 0: command is not supported.
> > > > + *
> > > > + */
> > > > +NDCTL_EXPORT int ndctl_bus_is_nfit_cmd_supported(struct ndctl_bus
> > *bus,
> > > > +                int cmd)
> > > > +{
> > > > +        return !!(ndctl_bus_get_nfit_dsm_mask(bus) & (1ULL << cmd));
> > > > +}
> > > > +
> > > > +static int bus_has_translate_spa(struct ndctl_bus *bus)
> > > > +{
> > > > +       if (!ndctl_bus_has_nfit(bus))
> > > > +               return 0;
> > > > +
> > > > +       return ndctl_bus_is_nfit_cmd_supported(bus,
> > NFIT_CMD_TRANSLATE_SPA);
> > > > +}
> > > > +
> > > > +static struct ndctl_cmd *ndctl_bus_cmd_new_translate_spa(struct
> > ndctl_bus *bus)
> > > > +{
> > > > +       struct ndctl_cmd *cmd;
> > > > +       struct nd_cmd_pkg *pkg;
> > > > +       struct nd_cmd_translate_spa *translate_spa;
> > > > +       size_t size, spa_length;
> > > > +
> > > > +       spa_length = sizeof(struct nd_cmd_translate_spa)
> > > > +               + sizeof(struct nd_nvdimm_device);
> > > > +       size = sizeof(*cmd) + sizeof(*pkg) + spa_length;
> > > > +       cmd = calloc(1, size);
> > > > +       if (!cmd)
> > > > +               return NULL;
> > > > +
> > > > +       cmd->bus = bus;
> > > > +       ndctl_cmd_ref(cmd);
> > > > +       cmd->type = ND_CMD_CALL;
> > > > +       cmd->size = size;
> > > > +       cmd->status = 1;
> > > > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > > > +       pkg->nd_command = NFIT_CMD_TRANSLATE_SPA;
> > > > +       pkg->nd_size_in = sizeof(unsigned long long);
> > > > +       pkg->nd_size_out = spa_length;
> > > > +       pkg->nd_fw_size = spa_length;
> > > > +       translate_spa = (struct nd_cmd_translate_spa
> > *)&pkg->nd_payload[0];
> > > > +       cmd->firmware_status = &translate_spa->status;
> > > > +       translate_spa->translate_length = spa_length;
> > > > +
> > > > +       return cmd;
> > > > +}
> > > > +
> > > > +static int ndctl_bus_cmd_get_translate_spa(struct ndctl_cmd *cmd,
> > > > +                                       unsigned int *handle, unsigned
> > long long *dpa)
> > > > +{
> > > > +       struct nd_cmd_pkg *pkg;
> > > > +       struct nd_cmd_translate_spa *translate_spa;
> > > > +
> > > > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > > > +       translate_spa = (struct nd_cmd_translate_spa
> > *)&pkg->nd_payload[0];
> > > > +
> > > > +       if (translate_spa->status == ND_TRANSLATE_SPA_STATUS_
> > INVALID_SPA)
> > > > +               return -EINVAL;
> > > > +
> > > > +       /*
> > > > +        * XXX: Currently NVDIMM mirroring is not supported.
> > > > +        * Even if ACPI returned plural dimms due to mirroring,
> > > > +        * this function returns just the first dimm.
> > > > +        */
> > > > +
> > > > +       *handle = translate_spa->devices[0].nfit_device_handle;
> > > > +       *dpa = translate_spa->devices[0].dpa;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int is_valid_spa(struct ndctl_bus *bus, unsigned long long spa)
> > > > +{
> > > > +       return !!ndctl_region_get_by_physical_address(bus, spa);
> > > > +}
> > > > +
> > > > +/**
> > > > + * ndctl_bus_cmd_translate_spa - call translate spa.
> > > > + * @bus: bus which belongs to.
> > > > + * @address: address (System Physical Address)
> > > > + * @handle: pointer to return dimm handle
> > > > + * @dpa: pointer to return Dimm Physical address
> > > > + *
> > > > + * If success, returns zero, store dimm's @handle, and @dpa.
> > > > + */
> > > > +NDCTL_EXPORT int ndctl_bus_cmd_translate_spa(struct ndctl_bus *bus,
> > > > +       unsigned long long address, unsigned int *handle, unsigned
> > long long *dpa)
> > > > +{
> > > > +
> > > > +       struct ndctl_cmd *cmd;
> > > > +       struct nd_cmd_pkg *pkg;
> > > > +       struct nd_cmd_translate_spa *translate_spa;
> > > > +       int rc;
> > > > +
> > > > +       if (!bus || !handle || !dpa)
> > > > +               return -EINVAL;
> > > > +
> > > > +       if (!bus_has_translate_spa(bus))
> > > > +               return -ENOTTY;
> > > > +
> > > > +       if (!is_valid_spa(bus, address))
> > > > +               return -EINVAL;
> > > > +
> > > > +       cmd = ndctl_bus_cmd_new_translate_spa(bus);
> > > > +       if (!cmd)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       pkg = (struct nd_cmd_pkg *)&cmd->cmd_buf[0];
> > > > +       translate_spa = (struct nd_cmd_translate_spa
> > *)&pkg->nd_payload[0];
> > > > +       translate_spa->spa = address;
> > > > +
> > > > +       rc = ndctl_cmd_submit(cmd);
> > > > +       if (rc) {
> > > > +               ndctl_cmd_unref(cmd);
> > > > +               return rc;
> > > > +       }
> > > > +
> > > > +       rc = ndctl_bus_cmd_get_translate_spa(cmd, handle, dpa);
> > > > +       ndctl_cmd_unref(cmd);
> > > > +
> > > > +       return rc;
> > >
> > > Since the "handle" is not generic I don't think we can make this
> > > publicly exported. Let's hide this detail behind
> > > ndctl_bus_nfit_translate_spa() that is called by
> > > ndctl_bus_get_dimm_by_physical_address().
> > >
> >
> > Hmmmmm.
> >
> > ndctl_bus_cmd_translate_spa() is defined at ndctl/lib/nfit.c,
> > but ndctl_bus_get_dimm_by_physical_address() is defined at
> > ndctl/lib/libndctl.c,
> >
> > So, even if ndctl_bus_nfit_translate_spa() becomes local definition,
> > another function must be created and exported yet.
> >
> > Current my idea is...
> >
> > - ndctl_bus_get_dimm_by_physical_address() is defined at
> > ndctl/lib/libndctl.c.
> > - ndctl_bus_cmd_translate_spa() is defined at ndctl/lib/nfit.c, and
> > becomes local definition.
> > - ndctl_bus_nfit_get_dimm_by_physical_address() is created at
> > ndctl/lib/nfit.c, and
> >   it is exported.
> >
> > How do you think?
> >
> 
> You don't need to NDCTL_EXPORT a routine to share it between library source
> files. NDCTL_EXPORT is only for declaring symbols that will become library
> interfaces. For example, see the definition of region_flag_refresh() in the
> latest state of the pending branch. It is defined in ndctl/lib/libndctl.c,
> but called from ndctl/lib/dimm.c.

Ah... Ok, Thanks. 

I'll make next version.




_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm