diff mbox series

[v2] intel_idle: remove a couple of useless variables

Message ID 20210924104504.2430239-1-dedekind1@gmail.com (mailing list archive)
State Rejected, archived
Headers show
Series [v2] intel_idle: remove a couple of useless variables | expand

Commit Message

Artem Bityutskiy Sept. 24, 2021, 10:45 a.m. UTC 
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This patch is a cleanup and has no functional impact.

Remove the 'auto_demotion_disable_flags' and 'disable_promotion_to_c1e' global
variables because we already have them in the global 'icpu' structure. Other
'icpu' members like 'use_acpi' are not duplicated, and it's better to be
consistent on this. Consistency improves readability.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---

Changes since v1:
 * fixed crash on a platform for which the driver does not have a table. In
   this case 'icpu' is NULL and we need to check for NULL in
   'intel_idle_cpu_init()'.

 drivers/idle/intel_idle.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Rafael J. Wysocki Sept. 24, 2021, 6:19 p.m. UTC | #1 
On Fri, Sep 24, 2021 at 12:45 PM Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> This patch is a cleanup and has no functional impact.

Are you sure?

> Remove the 'auto_demotion_disable_flags' and 'disable_promotion_to_c1e' global
> variables because we already have them in the global 'icpu' structure.

Which is __initdata and these variables aren't.

> Other 'icpu' members like 'use_acpi' are not duplicated,

Because it is not necessary to access them after initialization.

> and it's better to be consistent on this.

Well, the code is as consistent as it can be.

>Consistency improves readability.

That's true, but it has no bearing in this particular case IMO.

> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>
> Changes since v1:
>  * fixed crash on a platform for which the driver does not have a table. In
>    this case 'icpu' is NULL and we need to check for NULL in
>    'intel_idle_cpu_init()'.
>
>  drivers/idle/intel_idle.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 70c2237a7261..a73183016dd4 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -67,9 +67,6 @@ static unsigned int disabled_states_mask;
>
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>
> -static unsigned long auto_demotion_disable_flags;
> -static bool disable_promotion_to_c1e;
> -
>  struct idle_cpu {
>         struct cpuidle_state *state_table;
>
> @@ -1644,7 +1641,7 @@ static void auto_demotion_disable(void)
>         unsigned long long msr_bits;
>
>         rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
> -       msr_bits &= ~auto_demotion_disable_flags;
> +       msr_bits &= ~icpu->auto_demotion_disable_flags;
>         wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>  }
>
> @@ -1676,10 +1673,13 @@ static int intel_idle_cpu_init(unsigned int cpu)
>                 return -EIO;
>         }
>
> -       if (auto_demotion_disable_flags)
> +       if (!icpu)
> +               return 0;
> +
> +       if (icpu->auto_demotion_disable_flags)
>                 auto_demotion_disable();
>
> -       if (disable_promotion_to_c1e)
> +       if (icpu->disable_promotion_to_c1e)
>                 c1e_promotion_disable();
>
>         return 0;
> @@ -1757,8 +1757,6 @@ static int __init intel_idle_init(void)
>         icpu = (const struct idle_cpu *)id->driver_data;
>         if (icpu) {
>                 cpuidle_state_table = icpu->state_table;
> -               auto_demotion_disable_flags = icpu->auto_demotion_disable_flags;
> -               disable_promotion_to_c1e = icpu->disable_promotion_to_c1e;
>                 if (icpu->use_acpi || force_use_acpi)
>                         intel_idle_acpi_cst_extract();
>         } else if (!intel_idle_acpi_cst_extract()) {
> --
> 2.31.1
>
Artem Bityutskiy Sept. 27, 2021, 6:52 a.m. UTC | #2 
On Fri, 2021-09-24 at 20:19 +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 24, 2021 at 12:45 PM Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > 
> > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> > 
> > This patch is a cleanup and has no functional impact.
> 
> Are you sure?

Not anymore. I shouldn't have sent this patch out so quickly, sorry.
Thanks for review.

Artem.
diff mbox series

Patch

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 70c2237a7261..a73183016dd4 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -67,9 +67,6 @@  static unsigned int disabled_states_mask;
 
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 
-static unsigned long auto_demotion_disable_flags;
-static bool disable_promotion_to_c1e;
-
 struct idle_cpu {
 	struct cpuidle_state *state_table;
 
@@ -1644,7 +1641,7 @@  static void auto_demotion_disable(void)
 	unsigned long long msr_bits;
 
 	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
-	msr_bits &= ~auto_demotion_disable_flags;
+	msr_bits &= ~icpu->auto_demotion_disable_flags;
 	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 }
 
@@ -1676,10 +1673,13 @@  static int intel_idle_cpu_init(unsigned int cpu)
 		return -EIO;
 	}
 
-	if (auto_demotion_disable_flags)
+	if (!icpu)
+		return 0;
+
+	if (icpu->auto_demotion_disable_flags)
 		auto_demotion_disable();
 
-	if (disable_promotion_to_c1e)
+	if (icpu->disable_promotion_to_c1e)
 		c1e_promotion_disable();
 
 	return 0;
@@ -1757,8 +1757,6 @@  static int __init intel_idle_init(void)
 	icpu = (const struct idle_cpu *)id->driver_data;
 	if (icpu) {
 		cpuidle_state_table = icpu->state_table;
-		auto_demotion_disable_flags = icpu->auto_demotion_disable_flags;
-		disable_promotion_to_c1e = icpu->disable_promotion_to_c1e;
 		if (icpu->use_acpi || force_use_acpi)
 			intel_idle_acpi_cst_extract();
 	} else if (!intel_idle_acpi_cst_extract()) {