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

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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