[ndctl PATCH v3 0/8] Improvements for namespace creation/interrogation

[ndctl PATCH v3 0/8] Improvements for namespace creation/interrogation

[Dan Williams]

Changes since v2 [1]:
- Drop the patches that have already been applied to the 'pending' branch
- Rebase the dimm extent series and the small 'create-namespace' fixlets
- Move the new libndctl apis to the next library symbol version (Vishal)
- Defer the following to a post ndctl-v66 release:

      ndctl/namespace: Add read-infoblock command
      ndctl/test: Update dax-dev to handle multiple e820 ranges
      ndctl/test: Make dax.sh more robust vs small namespaces
      ndctl/namespace: Always zero info-blocks
      ndctl/namespace: Disable autorecovery of create-namespace failures
      ndctl/test: Checkout device-mapper + dax operation
      ndctl/test: Exercise sub-section sized namespace creation/deletion
      ndctl/namespace: Kill off the legacy mode names
      ndctl/namespace: Introduce mode-to-name and name-to-mode helpers
      ndctl/namespace: Validate namespace size within validate_namespace_options()
      ndctl/namespace: Clarify 16M minimum size requirement

[1]: https://lists.01.org/pipermail/linux-nvdimm/2019-July/022766.html

---

This trimmed version includes the extent support for label operations
which significantly speeds up common label operations like
'init-labels'. It also fixes up some surprising results from
'create-namespace' where it would fail even though available capacity is
present. Lastly it suppresses a new warning found in Fedora Rawhide
builds that has moved to gcc 9.1.1.

---

Dan Williams (8):
      ndctl/build: Suppress -Waddress-of-packed-member
      ndctl/dimm: Support small label reads/writes
      ndctl/dimm: Minimize data-transfer for init-labels
      ndctl/dimm: Add offset and size options to {read,write,zero}-labels
      ndctl/dimm: Limit read-labels with --index option
      ndctl/namespace: Minimize label data transfer for autolabel
      ndctl/namespace: Continue region search on 'missing seed' event
      ndctl/namespace: Report ENOSPC when regions are full


 Documentation/ndctl/labels-options.txt    |    9 ++
 Documentation/ndctl/ndctl-read-labels.txt |    7 ++
 configure.ac                              |    1 
 ndctl/dimm.c                              |   92 +++++++++++++++++--------
 ndctl/lib/dimm.c                          |   85 +++++++++++++++++++++--
 ndctl/lib/libndctl.c                      |  108 +++++++++++++++++++++++++----
 ndctl/lib/libndctl.sym                    |    9 ++
 ndctl/lib/private.h                       |    4 -
 ndctl/libndctl.h                          |    9 ++
 ndctl/namespace.c                         |   13 +++
 util/util.h                               |    4 +
 11 files changed, 286 insertions(+), 55 deletions(-)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v3 1/8] ndctl/build: Suppress -Waddress-of-packed-member

[Dan Williams]

gcc 9.1.1 emits a slew of warnings for many of the command field
accesses. I.e. warnings of the form:

libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member]
 2586 |  cmd->iter.offset = &cmd->get_data->in_offset;
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~

Suppress these as fixing the warning would defeat the abstraction of being able
to have common code that operates on commands with common fields at different
offsets in the payload.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 configure.ac |    1 +
 1 file changed, 1 insertion(+)

diff --git a/configure.ac b/configure.ac
index 4737cfff77f2..79f82977fa44 100644
--- a/configure.ac
+++ b/configure.ac
@@ -214,6 +214,7 @@ my_CFLAGS="\
 -Wmaybe-uninitialized \
 -Wdeclaration-after-statement \
 -Wunused-result \
+-Wno-address-of-packed-member \
 -D_FORTIFY_SOURCE=2 \
 -O2
 "

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

[ndctl PATCH v3 2/8] ndctl/dimm: Support small label reads/writes

[Dan Williams]

The initial ndctl label read/write implementation assumed that label
read / writes were relatively inexpensive, but that assumption is
invalid in practice. The process of reading a full label area can take
10s of seconds. Implement ndctl_cmd_cfg_{read,write}_set_extent() to
trim the label read/write commands to a range smaller than the full
label area.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/lib/libndctl.c   |  107 +++++++++++++++++++++++++++++++++++++++++-------
 ndctl/lib/libndctl.sym |    6 +++
 ndctl/lib/private.h    |    1 
 ndctl/libndctl.h       |    4 ++
 4 files changed, 103 insertions(+), 15 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 81e155171c8c..c1ecdd8c9c61 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2582,6 +2582,7 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_read(struct ndctl_cmd *cfg
 	cmd->get_data->in_offset = 0;
 	cmd->get_data->in_length = cfg_size->get_size->max_xfer;
 	cmd->firmware_status = &cmd->get_data->status;
+	cmd->iter.init_offset = 0;
 	cmd->iter.offset = &cmd->get_data->in_offset;
 	cmd->iter.xfer = &cmd->get_data->in_length;
 	cmd->iter.max_xfer = cfg_size->get_size->max_xfer;
@@ -2593,10 +2594,43 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_read(struct ndctl_cmd *cfg
 		free(cmd);
 		return NULL;
 	}
+	cmd->source = cfg_size;
+	ndctl_cmd_ref(cfg_size);
 
 	return cmd;
 }
 
+static void iter_set_extent(struct ndctl_cmd_iter *iter, unsigned int len,
+		unsigned int offset)
+{
+	iter->init_offset = offset;
+	*iter->offset = offset;
+	*iter->xfer = min(iter->max_xfer, len);
+	iter->total_xfer = len;
+}
+
+NDCTL_EXPORT int ndctl_cmd_cfg_read_set_extent(struct ndctl_cmd *cfg_read,
+		unsigned int len, unsigned int offset)
+{
+	struct ndctl_ctx *ctx = ndctl_bus_get_ctx(cmd_to_bus(cfg_read));
+	struct ndctl_cmd *cfg_size = cfg_read->source;
+
+	if (cfg_read->type != ND_CMD_GET_CONFIG_DATA
+			|| cfg_read->status <= 0) {
+		dbg(ctx, "expected unsubmitted cfg_read command\n");
+		return -EINVAL;
+	}
+
+	if (offset + len > cfg_size->get_size->config_size) {
+		dbg(ctx, "read %d from %d exceeds %d\n", len, offset,
+				cfg_size->get_size->config_size);
+		return -EINVAL;
+	}
+
+	iter_set_extent(&cfg_read->iter, len, offset);
+	return 0;
+}
+
 NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_write(struct ndctl_cmd *cfg_read)
 {
 	struct ndctl_ctx *ctx = ndctl_bus_get_ctx(cmd_to_bus(cfg_read));
@@ -2632,10 +2666,11 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_write(struct ndctl_cmd *cf
 	cmd->type = ND_CMD_SET_CONFIG_DATA;
 	cmd->size = size;
 	cmd->status = 1;
-	cmd->set_data->in_offset = 0;
+	cmd->set_data->in_offset = cfg_read->iter.init_offset;
 	cmd->set_data->in_length = cfg_read->iter.max_xfer;
 	cmd->firmware_status = (u32 *) (cmd->cmd_buf
 		+ sizeof(struct nd_cmd_set_config_hdr) + cfg_read->iter.max_xfer);
+	cmd->iter.init_offset = cfg_read->iter.init_offset;
 	cmd->iter.offset = &cmd->set_data->in_offset;
 	cmd->iter.xfer = &cmd->set_data->in_length;
 	cmd->iter.max_xfer = cfg_read->iter.max_xfer;
@@ -2657,18 +2692,33 @@ NDCTL_EXPORT unsigned int ndctl_cmd_cfg_size_get_size(struct ndctl_cmd *cfg_size
 	return 0;
 }
 
+static ssize_t iter_access(struct ndctl_cmd_iter *iter, unsigned int len,
+		unsigned int offset)
+{
+	if (offset < iter->init_offset
+			|| offset > iter->init_offset + iter->total_xfer
+			|| len + offset < len)
+		return -EINVAL;
+	if (len + offset > iter->init_offset + iter->total_xfer)
+		len = iter->total_xfer - offset;
+	return len;
+}
+
 NDCTL_EXPORT ssize_t ndctl_cmd_cfg_read_get_data(struct ndctl_cmd *cfg_read,
-		void *buf, unsigned int len, unsigned int offset)
+		void *buf, unsigned int _len, unsigned int offset)
 {
+	struct ndctl_cmd_iter *iter;
+	ssize_t len;
+
 	if (cfg_read->type != ND_CMD_GET_CONFIG_DATA || cfg_read->status > 0)
 		return -EINVAL;
 	if (cfg_read->status < 0)
 		return cfg_read->status;
-	if (offset > cfg_read->iter.total_xfer || len + offset < len)
-		return -EINVAL;
-	if (len + offset > cfg_read->iter.total_xfer)
-		len = cfg_read->iter.total_xfer - offset;
-	memcpy(buf, cfg_read->iter.total_buf + offset, len);
+
+	iter = &cfg_read->iter;
+	len = iter_access(&cfg_read->iter, _len, offset);
+	if (len >= 0)
+		memcpy(buf, iter->total_buf + offset, len);
 	return len;
 }
 
@@ -2681,29 +2731,56 @@ NDCTL_EXPORT ssize_t ndctl_cmd_cfg_read_get_size(struct ndctl_cmd *cfg_read)
 	return cfg_read->iter.total_xfer;
 }
 
+NDCTL_EXPORT int ndctl_cmd_cfg_write_set_extent(struct ndctl_cmd *cfg_write,
+		unsigned int len, unsigned int offset)
+{
+	struct ndctl_ctx *ctx = ndctl_bus_get_ctx(cmd_to_bus(cfg_write));
+	struct ndctl_cmd *cfg_size, *cfg_read;
+
+	if (cfg_write->type != ND_CMD_SET_CONFIG_DATA
+			|| cfg_write->status <= 0) {
+		dbg(ctx, "expected unsubmitted cfg_write command\n");
+		return -EINVAL;
+	}
+
+	cfg_read = cfg_write->source;
+	cfg_size = cfg_read->source;
+
+	if (offset + len > cfg_size->get_size->config_size) {
+		dbg(ctx, "write %d from %d exceeds %d\n", len, offset,
+				cfg_size->get_size->config_size);
+		return -EINVAL;
+	}
+
+	iter_set_extent(&cfg_write->iter, len, offset);
+	return 0;
+}
+
 NDCTL_EXPORT ssize_t ndctl_cmd_cfg_write_set_data(struct ndctl_cmd *cfg_write,
-		void *buf, unsigned int len, unsigned int offset)
+		void *buf, unsigned int _len, unsigned int offset)
 {
+	ssize_t len;
+
 	if (cfg_write->type != ND_CMD_SET_CONFIG_DATA || cfg_write->status < 1)
 		return -EINVAL;
 	if (cfg_write->status < 0)
 		return cfg_write->status;
-	if (offset > cfg_write->iter.total_xfer || len + offset < len)
-		return -EINVAL;
-	if (len + offset > cfg_write->iter.total_xfer)
-		len = cfg_write->iter.total_xfer - offset;
-	memcpy(cfg_write->iter.total_buf + offset, buf, len);
+	len = iter_access(&cfg_write->iter, _len, offset);
+	if (len >= 0)
+		memcpy(cfg_write->iter.total_buf + offset, buf, len);
 	return len;
 }
 
 NDCTL_EXPORT ssize_t ndctl_cmd_cfg_write_zero_data(struct ndctl_cmd *cfg_write)
 {
+	struct ndctl_cmd_iter *iter = &cfg_write->iter;
+
 	if (cfg_write->type != ND_CMD_SET_CONFIG_DATA || cfg_write->status < 1)
 		return -EINVAL;
 	if (cfg_write->status < 0)
 		return cfg_write->status;
-	memset(cfg_write->iter.total_buf, 0, cfg_write->iter.total_xfer);
-	return cfg_write->iter.total_xfer;
+	memset(iter->total_buf + iter->init_offset, 0, iter->total_xfer);
+	return iter->total_xfer;
 }
 
 NDCTL_EXPORT void ndctl_cmd_unref(struct ndctl_cmd *cmd)
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 297f03d7ae39..84359b97b793 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -410,3 +410,9 @@ LIBNDCTL_20 {
 global:
 	ndctl_bus_poll_scrub_completion;
 } LIBNDCTL_19;
+
+
+LIBNDCTL_21 {
+	ndctl_cmd_cfg_read_set_extent;
+	ndctl_cmd_cfg_write_set_extent;
+} LIBNDCTL_20;
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index 2ddc1d2c34f8..3fc0290ff6a7 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -254,6 +254,7 @@ struct ndctl_cmd {
 	int status;
 	u32 *firmware_status;
 	struct ndctl_cmd_iter {
+		u32 init_offset;
 		u32 *offset;
 		u32 *xfer; /* pointer to xfer length in cmd */
 		u8 *data; /* pointer to the data buffer location in cmd */
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index e378802ee4c1..310814fe924c 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -321,6 +321,10 @@ unsigned int ndctl_cmd_cfg_size_get_size(struct ndctl_cmd *cfg_size);
 ssize_t ndctl_cmd_cfg_read_get_data(struct ndctl_cmd *cfg_read, void *buf,
 		unsigned int len, unsigned int offset);
 ssize_t ndctl_cmd_cfg_read_get_size(struct ndctl_cmd *cfg_read);
+int ndctl_cmd_cfg_read_set_extent(struct ndctl_cmd *cfg_read,
+		unsigned int len, unsigned int offset);
+int ndctl_cmd_cfg_write_set_extent(struct ndctl_cmd *cfg_write,
+		unsigned int len, unsigned int offset);
 ssize_t ndctl_cmd_cfg_write_set_data(struct ndctl_cmd *cfg_write, void *buf,
 		unsigned int len, unsigned int offset);
 ssize_t ndctl_cmd_cfg_write_zero_data(struct ndctl_cmd *cfg_write);

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

[ndctl PATCH v3 3/8] ndctl/dimm: Minimize data-transfer for init-labels

[Dan Williams]

Currently init-labels implementation reads the entire namespace-label
capacity, initializes just the namespace index, and then writes the
entire label capacity. It turns out that DIMM label-area access methods
can be exceedingly slow.

For example, the time to read the entire label area on a single dimm:
2s, but the time to just read the index block space: 45ms.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/dimm.c           |    2 +-
 ndctl/lib/dimm.c       |   53 +++++++++++++++++++++++++++++++++++++++++++++---
 ndctl/lib/libndctl.sym |    1 +
 ndctl/libndctl.h       |    1 +
 4 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index db91f42421e4..d78e0dbc3ec6 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -982,7 +982,7 @@ static int __action_init(struct ndctl_dimm *dimm,
 	struct ndctl_cmd *cmd_read;
 	int rc;
 
-	cmd_read = ndctl_dimm_read_labels(dimm);
+	cmd_read = ndctl_dimm_read_label_index(dimm);
 	if (!cmd_read)
 		return -ENXIO;
 
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 22cf4e10b56c..9c5a34e542c3 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -370,14 +370,15 @@ static struct namespace_label *label_base(struct nvdimm_data *ndd)
 	return (struct namespace_label *) base;
 }
 
-static void init_ndd(struct nvdimm_data *ndd, struct ndctl_cmd *cmd_read)
+static void init_ndd(struct nvdimm_data *ndd, struct ndctl_cmd *cmd_read,
+		struct ndctl_cmd *cmd_size)
 {
 	ndctl_cmd_unref(ndd->cmd_read);
 	memset(ndd, 0, sizeof(*ndd));
 	ndd->cmd_read = cmd_read;
 	ndctl_cmd_ref(cmd_read);
 	ndd->data = cmd_read->iter.total_buf;
-	ndd->config_size = cmd_read->iter.total_xfer;
+	ndd->config_size = cmd_size->get_size->config_size;
 	ndd->ns_current = -1;
 	ndd->ns_next = -1;
 }
@@ -490,6 +491,52 @@ NDCTL_EXPORT int ndctl_dimm_validate_labels(struct ndctl_dimm *dimm)
 	return label_validate(&dimm->ndd);
 }
 
+NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_label_index(struct ndctl_dimm *dimm)
+{
+        struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
+        struct ndctl_cmd *cmd_size, *cmd_read;
+	struct nvdimm_data *ndd = &dimm->ndd;
+        int rc;
+
+        rc = ndctl_bus_wait_probe(bus);
+        if (rc < 0)
+                return NULL;
+
+        cmd_size = ndctl_dimm_cmd_new_cfg_size(dimm);
+        if (!cmd_size)
+                return NULL;
+        rc = ndctl_cmd_submit_xlat(cmd_size);
+        if (rc < 0)
+                goto out_size;
+
+        cmd_read = ndctl_dimm_cmd_new_cfg_read(cmd_size);
+        if (!cmd_read)
+                goto out_size;
+	/*
+	 * To calc the namespace index size use the minimum label
+	 * size which corresponds to the maximum namespace index size.
+	 */
+	init_ndd(ndd, cmd_read, cmd_size);
+	ndd->nslabel_size = 128;
+	rc = ndctl_cmd_cfg_read_set_extent(cmd_read,
+			sizeof_namespace_index(ndd) * 2, 0);
+	if (rc < 0)
+		goto out_size;
+
+        rc = ndctl_cmd_submit_xlat(cmd_read);
+        if (rc < 0)
+                goto out_read;
+	ndctl_cmd_unref(cmd_size);
+
+	return cmd_read;
+
+ out_read:
+        ndctl_cmd_unref(cmd_read);
+ out_size:
+        ndctl_cmd_unref(cmd_size);
+        return NULL;
+}
+
 NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
 {
         struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
@@ -515,7 +562,7 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
                 goto out_read;
 	ndctl_cmd_unref(cmd_size);
 
-	init_ndd(&dimm->ndd, cmd_read);
+	init_ndd(&dimm->ndd, cmd_read, cmd_size);
 
 	return cmd_read;
 
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 84359b97b793..648f83bced1b 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -415,4 +415,5 @@ global:
 LIBNDCTL_21 {
 	ndctl_cmd_cfg_read_set_extent;
 	ndctl_cmd_cfg_write_set_extent;
+	ndctl_dimm_read_label_index;
 } LIBNDCTL_20;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 310814fe924c..8aa4b8bbe6c2 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -308,6 +308,7 @@ struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_read(struct ndctl_cmd *cfg_size);
 struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_write(struct ndctl_cmd *cfg_read);
 int ndctl_dimm_zero_labels(struct ndctl_dimm *dimm);
 struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm);
+struct ndctl_cmd *ndctl_dimm_read_label_index(struct ndctl_dimm *dimm);
 int ndctl_dimm_validate_labels(struct ndctl_dimm *dimm);
 enum ndctl_namespace_version {
 	NDCTL_NS_VERSION_1_1,

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

[ndctl PATCH v3 4/8] ndctl/dimm: Add offset and size options to {read, write, zero}-labels

[Dan Williams]

Allow for more precision in label utilities, i.e. stop operating over
the entire label area.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ndctl/labels-options.txt |    9 ++++++
 ndctl/dimm.c                           |   49 ++++++++++++++++++++++++--------
 ndctl/lib/dimm.c                       |   36 ++++++++++++++++++++----
 ndctl/lib/libndctl.c                   |    1 +
 ndctl/lib/libndctl.sym                 |    2 +
 ndctl/lib/private.h                    |    3 --
 ndctl/libndctl.h                       |    4 +++
 util/util.h                            |    4 +++
 8 files changed, 87 insertions(+), 21 deletions(-)

diff --git a/Documentation/ndctl/labels-options.txt b/Documentation/ndctl/labels-options.txt
index 539ace079557..4aee37969fd5 100644
--- a/Documentation/ndctl/labels-options.txt
+++ b/Documentation/ndctl/labels-options.txt
@@ -5,6 +5,15 @@
 	operate on every dimm in the system, optionally filtered by bus id (see
         --bus= option).
 
+-s::
+--size=::
+	Limit the operation to the given number of bytes. A size of 0
+	indicates to operate over the entire label capacity.
+
+-O::
+--offset=::
+	Begin the operation at the given offset into the label area.
+
 -b::
 --bus=::
 	Limit operation to memory devices (dimms) that are on the given bus.
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index d78e0dbc3ec6..70128dd2df27 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -47,6 +47,8 @@ static struct parameters {
 	const char *infile;
 	const char *labelversion;
 	const char *kek;
+	unsigned len;
+	unsigned offset;
 	bool crypto_erase;
 	bool overwrite;
 	bool zero_key;
@@ -77,7 +79,7 @@ static int action_enable(struct ndctl_dimm *dimm, struct action_context *actx)
 
 static int action_zero(struct ndctl_dimm *dimm, struct action_context *actx)
 {
-	return ndctl_dimm_zero_labels(dimm);
+	return ndctl_dimm_zero_label_extent(dimm, param.len, param.offset);
 }
 
 static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
@@ -299,15 +301,17 @@ static struct json_object *dump_json(struct ndctl_dimm *dimm,
 	return NULL;
 }
 
-static int rw_bin(FILE *f, struct ndctl_cmd *cmd, ssize_t size, int rw)
+static int rw_bin(FILE *f, struct ndctl_cmd *cmd, ssize_t size,
+		unsigned int start_offset, int rw)
 {
 	char buf[4096];
 	ssize_t offset, write = 0;
 
-	for (offset = 0; offset < size; offset += sizeof(buf)) {
+	for (offset = start_offset; offset < start_offset + size;
+			offset += sizeof(buf)) {
 		ssize_t len = min_t(ssize_t, sizeof(buf), size - offset), rc;
 
-		if (rw) {
+		if (rw == WRITE) {
 			len = fread(buf, 1, len, f);
 			if (len == 0)
 				break;
@@ -343,9 +347,9 @@ static int action_write(struct ndctl_dimm *dimm, struct action_context *actx)
 		return -EBUSY;
 	}
 
-	cmd_read = ndctl_dimm_read_labels(dimm);
+	cmd_read = ndctl_dimm_read_label_extent(dimm, param.len, param.offset);
 	if (!cmd_read)
-		return -ENXIO;
+		return -EINVAL;
 
 	cmd_write = ndctl_dimm_cmd_new_cfg_write(cmd_read);
 	if (!cmd_write) {
@@ -354,7 +358,7 @@ static int action_write(struct ndctl_dimm *dimm, struct action_context *actx)
 	}
 
 	size = ndctl_cmd_cfg_read_get_size(cmd_read);
-	rc = rw_bin(actx->f_in, cmd_write, size, 1);
+	rc = rw_bin(actx->f_in, cmd_write, size, param.offset, WRITE);
 
 	/*
 	 * If the dimm is already disabled the kernel is not holding a cached
@@ -381,9 +385,9 @@ static int action_read(struct ndctl_dimm *dimm, struct action_context *actx)
 	ssize_t size;
 	int rc = 0;
 
-	cmd_read = ndctl_dimm_read_labels(dimm);
+	cmd_read = ndctl_dimm_read_label_extent(dimm, param.len, param.offset);
 	if (!cmd_read)
-		return -ENXIO;
+		return -EINVAL;
 
 	size = ndctl_cmd_cfg_read_get_size(cmd_read);
 	if (actx->jdimms) {
@@ -394,7 +398,7 @@ static int action_read(struct ndctl_dimm *dimm, struct action_context *actx)
 		else
 			rc = -ENOMEM;
 	} else
-		rc = rw_bin(actx->f_out, cmd_read, size, 0);
+		rc = rw_bin(actx->f_out, cmd_read, size, param.offset, READ);
 
 	ndctl_cmd_unref(cmd_read);
 
@@ -1082,18 +1086,31 @@ OPT_BOOLEAN('z', "zero-key", &param.zero_key, \
 OPT_BOOLEAN('m', "master-passphrase", &param.master_pass, \
 		"use master passphrase")
 
+#define LABEL_OPTIONS() \
+OPT_UINTEGER('s', "size", &param.len, "number of label bytes to operate"), \
+OPT_UINTEGER('O', "offset", &param.offset, \
+	"offset into the label area to start operation")
+
 static const struct option read_options[] = {
 	BASE_OPTIONS(),
+	LABEL_OPTIONS(),
 	READ_OPTIONS(),
 	OPT_END(),
 };
 
 static const struct option write_options[] = {
 	BASE_OPTIONS(),
+	LABEL_OPTIONS(),
 	WRITE_OPTIONS(),
 	OPT_END(),
 };
 
+static const struct option zero_options[] = {
+	BASE_OPTIONS(),
+	LABEL_OPTIONS(),
+	OPT_END(),
+};
+
 static const struct option update_options[] = {
 	BASE_OPTIONS(),
 	UPDATE_OPTIONS(),
@@ -1136,6 +1153,7 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 		NULL
 	};
 	unsigned long id;
+	bool json = false;
 
         argc = parse_options(argc, argv, options, u, 0);
 
@@ -1160,7 +1178,14 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 		return -EINVAL;
 	}
 
-	if (param.json || param.human) {
+	json = param.json || param.human;
+	if (action == action_read && json && (param.len || param.offset)) {
+		fprintf(stderr, "--size and --offset are incompatible with --json\n");
+		usage_with_options(u, options);
+		return -EINVAL;
+	}
+
+	if (json) {
 		actx.jdimms = json_object_new_array();
 		if (!actx.jdimms)
 			return -ENOMEM;
@@ -1294,7 +1319,7 @@ int cmd_read_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 
 int cmd_zero_labels(int argc, const char **argv, struct ndctl_ctx *ctx)
 {
-	int count = dimm_action(argc, argv, ctx, action_zero, base_options,
+	int count = dimm_action(argc, argv, ctx, action_zero, zero_options,
 			"ndctl zero-labels <nmem0> [<nmem1>..<nmemN>] [<options>]");
 
 	fprintf(stderr, "zeroed %d nmem%s\n", count >= 0 ? count : 0,
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 9c5a34e542c3..28b1dfb0bdfa 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -537,7 +537,8 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_label_index(struct ndctl_dimm *di
         return NULL;
 }
 
-NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
+NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_label_extent(
+		struct ndctl_dimm *dimm, unsigned int len, unsigned int offset)
 {
         struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
         struct ndctl_cmd *cmd_size, *cmd_read;
@@ -557,13 +558,25 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
         cmd_read = ndctl_dimm_cmd_new_cfg_read(cmd_size);
         if (!cmd_read)
                 goto out_size;
+
+	/*
+	 * For ndctl_read_labels() compat, enable subsequent calls that
+	 * will manipulate labels
+	 */
+	if (len == 0 && offset == 0)
+		init_ndd(&dimm->ndd, cmd_read, cmd_size);
+
+	if (len == 0)
+		len = cmd_size->get_size->config_size;
+	rc = ndctl_cmd_cfg_read_set_extent(cmd_read, len, offset);
+	if (rc < 0)
+		goto out_size;
+
         rc = ndctl_cmd_submit_xlat(cmd_read);
         if (rc < 0)
                 goto out_read;
 	ndctl_cmd_unref(cmd_size);
 
-	init_ndd(&dimm->ndd, cmd_read, cmd_size);
-
 	return cmd_read;
 
  out_read:
@@ -573,15 +586,21 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
         return NULL;
 }
 
-NDCTL_EXPORT int ndctl_dimm_zero_labels(struct ndctl_dimm *dimm)
+NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
+{
+	return ndctl_dimm_read_label_extent(dimm, 0, 0);
+}
+
+NDCTL_EXPORT int ndctl_dimm_zero_label_extent(struct ndctl_dimm *dimm,
+		unsigned int len, unsigned int offset)
 {
 	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
 	struct ndctl_cmd *cmd_read, *cmd_write;
 	int rc;
 
-	cmd_read = ndctl_dimm_read_labels(dimm);
+	cmd_read = ndctl_dimm_read_label_extent(dimm, len, offset);
 	if (!cmd_read)
-		return -ENXIO;
+		return -EINVAL;
 
 	if (ndctl_dimm_is_active(dimm)) {
 		dbg(ctx, "%s: regions active, abort label write\n",
@@ -623,6 +642,11 @@ NDCTL_EXPORT int ndctl_dimm_zero_labels(struct ndctl_dimm *dimm)
 	return rc;
 }
 
+NDCTL_EXPORT int ndctl_dimm_zero_labels(struct ndctl_dimm *dimm)
+{
+	return ndctl_dimm_zero_label_extent(dimm, 0, 0);
+}
+
 NDCTL_EXPORT unsigned long ndctl_dimm_get_available_labels(
 		struct ndctl_dimm *dimm)
 {
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index c1ecdd8c9c61..4d9cc7e29c6b 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -31,6 +31,7 @@
 #include <ccan/build_assert/build_assert.h>
 
 #include <ndctl.h>
+#include <util/util.h>
 #include <util/size.h>
 #include <util/sysfs.h>
 #include <ndctl/libndctl.h>
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 648f83bced1b..fef2907aa47d 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -416,4 +416,6 @@ LIBNDCTL_21 {
 	ndctl_cmd_cfg_read_set_extent;
 	ndctl_cmd_cfg_write_set_extent;
 	ndctl_dimm_read_label_index;
+	ndctl_dimm_read_label_extent;
+	ndctl_dimm_zero_label_extent;
 } LIBNDCTL_20;
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index 3fc0290ff6a7..1f6a01c55377 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -242,9 +242,6 @@ struct ndctl_namespace {
  *
  * A command may only specify one of @source, or @iter.total_buf, not both.
  */
-enum {
-	READ, WRITE,
-};
 struct ndctl_cmd {
 	struct ndctl_dimm *dimm;
 	struct ndctl_bus *bus;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 8aa4b8bbe6c2..c9d0dc120d3b 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -307,8 +307,12 @@ struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_size(struct ndctl_dimm *dimm);
 struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_read(struct ndctl_cmd *cfg_size);
 struct ndctl_cmd *ndctl_dimm_cmd_new_cfg_write(struct ndctl_cmd *cfg_read);
 int ndctl_dimm_zero_labels(struct ndctl_dimm *dimm);
+int ndctl_dimm_zero_label_extent(struct ndctl_dimm *dimm,
+		unsigned int len, unsigned int offset);
 struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm);
 struct ndctl_cmd *ndctl_dimm_read_label_index(struct ndctl_dimm *dimm);
+struct ndctl_cmd *ndctl_dimm_read_label_extent(struct ndctl_dimm *dimm,
+		unsigned int len, unsigned int offset);
 int ndctl_dimm_validate_labels(struct ndctl_dimm *dimm);
 enum ndctl_namespace_version {
 	NDCTL_NS_VERSION_1_1,
diff --git a/util/util.h b/util/util.h
index 001707e8b159..54c6ef18b6d7 100644
--- a/util/util.h
+++ b/util/util.h
@@ -73,6 +73,10 @@
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
 
+enum {
+	READ, WRITE,
+};
+
 static inline const char *skip_prefix(const char *str, const char *prefix)
 {
         size_t len = strlen(prefix);

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

[ndctl PATCH v3 5/8] ndctl/dimm: Limit read-labels with --index option

[Dan Williams]

Provide a capability to limit the read-labels payload to just the
index-block data space.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ndctl/ndctl-read-labels.txt |    7 +++++
 ndctl/dimm.c                              |   43 ++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/Documentation/ndctl/ndctl-read-labels.txt b/Documentation/ndctl/ndctl-read-labels.txt
index 756713ee12d7..b5ddae9c269e 100644
--- a/Documentation/ndctl/ndctl-read-labels.txt
+++ b/Documentation/ndctl/ndctl-read-labels.txt
@@ -19,6 +19,13 @@ file.  In the multi-dimm case the data is concatenated.
 OPTIONS
 -------
 include::labels-options.txt[]
+-I::
+--index::
+	Limit the span of the label operation to just the index-block
+	area. This is useful to determine if the dimm label area is
+	initialized. Note that this option and --size/--offset are
+	mutually exclusive.
+
 -o::
 --output::
 	output file
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 70128dd2df27..5e6fa19bab15 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -55,6 +55,7 @@ static struct parameters {
 	bool master_pass;
 	bool human;
 	bool force;
+	bool index;
 	bool json;
 	bool verbose;
 } param = {
@@ -276,27 +277,26 @@ static struct json_object *dump_json(struct ndctl_dimm *dimm,
 
 	if (!jdimm)
 		return NULL;
-	jindex = dump_index_json(cmd_read, size);
-	if (!jindex)
-		goto err_jindex;
-	jlabel = dump_label_json(dimm, cmd_read, size, flags);
-	if (!jlabel)
-		goto err_jlabel;
 
 	jobj = json_object_new_string(ndctl_dimm_get_devname(dimm));
 	if (!jobj)
-		goto err_jobj;
-
+		goto err;
 	json_object_object_add(jdimm, "dev", jobj);
+
+	jindex = dump_index_json(cmd_read, size);
+	if (!jindex)
+		goto err;
 	json_object_object_add(jdimm, "index", jindex);
+	if (param.index)
+		return jdimm;
+
+	jlabel = dump_label_json(dimm, cmd_read, size, flags);
+	if (!jlabel)
+		goto err;
 	json_object_object_add(jdimm, "label", jlabel);
-	return jdimm;
 
- err_jobj:
-	json_object_put(jlabel);
- err_jlabel:
-	json_object_put(jindex);
- err_jindex:
+	return jdimm;
+err:
 	json_object_put(jdimm);
 	return NULL;
 }
@@ -385,7 +385,11 @@ static int action_read(struct ndctl_dimm *dimm, struct action_context *actx)
 	ssize_t size;
 	int rc = 0;
 
-	cmd_read = ndctl_dimm_read_label_extent(dimm, param.len, param.offset);
+	if (param.index)
+		cmd_read = ndctl_dimm_read_label_index(dimm);
+	else
+		cmd_read = ndctl_dimm_read_label_extent(dimm, param.len,
+				param.offset);
 	if (!cmd_read)
 		return -EINVAL;
 
@@ -1054,7 +1058,8 @@ OPT_BOOLEAN('v',"verbose", &param.verbose, "turn on debug")
 OPT_STRING('o', "output", &param.outfile, "output-file", \
 	"filename to write label area contents"), \
 OPT_BOOLEAN('j', "json", &param.json, "parse label data into json"), \
-OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats (implies --json)")
+OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats (implies --json)"), \
+OPT_BOOLEAN('I', "index", &param.index, "limit read to the index block area")
 
 #define WRITE_OPTIONS() \
 OPT_STRING('i', "input", &param.infile, "input-file", \
@@ -1185,6 +1190,12 @@ static int dimm_action(int argc, const char **argv, struct ndctl_ctx *ctx,
 		return -EINVAL;
 	}
 
+	if (param.index && param.len) {
+		fprintf(stderr, "pick either --size, or --index, not both\n");
+		usage_with_options(u, options);
+		return -EINVAL;
+	}
+
 	if (json) {
 		actx.jdimms = json_object_new_array();
 		if (!actx.jdimms)

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

[ndctl PATCH v3 6/8] ndctl/namespace: Minimize label data transfer for autolabel

[Dan Williams]

Use the new ndctl_dimm_read_label_index() helper to minimize the amount
of label I/O needed to execute an autolabel event.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/namespace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index a654460ce4c5..26d03358c80d 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -997,7 +997,7 @@ retry:
 		int num_labels, avail;
 
 		ndctl_cmd_unref(cmd_read);
-		cmd_read = ndctl_dimm_read_labels(dimm);
+		cmd_read = ndctl_dimm_read_label_index(dimm);
 		if (!cmd_read)
 			continue;
 

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

[ndctl PATCH v3 7/8] ndctl/namespace: Continue region search on 'missing seed' event

[Dan Williams]

Consider a scenario where one region is in an error state but another is
not:

    # ndctl list -Ru
    [
      {
        "dev":"region3",
        "size":"127.00 GiB (136.37 GB)",
        "available_size":0,
        "max_available_extent":0,
        "type":"pmem",
        "persistence_domain":"unknown"
      },
      {
        "dev":"region2",
        "size":"127.00 GiB (136.37 GB)",
        "available_size":"127.00 GiB (136.37 GB)",
        "max_available_extent":"127.00 GiB (136.37 GB)",
        "type":"pmem",
        "iset_id":"0xba90120012b4dc",
        "persistence_domain":"unknown"
      }
    ]

    # ndctl create-namespace -m devdax -v
    [..]
    namespace_create:887: region3: no idle namespace seed
    failed to create namespace: No such device

Instead of failing when probing region3 for capacity, fallback to
region2.

    # ndctl create-namespace -m devdax
    {
      "dev":"namespace2.0",
      "mode":"devdax",
      "map":"dev",
      "size":"125.01 GiB (134.23 GB)",
      "uuid":"c3fa7d2f-6c20-4762-9aa8-627d06275e03",
      "daxregion":{
        "id":2,
        "size":"125.01 GiB (134.23 GB)",
        "align":2097152,
        "devices":[
          {
            "chardev":"dax2.0",
            "size":"125.01 GiB (134.23 GB)"
          }
        ]
      },
      "align":2097152
    }

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/namespace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 26d03358c80d..5c457224cb13 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -811,7 +811,7 @@ static int namespace_create(struct ndctl_region *region)
 	if (!ndns || is_namespace_active(ndns)) {
 		debug("%s: no %s namespace seed\n", devname,
 				ndns ? "idle" : "available");
-		return -ENODEV;
+		return -EAGAIN;
 	}
 
 	rc = setup_namespace(region, ndns, &p);

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

[ndctl PATCH v3 8/8] ndctl/namespace: Report ENOSPC when regions are full

[Dan Williams]

The create-namespace error message:

    failed to create namespace: Resource temporarily unavailable

...is misleading because the lack of capacity is permanent until the
user frees up space.

Trap EAGAIN and translate to ENOSPC in case the region capacity search
fails:

    failed to create namespace: No space left on device

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/namespace.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 5c457224cb13..89113f760bbf 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1415,6 +1415,15 @@ static int do_xaction_namespace(const char *namespace,
 		}
 	}
 
+	if (action == ACTION_CREATE && rc == -EAGAIN) {
+		/*
+		 * Namespace creation searched through all candidate
+		 * regions and all of them said "nope, I don't have
+		 * enough capacity", so report -ENOSPC
+		 */
+		rc = -ENOSPC;
+	}
+
 	return rc;
 }
 

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

Re: [ndctl PATCH v3 1/8] ndctl/build: Suppress -Waddress-of-packed-member

[Jeff Moyer]

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

> gcc 9.1.1 emits a slew of warnings for many of the command field
> accesses. I.e. warnings of the form:
>
> libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>  2586 |  cmd->iter.offset = &cmd->get_data->in_offset;
>       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> Suppress these as fixing the warning would defeat the abstraction of being able
> to have common code that operates on commands with common fields at different
> offsets in the payload.

As I understand it, taking a pointer to this potentially unaligned
member can result in bus errors (or worse, accessing wrong data) on
architectures that don't support unaligned accesses.  I'd be a whole lot
happier with this changelog if it mentioned that you had considered what
the warning actually meant, and decided it didn't matter for the
architectures you want to support.

x86 is, of course, safe.  I believe aarch64 is, as well.  I didn't look
into others.

Cheers,
Jeff


> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  configure.ac |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/configure.ac b/configure.ac
> index 4737cfff77f2..79f82977fa44 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -214,6 +214,7 @@ my_CFLAGS="\
>  -Wmaybe-uninitialized \
>  -Wdeclaration-after-statement \
>  -Wunused-result \
> +-Wno-address-of-packed-member \
>  -D_FORTIFY_SOURCE=2 \
>  -O2
>  "
>
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v3 1/8] ndctl/build: Suppress -Waddress-of-packed-member

[Dan Williams]

On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > gcc 9.1.1 emits a slew of warnings for many of the command field
> > accesses. I.e. warnings of the form:
> >
> > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member]
> >  2586 |  cmd->iter.offset = &cmd->get_data->in_offset;
> >       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Suppress these as fixing the warning would defeat the abstraction of being able
> > to have common code that operates on commands with common fields at different
> > offsets in the payload.
>
> As I understand it, taking a pointer to this potentially unaligned
> member can result in bus errors (or worse, accessing wrong data) on
> architectures that don't support unaligned accesses.  I'd be a whole lot
> happier with this changelog if it mentioned that you had considered what
> the warning actually meant, and decided it didn't matter for the
> architectures you want to support.

True, it was a fleeting thought, but not something I considered
fleshing out... I'll send a revision.

>
> x86 is, of course, safe.  I believe aarch64 is, as well.  I didn't look
> into others.
>

Keep in mind that this code is for interfacing with the ACPI DSM path.
If an unaligned-incapable architecture defined an NVDIMM command set
it is highly unlikely it would be ACPI based, or pick these
problematic command formats. I can add these notes to the changelog,
but the unfortunate definition of these payloads that require __packed
is something I hope other architectures avoid.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v3 1/8] ndctl/build: Suppress -Waddress-of-packed-member

[Jeff Moyer]

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

> On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > gcc 9.1.1 emits a slew of warnings for many of the command field
>> > accesses. I.e. warnings of the form:
>> >
>> > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>> >  2586 |  cmd->iter.offset = &cmd->get_data->in_offset;
>> >       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~
>> >
>> > Suppress these as fixing the warning would defeat the abstraction of being able
>> > to have common code that operates on commands with common fields at different
>> > offsets in the payload.
>>
>> As I understand it, taking a pointer to this potentially unaligned
>> member can result in bus errors (or worse, accessing wrong data) on
>> architectures that don't support unaligned accesses.  I'd be a whole lot
>> happier with this changelog if it mentioned that you had considered what
>> the warning actually meant, and decided it didn't matter for the
>> architectures you want to support.
>
> True, it was a fleeting thought, but not something I considered
> fleshing out... I'll send a revision.

Thanks.

>> x86 is, of course, safe.  I believe aarch64 is, as well.  I didn't look
>> into others.
>>
> Keep in mind that this code is for interfacing with the ACPI DSM path.
> If an unaligned-incapable architecture defined an NVDIMM command set
> it is highly unlikely it would be ACPI based, or pick these
> problematic command formats. I can add these notes to the changelog,
> but the unfortunate definition of these payloads that require __packed
> is something I hope other architectures avoid.

We can hope...  :) Anyway, thanks for the additional context.

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

Re: [ndctl PATCH v3 1/8] ndctl/build: Suppress -Waddress-of-packed-member

[Dan Williams]

On Mon, Aug 5, 2019 at 10:34 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote:
> >
> > Dan Williams <dan.j.williams@intel.com> writes:
> >
> > > gcc 9.1.1 emits a slew of warnings for many of the command field
> > > accesses. I.e. warnings of the form:
> > >
> > > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member]
> > >  2586 |  cmd->iter.offset = &cmd->get_data->in_offset;
> > >       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Suppress these as fixing the warning would defeat the abstraction of being able
> > > to have common code that operates on commands with common fields at different
> > > offsets in the payload.
> >
> > As I understand it, taking a pointer to this potentially unaligned
> > member can result in bus errors (or worse, accessing wrong data) on
> > architectures that don't support unaligned accesses.  I'd be a whole lot
> > happier with this changelog if it mentioned that you had considered what
> > the warning actually meant, and decided it didn't matter for the
> > architectures you want to support.
>
> True, it was a fleeting thought, but not something I considered
> fleshing out... I'll send a revision.
>
> >
> > x86 is, of course, safe.  I believe aarch64 is, as well.  I didn't look
> > into others.
> >
>
> Keep in mind that this code is for interfacing with the ACPI DSM path.
> If an unaligned-incapable architecture defined an NVDIMM command set
> it is highly unlikely it would be ACPI based, or pick these
> problematic command formats. I can add these notes to the changelog,
> but the unfortunate definition of these payloads that require __packed
> is something I hope other architectures avoid.

...and it turns out this is wrong because we have the default ioctls
that also use these packed structures

        ND_CMD_ARS_CAP = 1,
        ND_CMD_ARS_START = 2,
        ND_CMD_ARS_STATUS = 3,
        ND_CMD_CLEAR_ERROR = 4,

        ND_CMD_SMART = 1,
        ND_CMD_SMART_THRESHOLD = 2,
        ND_CMD_DIMM_FLAGS = 3,
        ND_CMD_GET_CONFIG_SIZE = 4,
        ND_CMD_GET_CONFIG_DATA = 5,
        ND_CMD_SET_CONFIG_DATA = 6,
        ND_CMD_VENDOR = 9,

I'll take a look at reworking this.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm