On Fri, 2019-10-25 at 15:21 -0700, Ira Weiny wrote: > How about this patch instead? Untested. > > Ira
Not a big deal, but just a quick note - if you include a scissors line here, I can easily apply it via git am --scissors --8<-- Otherwise this looks good in principle. I've already got Jeff's original (less intrusive) patch queued for v67 - maybe we can rebase this to be its own refactoring patch, and get some testing etc. for 68? > > 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; > } _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
