nvme: cleanup nvme_configure_apst

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>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 11d343c..b905f91 100644 (file)
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2181,28 +2181,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;
 
        /*
@@ -2223,83 +2223,72 @@ 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;
-
-                       /*
-                        * 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;
-
-                       exit_latency_us =
-                               (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
-                       if (exit_latency_us > ctrl->ps_max_latency_us)
-                               continue;
+               goto done;
+       }
 
-                       total_latency_us =
-                               exit_latency_us +
-                               le32_to_cpu(ctrl->psd[state].entry_lat);
+       /*
+        * 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;
 
-                       /*
-                        * 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;
+               if (target)
+                       table->entries[state] = target;
 
-                       target = cpu_to_le64((state << 3) |
-                                            (transition_ms << 8));
+               /*
+                * 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;
 
-                       if (max_ps == -1)
-                               max_ps = state;
+               /*
+                * 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;
 
-                       if (total_latency_us > max_lat_us)
-                               max_lat_us = total_latency_us;
-               }
+               exit_latency_us = (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
+               if (exit_latency_us > ctrl->ps_max_latency_us)
+                       continue;
 
-               apste = 1;
+               total_latency_us = exit_latency_us +
+                       le32_to_cpu(ctrl->psd[state].entry_lat);
 
-               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);
-               }
+               /*
+                * 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;
        }
 
+       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;
 }