[PATCH v2 1/3] cpuidle/coupled: Remove cpuidle_device::safe_state_index

[PATCH v2 1/3] cpuidle/coupled: Remove cpuidle_device::safe_state_index

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

cpuidle_device::safe_state_index need to be initialized before
use, it should be the same as cpuidle_driver::safe_state_index.

We tackled this issue by removing the safe_state_index from the
cpuidle_device structure and use the one in the cpuidle_driver
structure instead.

Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 drivers/cpuidle/coupled.c | 4 ++--
 include/linux/cpuidle.h   | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index 7936dce..6493e40 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -473,7 +473,7 @@ int cpuidle_enter_state_coupled(struct cpuidle_device *dev,
 			return entered_state;
 		}
 		entered_state = cpuidle_enter_state(dev, drv,
-			dev->safe_state_index);
+			drv->safe_state_index);
 		local_irq_disable();
 	}
 
@@ -521,7 +521,7 @@ retry:
 		}
 
 		entered_state = cpuidle_enter_state(dev, drv,
-			dev->safe_state_index);
+			drv->safe_state_index);
 		local_irq_disable();
 	}
 
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index d075d34..786ad32 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -84,7 +84,6 @@ struct cpuidle_device {
 	struct list_head 	device_list;
 
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
-	int			safe_state_index;
 	cpumask_t		coupled_cpus;
 	struct cpuidle_coupled	*coupled;
 #endif
-- 
1.9.1

[PATCH v2 2/3] cpuidle/coupled: Remove redundant 'dev' argument of cpuidle_state_is_coupled()

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

For cpuidle_state_is_coupled(), 'dev' is not used, so remove it.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 drivers/cpuidle/coupled.c | 4 +---
 drivers/cpuidle/cpuidle.c | 4 ++--
 drivers/cpuidle/cpuidle.h | 7 +++----
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
index 6493e40..1523e2d 100644
--- a/drivers/cpuidle/coupled.c
+++ b/drivers/cpuidle/coupled.c
@@ -176,14 +176,12 @@ void cpuidle_coupled_parallel_barrier(struct cpuidle_device *dev, atomic_t *a)
 
 /**
  * cpuidle_state_is_coupled - check if a state is part of a coupled set
- * @dev: struct cpuidle_device for the current cpu
  * @drv: struct cpuidle_driver for the platform
  * @state: index of the target state in drv->states
  *
  * Returns true if the target state is coupled with cpus besides this one
  */
-bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
-	struct cpuidle_driver *drv, int state)
+bool cpuidle_state_is_coupled(struct cpuidle_driver *drv, int state)
 {
 	return drv->states[state].flags & CPUIDLE_FLAG_COUPLED;
 }
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 3325393..17a6dc0 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -214,7 +214,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		tick_broadcast_exit();
 	}
 
-	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
+	if (!cpuidle_state_is_coupled(drv, entered_state))
 		local_irq_enable();
 
 	diff = ktime_to_us(ktime_sub(time_end, time_start));
@@ -263,7 +263,7 @@ int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		  int index)
 {
-	if (cpuidle_state_is_coupled(dev, drv, index))
+	if (cpuidle_state_is_coupled(drv, index))
 		return cpuidle_enter_state_coupled(dev, drv, index);
 	return cpuidle_enter_state(dev, drv, index);
 }
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index ee97e96..178c5ad 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -34,15 +34,14 @@ extern int cpuidle_add_sysfs(struct cpuidle_device *dev);
 extern void cpuidle_remove_sysfs(struct cpuidle_device *dev);
 
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
-bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
-		struct cpuidle_driver *drv, int state);
+bool cpuidle_state_is_coupled(struct cpuidle_driver *drv, int state);
 int cpuidle_enter_state_coupled(struct cpuidle_device *dev,
 		struct cpuidle_driver *drv, int next_state);
 int cpuidle_coupled_register_device(struct cpuidle_device *dev);
 void cpuidle_coupled_unregister_device(struct cpuidle_device *dev);
 #else
-static inline bool cpuidle_state_is_coupled(struct cpuidle_device *dev,
-		struct cpuidle_driver *drv, int state)
+static inline
+bool cpuidle_state_is_coupled(struct cpuidle_driver *drv, int state)
 {
 	return false;
 }
-- 
1.9.1

[PATCH v2 3/3] cpuidle/coupled: Add sanity check for safe_state_index

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

Since we're using cpuidle_driver::safe_state_index directly as the
target state index, it's better to add the sanity check at the point
of registering the driver.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 drivers/cpuidle/driver.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 5db1478..def299e 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -223,10 +223,23 @@ static void poll_idle_init(struct cpuidle_driver *drv) {}
 static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 {
 	int ret;
+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
+	int i;
+#endif
 
 	if (!drv || !drv->state_count)
 		return -EINVAL;
 
+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
+	for (i = drv->state_count - 1; i >= 0; i--) {
+		if (cpuidle_state_is_coupled(drv, i) &&
+		    (drv->safe_state_index == i ||
+		     drv->safe_state_index < 0 ||
+		     drv->safe_state_index >= drv->state_count))
+			return -EINVAL;
+	}
+#endif
+
 	if (cpuidle_disabled())
 		return -ENODEV;
 
-- 
1.9.1

Re: [PATCH v2 1/3] cpuidle/coupled: Remove cpuidle_device::safe_state_index

[pang.xunlei]

Ping

Xunlei Pang <xlpang@126.com>  wrote 2015-08-04 PM 01:48:55:
> [PATCH v2 1/3] cpuidle/coupled: Remove cpuidle_device::safe_state_index
> 
> From: Xunlei Pang <pang.xunlei@linaro.org>
> 
> cpuidle_device::safe_state_index need to be initialized before
> use, it should be the same as cpuidle_driver::safe_state_index.
> 
> We tackled this issue by removing the safe_state_index from the
> cpuidle_device structure and use the one in the cpuidle_driver
> structure instead.
> 
> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
> ---
>  drivers/cpuidle/coupled.c | 4 ++--
>  include/linux/cpuidle.h   | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index 7936dce..6493e40 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -473,7 +473,7 @@ int cpuidle_enter_state_coupled(struct 
> cpuidle_device *dev,
>           return entered_state;
>        }
>        entered_state = cpuidle_enter_state(dev, drv,
> -         dev->safe_state_index);
> +         drv->safe_state_index);
>        local_irq_disable();
>     }
> 
> @@ -521,7 +521,7 @@ retry:
>        }
> 
>        entered_state = cpuidle_enter_state(dev, drv,
> -         dev->safe_state_index);
> +         drv->safe_state_index);
>        local_irq_disable();
>     }
> 
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index d075d34..786ad32 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -84,7 +84,6 @@ struct cpuidle_device {
>     struct list_head    device_list;
> 
>  #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> -   int         safe_state_index;
>     cpumask_t      coupled_cpus;
>     struct cpuidle_coupled   *coupled;
>  #endif
> -- 
> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" 
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--------------------------------------------------------
ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s).  If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited.  If you have received this mail in error, please delete it and notify us immediately.

Re: [PATCH v2 1/3] cpuidle/coupled: Remove cpuidle_device::safe_state_index

[Rafael J. Wysocki]

On Tuesday, August 04, 2015 01:48:55 PM Xunlei Pang wrote:
> From: Xunlei Pang <pang.xunlei@linaro.org>
> 
> cpuidle_device::safe_state_index need to be initialized before
> use, it should be the same as cpuidle_driver::safe_state_index.
> 
> We tackled this issue by removing the safe_state_index from the
> cpuidle_device structure and use the one in the cpuidle_driver
> structure instead.
> 
> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>

I've queued up this one and the [2/3] for 4.3, but the [3/3] needs some
rework in my view.  I'll reply to it separately.

Thanks,
Rafael

Re: [PATCH v2 3/3] cpuidle/coupled: Add sanity check for safe_state_index

[Rafael J. Wysocki]

On Tuesday, August 04, 2015 01:48:57 PM Xunlei Pang wrote:
> From: Xunlei Pang <pang.xunlei@linaro.org>
> 
> Since we're using cpuidle_driver::safe_state_index directly as the
> target state index, it's better to add the sanity check at the point
> of registering the driver.
> 
> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
> ---
>  drivers/cpuidle/driver.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 5db1478..def299e 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -223,10 +223,23 @@ static void poll_idle_init(struct cpuidle_driver *drv) {}
>  static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>  {
>  	int ret;
> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> +	int i;
> +#endif
>  
>  	if (!drv || !drv->state_count)
>  		return -EINVAL;
>  
> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> +	for (i = drv->state_count - 1; i >= 0; i--) {
> +		if (cpuidle_state_is_coupled(drv, i) &&
> +		    (drv->safe_state_index == i ||
> +		     drv->safe_state_index < 0 ||
> +		     drv->safe_state_index >= drv->state_count))
> +			return -EINVAL;
> +	}
> +#endif

Please move this code to a separate function depending on
CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED, define a "return 0" stub of it for
CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED unset and call it from here.

Generally, please try to avoid using #ifdef and similar in function bodies
if possible.

> +
>  	if (cpuidle_disabled())
>  		return -ENODEV;
>  
> 

Thanks,
Rafael