[PATCH v2 1/4] ndctl: add more error outs to update firmware and enable verbose debug

[PATCH v2 1/4] ndctl: add more error outs to update firmware and enable verbose debug

[Dave Jiang]

update-firmware is missing verbose option for debug outputs. Also adding
additional error outs to give better indication if something has failed
and why.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/update.c |   99 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 30 deletions(-)

diff --git a/ndctl/update.c b/ndctl/update.c
index 877d37f..fc26acf 100644
--- a/ndctl/update.c
+++ b/ndctl/update.c
@@ -104,7 +104,7 @@ static int verify_fw_size(struct update_context *uctx)
 	struct fw_info *fw = &uctx->dimm_fw;
 
 	if (uctx->fw_size > fw->store_size) {
-		error("Firmware file size greater than DIMM store\n");
+		error("Firmware file size greater than DIMM store");
 		return -ENOSPC;
 	}
 
@@ -119,16 +119,20 @@ static int submit_get_firmware_info(struct update_context *uctx)
 	struct fw_info *fw = &uctx->dimm_fw;
 
 	cmd = ndctl_dimm_cmd_new_fw_get_info(uctx->dimm);
-	if (!cmd)
+	if (!cmd) {
+		error("%s: Fail to get new cmd", __func__);
 		return -ENXIO;
+	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc < 0)
+	if (rc < 0) {
+		error("%s: Fail to submit cmd", __func__);
 		return rc;
+	}
 
 	status = ndctl_cmd_fw_xlat_firmware_status(cmd);
 	if (status != FW_SUCCESS) {
-		error("GET FIRMWARE INFO failed: %#x\n", status);
+		error("GET FIRMWARE INFO failed: %#x", status);
 		return -ENXIO;
 	}
 
@@ -165,18 +169,22 @@ static int submit_start_firmware_upload(struct update_context *uctx)
 	struct fw_info *fw = &uctx->dimm_fw;
 
 	cmd = ndctl_dimm_cmd_new_fw_start_update(uctx->dimm);
-	if (!cmd)
+	if (!cmd) {
+		error("%s: Fail to get new cmd", __func__);
 		return -ENXIO;
+	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc < 0)
+	if (rc < 0) {
+		error("%s: Fail to submit cmd", __func__);
 		return rc;
+	}
 
 	status = ndctl_cmd_fw_xlat_firmware_status(cmd);
 	if (status != FW_SUCCESS) {
-		error("START FIRMWARE UPDATE failed: %#x\n", status);
+		error("START FIRMWARE UPDATE failed: %#x", status);
 		if (status == FW_EBUSY)
-			error("Another firmware upload in progress or finished.\n");
+			error("Another firmware upload in progress or finished.");
 		return -ENXIO;
 	}
 
@@ -196,8 +204,11 @@ static int get_fw_data_from_file(int fd, void *buf, uint32_t len,
 
 	while (len) {
 		rc = pread(fd, buf, len, offset);
-		if (rc < 0)
-			return -errno;
+		if (rc < 0) {
+			rc = -errno;
+			perror("pread");
+			return rc;
+		}
 		len -= rc;
 	}
 
@@ -241,7 +252,7 @@ static int send_firmware(struct update_context *uctx)
 
 		status = ndctl_cmd_fw_xlat_firmware_status(cmd);
 		if (status != FW_SUCCESS) {
-			error("SEND FIRMWARE failed: %#x\n", status);
+			error("SEND FIRMWARE failed: %#x", status);
 			rc = -ENXIO;
 			goto cleanup;
 		}
@@ -267,16 +278,20 @@ static int submit_finish_firmware(struct update_context *uctx)
 	enum ND_FW_STATUS status;
 
 	cmd = ndctl_dimm_cmd_new_fw_finish(uctx->start);
-	if (!cmd)
+	if (!cmd) {
+		error("%s: Fail to get new cmd", __func__);
 		return -ENXIO;
+	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc < 0)
+	if (rc < 0) {
+		error("%s: Fail to submit cmd", __func__);
 		goto out;
+	}
 
 	status = ndctl_cmd_fw_xlat_firmware_status(cmd);
 	if (status != FW_SUCCESS) {
-		error("FINISH FIRMWARE UPDATE failed: %#x\n", status);
+		error("FINISH FIRMWARE UPDATE failed: %#x", status);
 		rc = -ENXIO;
 		goto out;
 	}
@@ -302,7 +317,7 @@ static int submit_abort_firmware(struct update_context *uctx)
 
 	status = ndctl_cmd_fw_xlat_firmware_status(cmd);
 	if (!(status & ND_CMD_STATUS_FIN_ABORTED)) {
-		error("FW update abort failed: %#x\n", status);
+		error("FW update abort failed: %#x", status);
 		rc = -ENXIO;
 		goto out;
 	}
@@ -412,36 +427,42 @@ static int update_firmware(struct update_context *uctx)
 	int rc;
 
 	rc = submit_get_firmware_info(uctx);
-	if (rc < 0)
+	if (rc < 0) {
+		error("Unable to get firmware info");
 		return rc;
+	}
 
 	rc = submit_start_firmware_upload(uctx);
-	if (rc < 0)
+	if (rc < 0) {
+		error("Unable to start firmware upload");
 		return rc;
+	}
 
 	printf("Uploading %s to DIMM %s\n", uctx->fw_path, uctx->dimm_id);
 
 	rc = send_firmware(uctx);
 	if (rc < 0) {
-		error("Firmware send failed. Aborting...\n");
+		error("Firmware send failed. Aborting...");
 		rc = submit_abort_firmware(uctx);
 		if (rc < 0)
-			error("Aborting update sequence failed\n");
+			error("Aborting update sequence failed");
 		return rc;
 	}
 
 	rc = submit_finish_firmware(uctx);
 	if (rc < 0) {
-		error("Unable to end update sequence\n");
+		error("Unable to end update sequence");
 		rc = submit_abort_firmware(uctx);
 		if (rc < 0)
-			error("Aborting update sequence failed\n");
+			error("Aborting update sequence failed");
 		return rc;
 	}
 
 	rc = query_fw_finish_status(uctx);
-	if (rc < 0)
+	if (rc < 0) {
+		error("Query Firmware Finish Status failed");
 		return rc;
+	}
 
 	return 0;
 }
@@ -468,27 +489,34 @@ static int verify_fw_file(struct update_context *uctx)
 	int rc;
 
 	uctx->fw_fd = open(uctx->fw_path, O_RDONLY);
-	if (uctx->fw_fd < 0)
-		return -errno;
+	if (uctx->fw_fd < 0) {
+		rc = -errno;
+		error("Unable to open file %s: %d", uctx->fw_path, rc);
+		return rc;
+	}
 
 	rc = flock(uctx->fw_fd, LOCK_EX | LOCK_NB);
 	if (rc < 0) {
+		error("Unable to flock() file %s: %d", uctx->fw_path, rc);
 		rc = -errno;
 		goto cleanup;
 	}
 
 	if (fstat(uctx->fw_fd, &st) < 0) {
+		error("Unable to fstat() file %s: %d", uctx->fw_path, rc);
 		rc = -errno;
 		goto cleanup;
 	}
 
 	if (!S_ISREG(st.st_mode)) {
+		error("%s is not a regular file", uctx->fw_path);
 		rc = -EINVAL;
 		goto cleanup;
 	}
 
 	uctx->fw_size = st.st_size;
 	if (uctx->fw_size == 0) {
+		error("%s has file size of 0", uctx->fw_path);
 		rc = -EINVAL;
 		goto cleanup;
 	}
@@ -502,12 +530,14 @@ cleanup:
 
 int cmd_update_firmware(int argc, const char **argv, void *ctx)
 {
+	bool verbose;
 	struct update_context uctx = { 0 };
 	const struct option options[] = {
 		OPT_STRING('f', "firmware", &uctx.fw_path,
 				"file-name", "name of firmware"),
 		OPT_STRING('d', "dimm", &uctx.dimm_id, "dimm-id",
 				"dimm to be updated"),
+		OPT_BOOLEAN('v', "verbose", &verbose, "emit extra debug messages to stderr"),
 		OPT_END(),
 	};
 	const char * const u[] = {
@@ -518,33 +548,42 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
 
 	argc = parse_options(argc, argv, options, u, 0);
 	for (i = 0; i < argc; i++)
-		error("unknown parameter \"%s\"\n", argv[i]);
+		error("unknown parameter \"%s\"", argv[i]);
 	if (argc)
 		usage_with_options(u, options);
 
+	if (verbose)
+		ndctl_set_log_priority(ctx, LOG_DEBUG);
+
 	if (!uctx.fw_path) {
-		error("No firmware file provided\n");
+		error("No firmware file provided");
 		usage_with_options(u, options);
 		return -EINVAL;
 	}
 
 	if (!uctx.dimm_id) {
-		error("No DIMM ID provided\n");
+		error("No DIMM ID provided");
 		usage_with_options(u, options);
 		return -EINVAL;
 	}
 
 	rc = verify_fw_file(&uctx);
-	if (rc < 0)
+	if (rc < 0) {
+		error("Failed to verify firmware file %s", uctx.fw_path);
 		return rc;
+	}
 
 	rc = get_ndctl_dimm(&uctx, ctx);
-	if (rc < 0)
+	if (rc < 0) {
+		error("DIMM %s not found", uctx.dimm_id);
 		return rc;
+	}
 
 	rc = update_firmware(&uctx);
-	if (rc < 0)
+	if (rc < 0) {
+		error("Update firmware failed");
 		return rc;
+	}
 
 	if (uctx.start)
 		ndctl_cmd_unref(uctx.start);

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

[PATCH v2 2/4] ndctl: add "all" dimm-id option for update-firmware

[Dave Jiang]

Modifying the code so that when "all" option is passed in, we will process
all the DIMMs on the platform.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/update.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ndctl/update.c b/ndctl/update.c
index fc26acf..4fb572d 100644
--- a/ndctl/update.c
+++ b/ndctl/update.c
@@ -471,16 +471,22 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx)
 {
 	struct ndctl_dimm *dimm;
 	struct ndctl_bus *bus;
+	int rc = -ENODEV;
 
 	ndctl_bus_foreach(ctx, bus)
 		ndctl_dimm_foreach(bus, dimm) {
 			if (!util_dimm_filter(dimm, uctx->dimm_id))
 				continue;
 			uctx->dimm = dimm;
-			return 0;
+			rc = update_firmware(uctx);
+			if (rc < 0) {
+				error("Update firmware for dimm %s failed\n",
+						ndctl_dimm_get_devname(dimm));
+				continue;
+			}
 		}
 
-	return -ENODEV;
+	return rc;
 }
 
 static int verify_fw_file(struct update_context *uctx)
@@ -579,12 +585,6 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
 		return rc;
 	}
 
-	rc = update_firmware(&uctx);
-	if (rc < 0) {
-		error("Update firmware failed");
-		return rc;
-	}
-
 	if (uctx.start)
 		ndctl_cmd_unref(uctx.start);
 

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

Re: [PATCH v2 2/4] ndctl: add "all" dimm-id option for update-firmware

[Dan Williams]

On Thu, Feb 22, 2018 at 12:49 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> Modifying the code so that when "all" option is passed in, we will process
> all the DIMMs on the platform.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/update.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/ndctl/update.c b/ndctl/update.c
> index fc26acf..4fb572d 100644
> --- a/ndctl/update.c
> +++ b/ndctl/update.c
> @@ -471,16 +471,22 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx)
>  {
>         struct ndctl_dimm *dimm;
>         struct ndctl_bus *bus;
> +       int rc = -ENODEV;
>
>         ndctl_bus_foreach(ctx, bus)
>                 ndctl_dimm_foreach(bus, dimm) {
>                         if (!util_dimm_filter(dimm, uctx->dimm_id))
>                                 continue;
>                         uctx->dimm = dimm;
> -                       return 0;
> +                       rc = update_firmware(uctx);
> +                       if (rc < 0) {
> +                               error("Update firmware for dimm %s failed\n",
> +                                               ndctl_dimm_get_devname(dimm));
> +                               continue;
> +                       }

This really wants the -b option to available as well, and we would get
that basically for free by moving all of this functionality into
ndctl/dimm.c along with all the other DIMM specific commands. So we
can apply this for now, but update.c really does not deserve to be
standalone command infrastructure away from the existing dimm-specific
utility infrastructure.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 3/4] ndctl: add check for update firmware supported

[Dave Jiang]

Adding generic and intel support function to allow check if update firmware
is supported by the kernel.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/lib/firmware.c   |   11 +++++++++++
 ndctl/lib/intel.c      |   24 ++++++++++++++++++++++++
 ndctl/lib/libndctl.sym |    1 +
 ndctl/lib/private.h    |    1 +
 ndctl/libndctl.h       |    1 +
 ndctl/update.c         |    7 ++++++-
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/ndctl/lib/firmware.c b/ndctl/lib/firmware.c
index f6deec5..78d753c 100644
--- a/ndctl/lib/firmware.c
+++ b/ndctl/lib/firmware.c
@@ -107,3 +107,14 @@ ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd)
 	else
 		return FW_EUNKNOWN;
 }
+
+NDCTL_EXPORT bool
+ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm)
+{
+	struct ndctl_dimm_ops *ops = dimm->ops;
+
+	if (ops && ops->fw_update_supported)
+		return ops->fw_update_supported(dimm);
+	else
+		return false;
+}
diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
index cee5204..7d976c5 100644
--- a/ndctl/lib/intel.c
+++ b/ndctl/lib/intel.c
@@ -650,6 +650,29 @@ intel_dimm_cmd_new_lss(struct ndctl_dimm *dimm)
 	return cmd;
 }
 
+static bool intel_dimm_fw_update_supported(struct ndctl_dimm *dimm)
+{
+	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
+
+	if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) {
+		dbg(ctx, "unsupported cmd: %d\n", ND_CMD_CALL);
+		return false;
+	}
+
+	/*
+	 * We only need to check FW_GET_INFO. If that isn't supported then
+	 * the others aren't either.
+	 */
+	if (test_dimm_dsm(dimm, ND_INTEL_FW_GET_INFO)
+			== DIMM_DSM_UNSUPPORTED) {
+		dbg(ctx, "unsupported function: %d\n",
+				ND_INTEL_FW_GET_INFO);
+		return false;
+	}
+
+	return true;
+}
+
 struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) {
 	.cmd_desc = intel_cmd_desc,
 	.new_smart = intel_dimm_cmd_new_smart,
@@ -703,4 +726,5 @@ struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) {
 	.fw_fquery_get_fw_rev = intel_cmd_fw_fquery_get_fw_rev,
 	.fw_xlat_firmware_status = intel_cmd_fw_xlat_firmware_status,
 	.new_ack_shutdown_count = intel_dimm_cmd_new_lss,
+	.fw_update_supported = intel_dimm_fw_update_supported,
 };
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index af9b7d5..cc580f9 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -344,4 +344,5 @@ global:
 	ndctl_cmd_fw_fquery_get_fw_rev;
 	ndctl_cmd_fw_xlat_firmware_status;
 	ndctl_dimm_cmd_new_ack_shutdown_count;
+	ndctl_dimm_fw_update_supported;
 } LIBNDCTL_13;
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index 015eeb2..ae4454c 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -325,6 +325,7 @@ struct ndctl_dimm_ops {
 	unsigned long long (*fw_fquery_get_fw_rev)(struct ndctl_cmd *);
 	enum ND_FW_STATUS (*fw_xlat_firmware_status)(struct ndctl_cmd *);
 	struct ndctl_cmd *(*new_ack_shutdown_count)(struct ndctl_dimm *);
+	bool (*fw_update_supported)(struct ndctl_dimm *);
 };
 
 struct ndctl_dimm_ops * const intel_dimm_ops;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 9db775b..08030d3 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -625,6 +625,7 @@ unsigned int ndctl_cmd_fw_start_get_context(struct ndctl_cmd *cmd);
 unsigned long long ndctl_cmd_fw_fquery_get_fw_rev(struct ndctl_cmd *cmd);
 enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd);
 struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm);
+bool ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
 
 #ifdef __cplusplus
 } /* extern "C" */
diff --git a/ndctl/update.c b/ndctl/update.c
index 4fb572d..b148b70 100644
--- a/ndctl/update.c
+++ b/ndctl/update.c
@@ -477,6 +477,10 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx)
 		ndctl_dimm_foreach(bus, dimm) {
 			if (!util_dimm_filter(dimm, uctx->dimm_id))
 				continue;
+			if (!ndctl_dimm_fw_update_supported(dimm)) {
+				error("DIMM firmware update not supported by the kernel.");
+				return -ENOTSUP;
+			}
 			uctx->dimm = dimm;
 			rc = update_firmware(uctx);
 			if (rc < 0) {
@@ -581,7 +585,8 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
 
 	rc = get_ndctl_dimm(&uctx, ctx);
 	if (rc < 0) {
-		error("DIMM %s not found", uctx.dimm_id);
+		if (rc == -ENODEV)
+			error("DIMM %s not found", uctx.dimm_id);
 		return rc;
 	}
 

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

Re: [PATCH v2 3/4] ndctl: add check for update firmware supported

[Dan Williams]

On Thu, Feb 22, 2018 at 12:49 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> Adding generic and intel support function to allow check if update firmware
> is supported by the kernel.
>

Looks good, just one user message I'll fix up on applying...

> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/lib/firmware.c   |   11 +++++++++++
>  ndctl/lib/intel.c      |   24 ++++++++++++++++++++++++
>  ndctl/lib/libndctl.sym |    1 +
>  ndctl/lib/private.h    |    1 +
>  ndctl/libndctl.h       |    1 +
>  ndctl/update.c         |    7 ++++++-
>  6 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/ndctl/lib/firmware.c b/ndctl/lib/firmware.c
> index f6deec5..78d753c 100644
> --- a/ndctl/lib/firmware.c
> +++ b/ndctl/lib/firmware.c
> @@ -107,3 +107,14 @@ ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd)
>         else
>                 return FW_EUNKNOWN;
>  }
> +
> +NDCTL_EXPORT bool
> +ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm)
> +{
> +       struct ndctl_dimm_ops *ops = dimm->ops;
> +
> +       if (ops && ops->fw_update_supported)
> +               return ops->fw_update_supported(dimm);
> +       else
> +               return false;
> +}
> diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
> index cee5204..7d976c5 100644
> --- a/ndctl/lib/intel.c
> +++ b/ndctl/lib/intel.c
> @@ -650,6 +650,29 @@ intel_dimm_cmd_new_lss(struct ndctl_dimm *dimm)
>         return cmd;
>  }
>
> +static bool intel_dimm_fw_update_supported(struct ndctl_dimm *dimm)
> +{
> +       struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
> +
> +       if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) {
> +               dbg(ctx, "unsupported cmd: %d\n", ND_CMD_CALL);
> +               return false;
> +       }
> +
> +       /*
> +        * We only need to check FW_GET_INFO. If that isn't supported then
> +        * the others aren't either.
> +        */
> +       if (test_dimm_dsm(dimm, ND_INTEL_FW_GET_INFO)
> +                       == DIMM_DSM_UNSUPPORTED) {
> +               dbg(ctx, "unsupported function: %d\n",
> +                               ND_INTEL_FW_GET_INFO);
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) {
>         .cmd_desc = intel_cmd_desc,
>         .new_smart = intel_dimm_cmd_new_smart,
> @@ -703,4 +726,5 @@ struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) {
>         .fw_fquery_get_fw_rev = intel_cmd_fw_fquery_get_fw_rev,
>         .fw_xlat_firmware_status = intel_cmd_fw_xlat_firmware_status,
>         .new_ack_shutdown_count = intel_dimm_cmd_new_lss,
> +       .fw_update_supported = intel_dimm_fw_update_supported,
>  };
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index af9b7d5..cc580f9 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -344,4 +344,5 @@ global:
>         ndctl_cmd_fw_fquery_get_fw_rev;
>         ndctl_cmd_fw_xlat_firmware_status;
>         ndctl_dimm_cmd_new_ack_shutdown_count;
> +       ndctl_dimm_fw_update_supported;
>  } LIBNDCTL_13;
> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> index 015eeb2..ae4454c 100644
> --- a/ndctl/lib/private.h
> +++ b/ndctl/lib/private.h
> @@ -325,6 +325,7 @@ struct ndctl_dimm_ops {
>         unsigned long long (*fw_fquery_get_fw_rev)(struct ndctl_cmd *);
>         enum ND_FW_STATUS (*fw_xlat_firmware_status)(struct ndctl_cmd *);
>         struct ndctl_cmd *(*new_ack_shutdown_count)(struct ndctl_dimm *);
> +       bool (*fw_update_supported)(struct ndctl_dimm *);
>  };
>
>  struct ndctl_dimm_ops * const intel_dimm_ops;
> diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> index 9db775b..08030d3 100644
> --- a/ndctl/libndctl.h
> +++ b/ndctl/libndctl.h
> @@ -625,6 +625,7 @@ unsigned int ndctl_cmd_fw_start_get_context(struct ndctl_cmd *cmd);
>  unsigned long long ndctl_cmd_fw_fquery_get_fw_rev(struct ndctl_cmd *cmd);
>  enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd);
>  struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm);
> +bool ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
>
>  #ifdef __cplusplus
>  } /* extern "C" */
> diff --git a/ndctl/update.c b/ndctl/update.c
> index 4fb572d..b148b70 100644
> --- a/ndctl/update.c
> +++ b/ndctl/update.c
> @@ -477,6 +477,10 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx)
>                 ndctl_dimm_foreach(bus, dimm) {
>                         if (!util_dimm_filter(dimm, uctx->dimm_id))
>                                 continue;
> +                       if (!ndctl_dimm_fw_update_supported(dimm)) {
> +                               error("DIMM firmware update not supported by the kernel.");

Let's changes this message to:

    "%s: firmware update not supported.", ndctl_dimm_get_devname(dimm)

You don't have enough information to tell if it's the 'kernel' or the
'BIOS', so just say 'not supported'. 'DIMM' is ambiguous, the kernel
device name is not.

> +                               return -ENOTSUP;
> +                       }
>                         uctx->dimm = dimm;
>                         rc = update_firmware(uctx);
>                         if (rc < 0) {
> @@ -581,7 +585,8 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
>
>         rc = get_ndctl_dimm(&uctx, ctx);
>         if (rc < 0) {
> -               error("DIMM %s not found", uctx.dimm_id);
> +               if (rc == -ENODEV)
> +                       error("DIMM %s not found", uctx.dimm_id);

When we move this in with the other dimm functionality in ndctl/dimm.c
we should follow the same message convention there and use lowercase
'dimm' and the '"nmem%d", id' format when emitting messages for the
user.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 4/4] ndctl: accept DIMM name without -d option

[Dave Jiang]

Making update-firmware in sync with other ndctl syntax and accept DIMM
id without using a switch. The -d option will still be accepted. Now
it will accept multiple dimm ids as well as the "all" option.
i.e.
ndctl update-firmware -f file.bin nmem0 nmem1

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/update.c |  119 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 78 insertions(+), 41 deletions(-)

diff --git a/Documentation/ndctl/ndctl-update-firmware.txt b/Documentation/ndctl/ndctl-update-firmware.txt
index d742302..6d74c11 100644
--- a/Documentation/ndctl/ndctl-update-firmware.txt
+++ b/Documentation/ndctl/ndctl-update-firmware.txt
@@ -8,7 +8,18 @@ ndctl-update-firmware - provides updating of NVDIMM firmware
 SYNOPSIS
 --------
 [verse]
-'ndctl update-firmware' -f <firmware_file> -d <dimm name>
+'ndctl update-firmware' <nmem0> [<nmem1>..<nmemN>] [<options>]
+
+This command updates the persistent DIMM's firmware.
+
+OPTIONS
+-------
+-f::
+--firmware::
+	firmware file to be updated.
+-v::
+--verbose::
+	turn on verbose/debug option
 
 COPYRIGHT
 ---------
diff --git a/ndctl/update.c b/ndctl/update.c
index b148b70..acf2a64 100644
--- a/ndctl/update.c
+++ b/ndctl/update.c
@@ -467,32 +467,6 @@ static int update_firmware(struct update_context *uctx)
 	return 0;
 }
 
-static int get_ndctl_dimm(struct update_context *uctx, void *ctx)
-{
-	struct ndctl_dimm *dimm;
-	struct ndctl_bus *bus;
-	int rc = -ENODEV;
-
-	ndctl_bus_foreach(ctx, bus)
-		ndctl_dimm_foreach(bus, dimm) {
-			if (!util_dimm_filter(dimm, uctx->dimm_id))
-				continue;
-			if (!ndctl_dimm_fw_update_supported(dimm)) {
-				error("DIMM firmware update not supported by the kernel.");
-				return -ENOTSUP;
-			}
-			uctx->dimm = dimm;
-			rc = update_firmware(uctx);
-			if (rc < 0) {
-				error("Update firmware for dimm %s failed\n",
-						ndctl_dimm_get_devname(dimm));
-				continue;
-			}
-		}
-
-	return rc;
-}
-
 static int verify_fw_file(struct update_context *uctx)
 {
 	struct stat st;
@@ -538,6 +512,29 @@ cleanup:
 	return rc;
 }
 
+static int process_dimm(struct update_context *uctx, const char *dimm_id,
+		struct ndctl_dimm *dimm)
+{
+	int rc;
+
+	if (!util_dimm_filter(dimm, dimm_id))
+		return -ENODEV;
+	if (!ndctl_dimm_fw_update_supported(dimm)) {
+		error("DIMM firmware update not supported by the kernel.");
+		return -ENOTSUP;
+	}
+
+	uctx->dimm = dimm;
+	rc = update_firmware(uctx);
+	if (rc < 0) {
+		error("Update firmware for dimm %s failed\n",
+				ndctl_dimm_get_devname(dimm));
+		return rc;
+	}
+
+	return -ENODEV;
+}
+
 int cmd_update_firmware(int argc, const char **argv, void *ctx)
 {
 	bool verbose;
@@ -554,13 +551,34 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
 		"ndctl update_firmware [<options>]",
 		NULL
 	};
-	int i, rc;
+	int i, rc, err = 0;
+	unsigned long id;
 
 	argc = parse_options(argc, argv, options, u, 0);
-	for (i = 0; i < argc; i++)
-		error("unknown parameter \"%s\"", argv[i]);
-	if (argc)
+
+	if (argc == 0 && !uctx.dimm_id)
 		usage_with_options(u, options);
+	if (!uctx.dimm_id) {
+		for (i = 0; i < argc; i++) {
+			if (strcmp(argv[i], "all") == 0) {
+				argv[0] = "all";
+				argc = 1;
+				break;
+			}
+
+			if (sscanf(argv[i], "nmem%lu", &id) != 1) {
+				fprintf(stderr,
+					"'%s' is not a valid dimm name\n",
+					argv[i]);
+				err++;
+			}
+		}
+
+		if (err == argc) {
+			usage_with_options(u, options);
+			return -EINVAL;
+		}
+	}
 
 	if (verbose)
 		ndctl_set_log_priority(ctx, LOG_DEBUG);
@@ -571,25 +589,44 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
 		return -EINVAL;
 	}
 
-	if (!uctx.dimm_id) {
-		error("No DIMM ID provided");
-		usage_with_options(u, options);
-		return -EINVAL;
-	}
-
 	rc = verify_fw_file(&uctx);
 	if (rc < 0) {
 		error("Failed to verify firmware file %s", uctx.fw_path);
 		return rc;
 	}
 
-	rc = get_ndctl_dimm(&uctx, ctx);
-	if (rc < 0) {
-		if (rc == -ENODEV)
-			error("DIMM %s not found", uctx.dimm_id);
-		return rc;
+	for (i = 0; i < argc; i++) {
+		struct ndctl_dimm *dimm;
+		struct ndctl_bus *bus;
+
+		if (sscanf(argv[i], "nmem%lu", &id) != 1
+				&& strcmp(argv[i], "all") != 0)
+			continue;
+
+		ndctl_bus_foreach(ctx, bus)
+			ndctl_dimm_foreach(bus, dimm) {
+				rc = process_dimm(&uctx, argv[i], dimm);
+				if (rc < 0) {
+					if (rc == -ENOTSUP)
+						goto cleanup;
+					continue;
+				}
+			}
 	}
 
+	if (uctx.dimm_id) {
+		struct ndctl_dimm *dimm;
+		struct ndctl_bus *bus;
+
+		ndctl_bus_foreach(ctx, bus)
+			ndctl_dimm_foreach(bus, dimm) {
+				rc = process_dimm(&uctx, uctx.dimm_id, dimm);
+				if (rc < 0)
+					goto cleanup;
+			}
+	}
+
+cleanup:
 	if (uctx.start)
 		ndctl_cmd_unref(uctx.start);
 

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

Re: [PATCH v2 4/4] ndctl: accept DIMM name without -d option

[Dan Williams]

On Thu, Feb 22, 2018 at 12:49 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> Making update-firmware in sync with other ndctl syntax and accept DIMM
> id without using a switch. The -d option will still be accepted. Now
> it will accept multiple dimm ids as well as the "all" option.
> i.e.
> ndctl update-firmware -f file.bin nmem0 nmem1
>

This is duplicating the infrastructure that exists in ndctl/dimm.c
let's drop this patch and proceed with merging firmware update into
that file.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 1/4] ndctl: add more error outs to update firmware and enable verbose debug

[Dan Williams]

On Thu, Feb 22, 2018 at 12:49 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> update-firmware is missing verbose option for debug outputs. Also adding
> additional error outs to give better indication if something has failed
> and why.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

On second look, now that we are allowing multiple DIMMs to be updated
in one invocation, these runtime error messages should be of the form:

    "%s: message...\n", ndctl_dimm_get_devname(uctx->dimm)

...note the "\n", that seems to be missing on quite a few of these
error() calls, I missed that earlier.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm