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.we...@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 <jmo...@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.we...@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 <jmo...@redhat.com> Signed-off-by: Ira Weiny <ira.we...@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-le...@lists.01.org