| Message ID | 20191018202302.8122-4-jmoyer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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? > break; > case FW_EBUSY: > @@ -753,7 +751,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm, > ndctl_dimm_get_devname(dimm)); > case FW_EINVAL_CTX: > case FW_ESEQUENCE: > - done = true; > rc = -ENXIO; > goto out; > case FW_ENORES: > @@ -761,17 +758,15 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm, > "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; > } > - } while (!done); > + } while (true); I'm not a fan of "while (true)". But I'm not the maintainer. The Logic seems fine otherwise. Ira > > out: > ndctl_cmd_unref(cmd); > -- > 2.19.1 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
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. >> break; >> case FW_EBUSY: >> @@ -753,7 +751,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm, >> ndctl_dimm_get_devname(dimm)); >> case FW_EINVAL_CTX: >> case FW_ESEQUENCE: >> - done = true; >> rc = -ENXIO; >> goto out; >> case FW_ENORES: >> @@ -761,17 +758,15 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm, >> "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; >> } >> - } while (!done); >> + } while (true); > > I'm not a fan of "while (true)". But I'm not the maintainer. The Logic seems > fine otherwise. The way things stand today is a mashup of goto vs. break. I'll follow-up with fixed up patch next week if there is consensus on the change. If you have a suggestion for a better way, that's welcome as well. Thanks for looking, Ira! -Jeff
On Fri, Oct 18, 2019 at 05:06:10PM -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. > >> > >> goto out; > >> } > >> - } while (!done); > >> + } while (true); > > > > I'm not a fan of "while (true)". But I'm not the maintainer. The Logic seems > > fine otherwise. > > The way things stand today is a mashup of goto vs. break. I'll > follow-up with fixed up patch next week if there is consensus on the > change. If you have a suggestion for a better way, that's welcome as > well. Yea that is the reason I did not object strongly. I don't have a good idea of how to clean the loop up without a pretty big refactoring. Which I'm not prepared to do. :-/ So if Vishal is ok with it, I am. Ira > > Thanks for looking, Ira! > > -Jeff
On Fri, 2019-10-18 at 15:49 -0700, Ira Weiny wrote: > On Fri, Oct 18, 2019 at 05:06:10PM -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. > > > > > > > > goto out; > > > > } > > > > - } while (!done); > > > > + } while (true); > > > > > > I'm not a fan of "while (true)". But I'm not the maintainer. The Logic seems > > > fine otherwise. > > > > The way things stand today is a mashup of goto vs. break. I'll > > follow-up with fixed up patch next week if there is consensus on the > > change. If you have a suggestion for a better way, that's welcome as > > well. > > Yea that is the reason I did not object strongly. I don't have a good idea of > how to clean the loop up without a pretty big refactoring. Which I'm not > prepared to do. :-/ So if Vishal is ok with it, I am. I looked into this - and I agree that while (true) isn't the greatest. I think we can refactor it to loop off the timeout value, and that keeps the loop always bounded. For other cases we break out as usual. For now, we can go with the simpler fixup, and revisit the bigger refactoring later. Thanks, -Vishal
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: [..] > > > - } while (!done); > > > + } while (true); > > > > I'm not a fan of "while (true)". But I'm not the maintainer. The Logic seems > > fine otherwise. > > The way things stand today is a mashup of goto vs. break. I'll > follow-up with fixed up patch next week if there is consensus on the > change. If you have a suggestion for a better way, that's welcome as > well. > I've applied this as is for v67, we can look at a refactoring for the while (true) later.
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 :)
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; }
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.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; > }
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; }
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. > > 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. For this operation that is probably not a big deal. We should be waiting much longer than the operation should take anyway. > > It also doens't retry the sleep if interrupted. This could be an issue. > 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. I thought about doing something similar but to make the logic the same it becomes a bit awkward. > > 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; You need ndctl_cmd_submit() here to be the same logic. > + > + } > + > + 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); wait_for_cmd_completion() does not call ndctl_cmd_submit() Now I find it odd that we need to resubmit the command but I assume the logic is correct. Therefore we need to go back and call ndctl_cmd_submit() again. Or is this not required? anyway that is why I went ahead and used the goto... Ira
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. > >> >> 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. > > For this operation that is probably not a big deal. We should be waiting much > longer than the operation should take anyway. > >> >> It also doens't retry the sleep if interrupted. > > This could be an issue. > >> 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. > > I thought about doing something similar but to make the logic the same it > becomes a bit awkward. [...] >> + wait_for_cmd_completion(cmd, fw, &start); > > wait_for_cmd_completion() does not call ndctl_cmd_submit() > > Now I find it odd that we need to resubmit the command but I assume the logic > is correct. Therefore we need to go back and call ndctl_cmd_submit() again. > > Or is this not required? Ah. Stupid mistake. Yes, it definitely looks like the status query command needs to be resubmitted, and that's the whole point of the timeout between calls. You can't ask too often. ;-) > anyway that is why I went ahead and used the goto... I'll take another look. Thanks for pointing out that obvious thinko. -Jeff
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; }
On Mon, Oct 28, 2019 at 06:12:23PM -0400, Jeff Moyer wrote: > 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 && This needs to check for >= 0 because ndctl_cmd_submit() can return a positive value on success. See do_cmd() > + (status = ndctl_cmd_fw_xlat_firmware_status(cmd)) == FW_EBUSY) { Why not return this status rather than having to query for it again? While I'm not a fan of the goto either I think it does actually work ok. Why don't we go with that patch for now and if you want to pull the "again" loop into a separate function which fixes the signal handling of nanosleep we can do that as a follow on. But I think we need to fix the above and just return the status from this loop... Something like: static int query_fw_finish_status_timeout(struct ndctl_cmd *cmd, struct fw_info *fw, enum ND_FW_STATUS *status) { ... } [snip] > - 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"); [snip] > } > - } 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"); Final NIT I changed these to fprintf() as well. Ira
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; break; case FW_EBUSY: @@ -753,7 +751,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm, ndctl_dimm_get_devname(dimm)); case FW_EINVAL_CTX: case FW_ESEQUENCE: - done = true; rc = -ENXIO; goto out; case FW_ENORES: @@ -761,17 +758,15 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm, "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; } - } while (!done); + } while (true); out: ndctl_cmd_unref(cmd);
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(-)