[PATCH] nvme: cleanup nvme_configure_apst

[PATCH] nvme: cleanup nvme_configure_apst

[Christoph Hellwig]

Remove a level of indentation from the main code implementating the table
search by using a goto for the APST not supported case.  Also move the
main comment above the function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 148 ++++++++++++++++++---------------------
 1 file changed, 69 insertions(+), 79 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 314705da2c1076..78817190099040 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2550,28 +2550,28 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+/*
+ * APST (Autonomous Power State Transition) lets us program a table of power
+ * state transitions that the controller will perform automatically.
+ * We configure it with a simple heuristic: we are willing to spend at most 2%
+ * of the time transitioning between power states.  Therefore, when running in
+ * any given state, we will enter the next lower-power non-operational state
+ * after waiting 50 * (enlat + exlat) microseconds, as long as that state's exit
+ * latency is under the requested maximum latency.
+ *
+ * We will not autonomously enter any non-operational state for which the total
+ * latency exceeds ps_max_latency_us.
+ *
+ * Users can set ps_max_latency_us to zero to turn off APST.
+ */
 static int nvme_configure_apst(struct nvme_ctrl *ctrl)
 {
-	/*
-	 * APST (Autonomous Power State Transition) lets us program a
-	 * table of power state transitions that the controller will
-	 * perform automatically.  We configure it with a simple
-	 * heuristic: we are willing to spend at most 2% of the time
-	 * transitioning between power states.  Therefore, when running
-	 * in any given state, we will enter the next lower-power
-	 * non-operational state after waiting 50 * (enlat + exlat)
-	 * microseconds, as long as that state's exit latency is under
-	 * the requested maximum latency.
-	 *
-	 * We will not autonomously enter any non-operational state for
-	 * which the total latency exceeds ps_max_latency_us.  Users
-	 * can set ps_max_latency_us to zero to turn off APST.
-	 */
-
-	unsigned apste;
 	struct nvme_feat_auto_pst *table;
+	unsigned apste = 0;
 	u64 max_lat_us = 0;
+	__le64 target = 0;
 	int max_ps = -1;
+	int state;
 	int ret;
 
 	/*
@@ -2592,83 +2592,73 @@ static int nvme_configure_apst(struct nvme_ctrl *ctrl)
 
 	if (!ctrl->apst_enabled || ctrl->ps_max_latency_us == 0) {
 		/* Turn off APST. */
-		apste = 0;
 		dev_dbg(ctrl->device, "APST disabled\n");
-	} else {
-		__le64 target = cpu_to_le64(0);
-		int state;
-
-		/*
-		 * Walk through all states from lowest- to highest-power.
-		 * According to the spec, lower-numbered states use more
-		 * power.  NPSS, despite the name, is the index of the
-		 * lowest-power state, not the number of states.
-		 */
-		for (state = (int)ctrl->npss; state >= 0; state--) {
-			u64 total_latency_us, exit_latency_us, transition_ms;
-
-			if (target)
-				table->entries[state] = target;
-
-			/*
-			 * Don't allow transitions to the deepest state
-			 * if it's quirked off.
-			 */
-			if (state == ctrl->npss &&
-			    (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS))
-				continue;
+		goto done;
+	}
 
-			/*
-			 * Is this state a useful non-operational state for
-			 * higher-power states to autonomously transition to?
-			 */
-			if (!(ctrl->psd[state].flags &
-			      NVME_PS_FLAGS_NON_OP_STATE))
-				continue;
+	/*
+	 * Walk through all states from lowest- to highest-power.
+	 * According to the spec, lower-numbered states use more power.  NPSS,
+	 * despite the name, is the index of the lowest-power state, not the
+	 * number of states.
+	 */
+	for (state = (int)ctrl->npss; state >= 0; state--) {
+		u64 total_latency_us, exit_latency_us, transition_ms;
 
-			exit_latency_us =
-				(u64)le32_to_cpu(ctrl->psd[state].exit_lat);
-			if (exit_latency_us > ctrl->ps_max_latency_us)
-				continue;
+		if (target)
+			table->entries[state] = target;
 
-			total_latency_us =
-				exit_latency_us +
-				le32_to_cpu(ctrl->psd[state].entry_lat);
+		/*
+		 * Don't allow transitions to the deepest state if it's quirked
+		 * off.
+		 */
+		if (state == ctrl->npss &&
+		    (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS))
+			continue;
 
-			/*
-			 * This state is good.  Use it as the APST idle
-			 * target for higher power states.
-			 */
-			transition_ms = total_latency_us + 19;
-			do_div(transition_ms, 20);
-			if (transition_ms > (1 << 24) - 1)
-				transition_ms = (1 << 24) - 1;
+		/*
+		 * Is this state a useful non-operational state for higher-power
+		 * states to autonomously transition to?
+		 */
+		if (!(ctrl->psd[state].flags & NVME_PS_FLAGS_NON_OP_STATE))
+			continue;
 
-			target = cpu_to_le64((state << 3) |
-					     (transition_ms << 8));
+		exit_latency_us = (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
+		if (exit_latency_us > ctrl->ps_max_latency_us)
+			continue;
 
-			if (max_ps == -1)
-				max_ps = state;
+		total_latency_us = exit_latency_us +
+			le32_to_cpu(ctrl->psd[state].entry_lat);
 
-			if (total_latency_us > max_lat_us)
-				max_lat_us = total_latency_us;
-		}
+		/*
+		 * This state is good.  Use it as the APST idle target for
+		 * higher power states.
+		 */
+		transition_ms = total_latency_us + 19;
+		do_div(transition_ms, 20);
+		if (transition_ms > (1 << 24) - 1)
+			transition_ms = (1 << 24) - 1;
+
+		target = cpu_to_le64((state << 3) | (transition_ms << 8));
+		if (max_ps == -1)
+			max_ps = state;
+		if (total_latency_us > max_lat_us)
+			max_lat_us = total_latency_us;
+	}
 
-		apste = 1;
 
-		if (max_ps == -1) {
-			dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
-		} else {
-			dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
-				max_ps, max_lat_us, (int)sizeof(*table), table);
-		}
-	}
+	if (max_ps == -1)
+		dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
+	else
+		dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
+			max_ps, max_lat_us, (int)sizeof(*table), table);
+	apste = 1;
 
+done:
 	ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
 				table, sizeof(*table), NULL);
 	if (ret)
 		dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
-
 	kfree(table);
 	return ret;
 }
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: cleanup nvme_configure_apst

[Niklas Cassel]

On Fri, Apr 09, 2021 at 11:45:24AM +0200, Christoph Hellwig wrote:
> Remove a level of indentation from the main code implementating the table
> search by using a goto for the APST not supported case.  Also move the
> main comment above the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

(snip)

>  
> +done:
>  	ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
>  				table, sizeof(*table), NULL);
>  	if (ret)
>  		dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> -
>  	kfree(table);
>  	return ret;
>  }

Since your patch includes whitespace cleanup related to apst,
perhaps you could remove the trailing whitespace after the
nvme_configure_apst() call (in nvme_init_ctrl_finish()) as well?

It's the only trailing whitespace that we have in all of core.c,
and it makes my eyes flinch everytime I see it ;)


Kind regards,
Niklas
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: cleanup nvme_configure_apst

[Christoph Hellwig]

On Fri, Apr 09, 2021 at 04:49:30PM +0000, Niklas Cassel wrote:
> >  	ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
> >  				table, sizeof(*table), NULL);
> >  	if (ret)
> >  		dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> > -
> >  	kfree(table);
> >  	return ret;
> >  }
> 
> Since your patch includes whitespace cleanup related to apst,
> perhaps you could remove the trailing whitespace after the
> nvme_configure_apst() call (in nvme_init_ctrl_finish()) as well?
> 
> It's the only trailing whitespace that we have in all of core.c,
> and it makes my eyes flinch everytime I see it ;)

Well, this patch only touches nvme_configure_apst, not any of the
callers.  But I'd gladly take a patch to clean up the trailing
whitespace from you.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: cleanup nvme_configure_apst

[Christoph Hellwig]

Any chance to get a review on this patch?

On Fri, Apr 09, 2021 at 04:49:30PM +0000, Niklas Cassel wrote:
> On Fri, Apr 09, 2021 at 11:45:24AM +0200, Christoph Hellwig wrote:
> > Remove a level of indentation from the main code implementating the table
> > search by using a goto for the APST not supported case.  Also move the
> > main comment above the function.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> 
> (snip)
> 
> >  
> > +done:
> >  	ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
> >  				table, sizeof(*table), NULL);
> >  	if (ret)
> >  		dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> > -
> >  	kfree(table);
> >  	return ret;
> >  }
> 
> Since your patch includes whitespace cleanup related to apst,
> perhaps you could remove the trailing whitespace after the
> nvme_configure_apst() call (in nvme_init_ctrl_finish()) as well?
> 
> It's the only trailing whitespace that we have in all of core.c,
> and it makes my eyes flinch everytime I see it ;)
> 
> 
> Kind regards,
> Niklas---end quoted text---

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: cleanup nvme_configure_apst

[Niklas Cassel]

On Fri, Apr 09, 2021 at 11:45:24AM +0200, Christoph Hellwig wrote:
> Remove a level of indentation from the main code implementating the table
> search by using a goto for the APST not supported case.  Also move the
> main comment above the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 148 ++++++++++++++++++---------------------
>  1 file changed, 69 insertions(+), 79 deletions(-)
> 

(snip)

> +		/*
> +		 * This state is good.  Use it as the APST idle target for
> +		 * higher power states.
> +		 */
> +		transition_ms = total_latency_us + 19;
> +		do_div(transition_ms, 20);
> +		if (transition_ms > (1 << 24) - 1)
> +			transition_ms = (1 << 24) - 1;
> +
> +		target = cpu_to_le64((state << 3) | (transition_ms << 8));
> +		if (max_ps == -1)
> +			max_ps = state;
> +		if (total_latency_us > max_lat_us)
> +			max_lat_us = total_latency_us;
> +	}
>  

First new line.

> -		apste = 1;
>  

Second new line.

> -		if (max_ps == -1) {
> -			dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
> -		} else {
> -			dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
> -				max_ps, max_lat_us, (int)sizeof(*table), table);
> -		}
> -	}
> +	if (max_ps == -1)
> +		dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
> +	else
> +		dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
> +			max_ps, max_lat_us, (int)sizeof(*table), table);
> +	apste = 1;
>  
> +done:
>  	ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
>  				table, sizeof(*table), NULL);
>  	if (ret)
>  		dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> -
>  	kfree(table);
>  	return ret;
>  }

If you remove the second new line, such that there are not two succeeding
new lines:

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme