From: Xunlei Pang <[email protected]>
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 <[email protected]>
Signed-off-by: Xunlei Pang <[email protected]>
---
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
From: Xunlei Pang <[email protected]>
For cpuidle_state_is_coupled(), 'dev' is not used, so remove it.
Signed-off-by: Xunlei Pang <[email protected]>
---
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
From: Xunlei Pang <[email protected]>
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 <[email protected]>
---
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
Ping
Xunlei Pang <[email protected]> wrote 2015-08-04 PM 01:48:55:
> [PATCH v2 1/3] cpuidle/coupled: Remove cpuidle_device::safe_state_index
>
> From: Xunlei Pang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Xunlei Pang <[email protected]>
> ---
> 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 [email protected]
> 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.
On Tuesday, August 04, 2015 01:48:55 PM Xunlei Pang wrote:
> From: Xunlei Pang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Xunlei Pang <[email protected]>
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
On Tuesday, August 04, 2015 01:48:57 PM Xunlei Pang wrote:
> From: Xunlei Pang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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