Re: [ndctl patch 3/4] query_fw_finish_status: get rid of redundant variable

[Jeff Moyer <jmoyer@redhat.com>]

Ira Weiny <ira.weiny@intel.com> writes:

> On Mon, Oct 28, 2019 at 03:37:48PM -0400, Jeff Moyer wrote:
>> Ira Weiny <ira.weiny@intel.com> writes:
>> 
>> >> (Watching the unit test run fall into an infinite loop..) Nope, the
>> >> break is in the switch scope, the while loop needs the 'goto out'.
>> >> 
>> >> Yes this bit definitely needs to be refactored :)
>> >
>> > How about this patch instead?  Untested.
>> 
>> I'm not a fan of the looping with gotos.
>
> Me either... But... the logic here is not the same.

How about this one, then?  Again, compile-tested only.  I'll run it
through testing only if you like it better than your approach.  If you
like your appraoch better, I'll go ahead and review and test that.

Cheers,
Jeff

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index b1b84c2..63d4d4a 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -674,6 +674,52 @@ out:
 	return rc;
 }
 
+/*
+ * Wait for a command to complete, up to the firmware-specified timeout.
+ * Returns -errno on error.  On success, which means either the command
+ * completed (sucessfully or with an error), or we timed out waiting for
+ * it, return 0.  The caller needs to check the status on its own if this
+ * function returns 0.
+ */
+static int query_fw_finish_status_timeout(struct ndctl_cmd *cmd,
+					  struct fw_info *fw)
+{
+	enum ND_FW_STATUS status;
+	struct timespec sleeptime, start, now;
+	int rc;
+
+	rc = clock_gettime(CLOCK_MONOTONIC, &start);
+	if (rc < 0)
+		return rc;
+
+	sleeptime.tv_nsec = fw->query_interval / 1000;
+	sleeptime.tv_sec = 0;
+
+	while ((rc = ndctl_cmd_submit(cmd)) == 0 &&
+	       (status = ndctl_cmd_fw_xlat_firmware_status(cmd)) == FW_EBUSY) {
+
+		rc = clock_gettime(CLOCK_MONOTONIC, &now);
+		if (rc < 0)
+			break;
+
+		/*
+		 * If we expire max query time, we timed out
+		 */
+		if (now.tv_sec - start.tv_sec > fw->max_query / 1000000)
+			break;
+
+		/*
+		 * Sleep the interval dictated by firmware before
+		 * query again.
+		 */
+		rc = nanosleep(&sleeptime, NULL);
+		if (rc < 0)
+			break;
+	}
+
+	return rc;
+}
+
 static int query_fw_finish_status(struct ndctl_dimm *dimm,
 		struct action_context *actx)
 {
@@ -682,94 +728,55 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
 	struct ndctl_cmd *cmd;
 	int rc;
 	enum ND_FW_STATUS status;
-	struct timespec now, before, after;
 	uint64_t ver;
 
 	cmd = ndctl_dimm_cmd_new_fw_finish_query(uctx->start);
 	if (!cmd)
 		return -ENXIO;
 
-	rc = clock_gettime(CLOCK_MONOTONIC, &before);
+	rc = query_fw_finish_status_timeout(cmd, fw);
 	if (rc < 0)
-		goto out;
-
-	now.tv_nsec = fw->query_interval / 1000;
-	now.tv_sec = 0;
-
-	do {
-		rc = ndctl_cmd_submit(cmd);
-		if (rc < 0)
-			break;
-
-		status = ndctl_cmd_fw_xlat_firmware_status(cmd);
-		switch (status) {
-		case FW_SUCCESS:
-			ver = ndctl_cmd_fw_fquery_get_fw_rev(cmd);
-			if (ver == 0) {
-				fprintf(stderr, "No firmware updated.\n");
-				rc = -ENXIO;
-				goto out;
-			}
-
-			printf("Image updated successfully to DIMM %s.\n",
-					ndctl_dimm_get_devname(dimm));
-			printf("Firmware version %#lx.\n", ver);
-			printf("Cold reboot to activate.\n");
-			rc = 0;
-			goto out;
-			break;
-		case FW_EBUSY:
-			/* Still on going, continue */
-			rc = clock_gettime(CLOCK_MONOTONIC, &after);
-			if (rc < 0) {
-				rc = -errno;
-				goto out;
-			}
-
-			/*
-			 * If we expire max query time,
-			 * we timed out
-			 */
-			if (after.tv_sec - before.tv_sec >
-					fw->max_query / 1000000) {
-				rc = -ETIMEDOUT;
-				goto out;
-			}
+		goto unref;
 
-			/*
-			 * Sleep the interval dictated by firmware
-			 * before query again.
-			 */
-			rc = nanosleep(&now, NULL);
-			if (rc < 0) {
-				rc = -errno;
-				goto out;
-			}
-			break;
-		case FW_EBADFW:
-			fprintf(stderr,
-				"Firmware failed to verify by DIMM %s.\n",
-				ndctl_dimm_get_devname(dimm));
-		case FW_EINVAL_CTX:
-		case FW_ESEQUENCE:
+	status = ndctl_cmd_fw_xlat_firmware_status(cmd);
+	switch (status) {
+	case FW_SUCCESS:
+		ver = ndctl_cmd_fw_fquery_get_fw_rev(cmd);
+		if (ver == 0) {
+			fprintf(stderr, "No firmware updated.\n");
 			rc = -ENXIO;
-			goto out;
-		case FW_ENORES:
-			fprintf(stderr,
-				"Firmware update sequence timed out: %s\n",
-				ndctl_dimm_get_devname(dimm));
-			rc = -ETIMEDOUT;
-			goto out;
-		default:
-			fprintf(stderr,
-				"Unknown update status: %#x on DIMM %s\n",
-				status, ndctl_dimm_get_devname(dimm));
-			rc = -EINVAL;
-			goto out;
+			break;
 		}
-	} while (true);
 
-out:
+		printf("Image updated successfully to DIMM %s.\n",
+		       ndctl_dimm_get_devname(dimm));
+		printf("Firmware version %#lx.\n", ver);
+		printf("Cold reboot to activate.\n");
+		break;
+	case FW_EBADFW:
+		fprintf(stderr,
+			"Firmware failed to verify by DIMM %s.\n",
+			ndctl_dimm_get_devname(dimm));
+	case FW_EINVAL_CTX:
+	case FW_ESEQUENCE:
+		rc = -ENXIO;
+		break;
+	case FW_EBUSY:
+	case FW_ENORES:
+		fprintf(stderr,
+			"Firmware update sequence timed out: %s\n",
+			ndctl_dimm_get_devname(dimm));
+		rc = -ETIMEDOUT;
+		break;
+	default:
+		fprintf(stderr,
+			"Unknown update status: %#x on DIMM %s\n",
+			status, ndctl_dimm_get_devname(dimm));
+		rc = -EINVAL;
+		break;
+	}
+
+unref:
 	ndctl_cmd_unref(cmd);
 	return rc;
 }
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org