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:

>> (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.  I think separating out the
waiting for busy to its own function would make this more clear.
Looking more closely, there are other issues.  The timeout code looks at
the seconds, but ignores the fractions, so you could be off by almost an
entire second, there.  It also doens't retry the sleep if interrupted.
Finally, I find the variables names to be highly confusing.

I've decided not to fix those last two bugs just yet, but here's a patch
that shows the dirction I think it should go.  Compile-tested only for
now.  Let me know what you think.

Ira, I used the same base as you.  If you updated ndctl, you'll have to
revert 9e0391e057b36 to apply this patch.

Cheers,
Jeff

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index c8821d6..701f58b 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -674,6 +674,41 @@ out:
 	return rc;
 }
 
+static void wait_for_cmd_completion(struct ndctl_cmd *cmd, struct fw_info *fw,
+				    struct timespec *start)
+{
+	enum ND_FW_STATUS status;
+	struct timespec sleeptime, now;
+	int rc;
+
+	sleeptime.tv_nsec = fw->query_interval / 1000;
+	sleeptime.tv_sec = 0;
+
+	while ((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;
+}
+
 static int query_fw_finish_status(struct ndctl_dimm *dimm,
 		struct action_context *actx)
 {
@@ -682,98 +717,65 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
 	struct ndctl_cmd *cmd;
 	int rc;
 	enum ND_FW_STATUS status;
-	bool done = false;
-	struct timespec now, before, after;
+	struct timespec start;
 	uint64_t ver;
 
 	cmd = ndctl_dimm_cmd_new_fw_finish_query(uctx->start);
 	if (!cmd)
 		return -ENXIO;
 
-	rc = clock_gettime(CLOCK_MONOTONIC, &before);
+	rc = clock_gettime(CLOCK_MONOTONIC, &start);
 	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;
+		goto unref;
 
-		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");
-			done = true;
-			rc = 0;
-			break;
-		case FW_EBUSY:
-			/* Still on going, continue */
-			rc = clock_gettime(CLOCK_MONOTONIC, &after);
-			if (rc < 0) {
-				rc = -errno;
-				goto out;
-			}
+	rc = ndctl_cmd_submit(cmd);
+	if (rc < 0)
+		goto unref;
 
-			/*
-			 * If we expire max query time,
-			 * we timed out
-			 */
-			if (after.tv_sec - before.tv_sec >
-					fw->max_query / 1000000) {
-				rc = -ETIMEDOUT;
-				goto out;
-			}
+	wait_for_cmd_completion(cmd, fw, &start);
 
-			/*
-			 * 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:
-			done = true;
+	/* We are done determine error code */
+	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;
-			done = true;
-			goto out;
-		default:
-			fprintf(stderr,
-				"Unknown update status: %#x on DIMM %s\n",
-				status, ndctl_dimm_get_devname(dimm));
-			rc = -EINVAL;
-			done = true;
-			goto out;
+			break;
 		}
-	} while (!done);
 
-out:
+		fprintf(stderr, "Image updated successfully to DIMM %s.\n",
+			ndctl_dimm_get_devname(dimm));
+		fprintf(stderr, "Firmware version %#lx.\n", ver);
+		fprintf(stderr, "Cold reboot to activate.\n");
+		rc = 0;
+		break;
+	case FW_EBADFW:
+		fprintf(stderr,
+			"Firmware failed to verify by DIMM %s.\n",
+			ndctl_dimm_get_devname(dimm));
+		/* FALLTHROUGH */
+	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