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

[Ira Weiny <ira.weiny@intel.com>]

On Wed, Oct 23, 2019 at 03:51:21PM -0700, 'Vishal Verma' wrote:
> On Wed, 2019-10-23 at 22:28 +0000, Verma, Vishal L wrote:
> > On Fri, 2019-10-18 at 17:06 -0400, Jeff Moyer wrote:
> > > Ira Weiny <ira.weiny@intel.com> writes:
> > > > On Fri, Oct 18, 2019 at 04:23:01PM -0400, Jeff Moyer wrote:
> > > > > The 'done' variable only adds confusion.
> > > > > 
> > > > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> > > > > ---
> > > > >  ndctl/dimm.c | 7 +------
> > > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> > > > > index c8821d6..f28b9c1 100644
> > > > > --- a/ndctl/dimm.c
> > > > > +++ b/ndctl/dimm.c
> > > > > @@ -682,7 +682,6 @@ 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;
> > > > >  	uint64_t ver;
> > > > >  
> > > > > @@ -716,7 +715,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
> > > > >  					ndctl_dimm_get_devname(dimm));
> > > > >  			printf("Firmware version %#lx.\n", ver);
> > > > >  			printf("Cold reboot to activate.\n");
> > > > > -			done = true;
> > > > >  			rc = 0;
> > > > 
> > > > Do we need "goto out" here?
> > > 
> > > Yes, I missed that one.  Thanks.
> > 
> > This actually looks fine, since there is a 'break' down below.
> > 
> > > > >  			break;
> > > > >  		case FW_EBUSY:
> 
> (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.

Ira

From 24511b6a9f1b5e5c9e36c70ef6a03da5100cf4c7 Mon Sep 17 00:00:00 2001
From: Ira Weiny <ira.weiny@intel.com>
Date: Fri, 25 Oct 2019 15:16:13 -0700
Subject: [PATCH] ndctl: Clean up loop logic in query_fw_finish_status

This gets rid of a redundant variable as originally pointed out by Jeff
Moyer[1]

Also, while we are here change the printf's to fprintf(stderr, ...)

[1] https://patchwork.kernel.org/patch/11199557/

Suggested-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 ndctl/dimm.c | 142 +++++++++++++++++++++++++--------------------------
 1 file changed, 70 insertions(+), 72 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 5e6fa19bab15..84de014e93d6 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -682,7 +682,6 @@ 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;
 	uint64_t ver;
 
@@ -692,88 +691,87 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
 
 	rc = clock_gettime(CLOCK_MONOTONIC, &before);
 	if (rc < 0)
-		goto out;
+		goto unref;
 
 	now.tv_nsec = fw->query_interval / 1000;
 	now.tv_sec = 0;
 
-	do {
-		rc = ndctl_cmd_submit(cmd);
-		if (rc < 0)
-			break;
+again:
+	rc = ndctl_cmd_submit(cmd);
+	if (rc < 0)
+		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;
-			}
+	status = ndctl_cmd_fw_xlat_firmware_status(cmd);
+	if (status == FW_EBUSY) {
+		/* Still on going, continue */
+		rc = clock_gettime(CLOCK_MONOTONIC, &after);
+		if (rc < 0) {
+			rc = -errno;
+			goto unref;
+		}
 
-			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;
-			}
+		/*
+		 * If we expire max query time,
+		 * we timed out
+		 */
+		if (after.tv_sec - before.tv_sec >
+				fw->max_query / 1000000) {
+			rc = -ETIMEDOUT;
+			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;
-			}
+		/*
+		 * Sleep the interval dictated by firmware
+		 * before query again.
+		 */
+		rc = nanosleep(&now, NULL);
+		if (rc < 0) {
+			rc = -errno;
+			goto unref;
+		}
+		goto again;
+	}
 
-			/*
-			 * 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 */
+	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;
+			goto unref;
 		}
-	} 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_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;
 }
-- 
2.20.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org