[ndctl patch 0/4] misc. cleanups

[ndctl patch 0/4] misc. cleanups

[Jeff Moyer]

These patches are motivated by warnings from the clang static
analyzer.  They don't fix bugs, and shouldn't result in any changes
in behavior.  The one exception is the fix for building tags tables.

-Jeff

[ndctl patch 1/4] util/abspath: cleanup prefix_filename
[ndctl patch 2/4] fix building of tags tables
[ndctl patch 3/4] query_fw_finish_status: get rid of redundant
[ndctl patch 4/4] load-keys: get rid of duplicate assignment
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl patch 1/4] util/abspath: cleanup prefix_filename

[Jeff Moyer]

Static checkers complain about the unused assignment to pfx_len.
The code can obviously be simplified.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 util/abspath.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/util/abspath.c b/util/abspath.c
index 09bbd27..e44236f 100644
--- a/util/abspath.c
+++ b/util/abspath.c
@@ -9,11 +9,7 @@ char *prefix_filename(const char *pfx, const char *arg)
 	struct strbuf path = STRBUF_INIT;
 	size_t pfx_len = pfx ? strlen(pfx) : 0;
 
-	if (!pfx_len)
-		;
-	else if (is_absolute_path(arg))
-		pfx_len = 0;
-	else
+	if (pfx_len && !is_absolute_path(arg))
 		strbuf_add(&path, pfx, pfx_len);
 
 	strbuf_addstr(&path, arg);
-- 
2.19.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl patch 2/4] fix building of tags tables

[Jeff Moyer]

Make <tags-variant> currently fails with:

make[1]: *** No rule to make target 'libndctl.h', needed by 'tags-am'.  Stop.

The path to libndctl.h is wrong in ndctl/lib/Makefile.am.  Fix it.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 ndctl/lib/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index fb75fda..e4eb006 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -7,7 +7,7 @@ pkginclude_HEADERS = ../libndctl.h ../ndctl.h
 lib_LTLIBRARIES = libndctl.la
 
 libndctl_la_SOURCES =\
-	libndctl.h \
+	../libndctl.h \
 	private.h \
 	../../util/log.c \
 	../../util/log.h \
-- 
2.19.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Jeff Moyer]

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;
 			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);
-- 
2.19.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[ndctl patch 4/4] load-keys: get rid of duplicate assignment

[Jeff Moyer]

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 ndctl/load-keys.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
index 981f80f..f0b7a5a 100644
--- a/ndctl/load-keys.c
+++ b/ndctl/load-keys.c
@@ -185,7 +185,6 @@ static int load_keys(struct loadkeys *lk_ctx, const char *keypath,
 
 	rc = chdir(keypath);
 	if (rc < 0) {
-		rc = -errno;
 		fprintf(stderr, "Change current work dir to %s failed: %s\n",
 				param.key_path, strerror(errno));
 		rc = -errno;
-- 
2.19.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Ira Weiny]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl patch 1/4] util/abspath: cleanup prefix_filename

[Ira Weiny]

On Fri, Oct 18, 2019 at 04:22:59PM -0400, Jeff Moyer wrote:
> Static checkers complain about the unused assignment to pfx_len.
> The code can obviously be simplified.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  util/abspath.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/util/abspath.c b/util/abspath.c
> index 09bbd27..e44236f 100644
> --- a/util/abspath.c
> +++ b/util/abspath.c
> @@ -9,11 +9,7 @@ char *prefix_filename(const char *pfx, const char *arg)
>  	struct strbuf path = STRBUF_INIT;
>  	size_t pfx_len = pfx ? strlen(pfx) : 0;
>  
> -	if (!pfx_len)
> -		;
> -	else if (is_absolute_path(arg))
> -		pfx_len = 0;
> -	else
> +	if (pfx_len && !is_absolute_path(arg))
>  		strbuf_add(&path, pfx, pfx_len);
>  
>  	strbuf_addstr(&path, arg);
> -- 
> 2.19.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl patch 2/4] fix building of tags tables

[Ira Weiny]

On Fri, Oct 18, 2019 at 04:23:00PM -0400, Jeff Moyer wrote:
> Make <tags-variant> currently fails with:
> 
> make[1]: *** No rule to make target 'libndctl.h', needed by 'tags-am'.  Stop.
> 
> The path to libndctl.h is wrong in ndctl/lib/Makefile.am.  Fix it.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  ndctl/lib/Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> index fb75fda..e4eb006 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -7,7 +7,7 @@ pkginclude_HEADERS = ../libndctl.h ../ndctl.h
>  lib_LTLIBRARIES = libndctl.la
>  
>  libndctl_la_SOURCES =\
> -	libndctl.h \
> +	../libndctl.h \
>  	private.h \
>  	../../util/log.c \
>  	../../util/log.h \
> -- 
> 2.19.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl patch 4/4] load-keys: get rid of duplicate assignment

[Ira Weiny]

On Fri, Oct 18, 2019 at 04:23:02PM -0400, Jeff Moyer wrote:
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  ndctl/load-keys.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/ndctl/load-keys.c b/ndctl/load-keys.c
> index 981f80f..f0b7a5a 100644
> --- a/ndctl/load-keys.c
> +++ b/ndctl/load-keys.c
> @@ -185,7 +185,6 @@ static int load_keys(struct loadkeys *lk_ctx, const char *keypath,
>  
>  	rc = chdir(keypath);
>  	if (rc < 0) {
> -		rc = -errno;
>  		fprintf(stderr, "Change current work dir to %s failed: %s\n",
>  				param.key_path, strerror(errno));
>  		rc = -errno;
> -- 
> 2.19.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Jeff Moyer]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Ira Weiny]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Verma, Vishal L]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Verma, Vishal L]

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.

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Verma, Vishal L]

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 :)
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Ira Weiny]

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

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

[Verma, Vishal L]

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;
>  }

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Jeff Moyer]

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

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

[Ira Weiny]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Jeff Moyer]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Jeff Moyer]

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;
 }
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Ira Weiny]

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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org