* [PATCH 1/2] kobject: Add kobject_initialized() function
@ 2014-10-28 15:26 Tomeu Vizoso
2014-10-28 15:26 ` [PATCH 2/2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get Tomeu Vizoso
2014-10-29 2:43 ` [PATCH 1/2] kobject: Add kobject_initialized() function Greg Kroah-Hartman
0 siblings, 2 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-10-28 15:26 UTC (permalink / raw)
To: linux-kernel; +Cc: Javier Martinez Canillas, Tomeu Vizoso, Greg Kroah-Hartman
To be used by users of kobject to tell when one hasn't been initialized yet.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
include/linux/kobject.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 2d61b90..6bb5a92 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -88,6 +88,11 @@ static inline const char *kobject_name(const struct kobject *kobj)
return kobj->name;
}
+static inline bool kobject_initialized(const struct kobject *kobj)
+{
+ return kobj->state_initialized;
+}
+
extern void kobject_init(struct kobject *kobj, struct kobj_type *ktype);
extern __printf(3, 4) __must_check
int kobject_add(struct kobject *kobj, struct kobject *parent,
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get
2014-10-28 15:26 [PATCH 1/2] kobject: Add kobject_initialized() function Tomeu Vizoso
@ 2014-10-28 15:26 ` Tomeu Vizoso
2014-10-29 2:44 ` Greg KH
2014-10-29 2:43 ` [PATCH 1/2] kobject: Add kobject_initialized() function Greg Kroah-Hartman
1 sibling, 1 reply; 8+ messages in thread
From: Tomeu Vizoso @ 2014-10-28 15:26 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, Tomeu Vizoso, Rafael J. Wysocki,
Viresh Kumar, linux-pm
There's a substantial window of opportunity from the time the policy objects
are created until they are initialized, causing this:
[ 4.651435] ------------[ cut here ]------------
[ 4.651453] WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
[ 4.651463] Modules linked in:
[ 4.651473] CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
[ 4.651497] [<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
[ 4.651505] [<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
[ 4.651515] [<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
[ 4.651522] [<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
[ 4.651530] [<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
[ 4.651539] [<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
[ 4.651547] [<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
[ 4.651555] [<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
[ 4.651563] [<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
[ 4.651570] [<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
[ 4.651579] [<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
[ 4.651587] [<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
[ 4.651591] ---[ end trace 7f6a15d1272e6ef3 ]---
This commit checks that the policy object has been initialized already before
trying to ref it.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/cpufreq/cpufreq.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 644b54e..37cd6c9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -214,8 +214,12 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
if (cpufreq_driver) {
/* get the CPU */
policy = per_cpu(cpufreq_cpu_data, cpu);
- if (policy)
- kobject_get(&policy->kobj);
+ if (policy) {
+ if (!kobject_initialized(&policy->kobj))
+ policy = NULL;
+ else
+ kobject_get(&policy->kobj);
+ }
}
read_unlock_irqrestore(&cpufreq_driver_lock, flags);
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] kobject: Add kobject_initialized() function
2014-10-28 15:26 [PATCH 1/2] kobject: Add kobject_initialized() function Tomeu Vizoso
2014-10-28 15:26 ` [PATCH 2/2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get Tomeu Vizoso
@ 2014-10-29 2:43 ` Greg Kroah-Hartman
1 sibling, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-29 2:43 UTC (permalink / raw)
To: Tomeu Vizoso; +Cc: linux-kernel, Javier Martinez Canillas
On Tue, Oct 28, 2014 at 04:26:47PM +0100, Tomeu Vizoso wrote:
> To be used by users of kobject to tell when one hasn't been initialized yet.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> include/linux/kobject.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index 2d61b90..6bb5a92 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -88,6 +88,11 @@ static inline const char *kobject_name(const struct kobject *kobj)
> return kobj->name;
> }
>
> +static inline bool kobject_initialized(const struct kobject *kobj)
> +{
> + return kobj->state_initialized;
> +}
Ick, no. Why would you ever need this? You "own" the kobject, and you
better know if you initialized it or not. If not, you should fix your
use of a kobject.
And why are you even using a kobject at all?
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get
2014-10-28 15:26 ` [PATCH 2/2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get Tomeu Vizoso
@ 2014-10-29 2:44 ` Greg KH
2014-10-29 15:45 ` [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get() Tomeu Vizoso
0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2014-10-29 2:44 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, Javier Martinez Canillas, Rafael J. Wysocki,
Viresh Kumar, linux-pm
On Tue, Oct 28, 2014 at 04:26:48PM +0100, Tomeu Vizoso wrote:
> There's a substantial window of opportunity from the time the policy objects
> are created until they are initialized, causing this:
>
> [ 4.651435] ------------[ cut here ]------------
> [ 4.651453] WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
> [ 4.651463] Modules linked in:
> [ 4.651473] CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
> [ 4.651497] [<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
> [ 4.651505] [<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
> [ 4.651515] [<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
> [ 4.651522] [<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
> [ 4.651530] [<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
> [ 4.651539] [<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
> [ 4.651547] [<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
> [ 4.651555] [<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
> [ 4.651563] [<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
> [ 4.651570] [<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
> [ 4.651579] [<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
> [ 4.651587] [<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
> [ 4.651591] ---[ end trace 7f6a15d1272e6ef3 ]---
>
> This commit checks that the policy object has been initialized already before
> trying to ref it.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> drivers/cpufreq/cpufreq.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 644b54e..37cd6c9 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -214,8 +214,12 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> if (cpufreq_driver) {
> /* get the CPU */
> policy = per_cpu(cpufreq_cpu_data, cpu);
> - if (policy)
> - kobject_get(&policy->kobj);
> + if (policy) {
> + if (!kobject_initialized(&policy->kobj))
> + policy = NULL;
> + else
> + kobject_get(&policy->kobj);
> + }
Something else is going on here, sorry, this isn't the correct fix, as
you are using kobjects incorrectly if this ever happens...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get()
2014-10-29 2:44 ` Greg KH
@ 2014-10-29 15:45 ` Tomeu Vizoso
2014-10-29 21:24 ` Rafael J. Wysocki
2014-11-10 9:54 ` Viresh Kumar
0 siblings, 2 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-10-29 15:45 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, Tomeu Vizoso, Rafael J. Wysocki,
Viresh Kumar, linux-pm
There's a substantial window of opportunity from the time the policy objects
are created until they are initialized, causing this:
WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
Modules linked in:
CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
[<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
[<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
[<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
[<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
[<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
[<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
[<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
[<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
[<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
[<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
[<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
[<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
This commit checks that initialization has finished before trying to do
anything with the policy.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/cpufreq/cpufreq.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 644b54e..7b84d1a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -48,6 +48,9 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
/* Flag to suspend/resume CPUFreq governors */
static bool cpufreq_suspended;
+/* Flag that tells whether initialization is completed */
+static atomic_t cpufreq_initialized = ATOMIC_INIT(0);
+
static inline bool has_target(void)
{
return cpufreq_driver->target_index || cpufreq_driver->target;
@@ -211,7 +214,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
/* get the cpufreq driver */
read_lock_irqsave(&cpufreq_driver_lock, flags);
- if (cpufreq_driver) {
+ if (atomic_read(&cpufreq_initialized)) {
/* get the CPU */
policy = per_cpu(cpufreq_cpu_data, cpu);
if (policy)
@@ -1289,6 +1292,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
kobject_uevent(&policy->kobj, KOBJ_ADD);
up_read(&cpufreq_rwsem);
+ atomic_set(&cpufreq_initialized, 1);
+
pr_debug("initialization complete\n");
return 0;
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get()
2014-10-29 15:45 ` [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get() Tomeu Vizoso
@ 2014-10-29 21:24 ` Rafael J. Wysocki
2014-10-31 8:53 ` Tomeu Vizoso
2014-11-10 9:54 ` Viresh Kumar
1 sibling, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2014-10-29 21:24 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, Javier Martinez Canillas, Viresh Kumar, linux-pm
On Wednesday, October 29, 2014 04:45:47 PM Tomeu Vizoso wrote:
> There's a substantial window of opportunity from the time the policy objects
> are created until they are initialized, causing this:
>
> WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
> Modules linked in:
> CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
> [<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
> [<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
> [<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
> [<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
> [<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
> [<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
> [<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
> [<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
> [<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
> [<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
> [<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
> [<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
>
> This commit checks that initialization has finished before trying to do
> anything with the policy.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> drivers/cpufreq/cpufreq.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 644b54e..7b84d1a 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -48,6 +48,9 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
> /* Flag to suspend/resume CPUFreq governors */
> static bool cpufreq_suspended;
>
> +/* Flag that tells whether initialization is completed */
> +static atomic_t cpufreq_initialized = ATOMIC_INIT(0);
> +
> static inline bool has_target(void)
> {
> return cpufreq_driver->target_index || cpufreq_driver->target;
> @@ -211,7 +214,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> /* get the cpufreq driver */
> read_lock_irqsave(&cpufreq_driver_lock, flags);
>
> - if (cpufreq_driver) {
> + if (atomic_read(&cpufreq_initialized)) {
The atomics don't help you here, because they don't make race conditions go
away. Memory barriers would be needed for that, but then there should be an
alternative way to address the problem at hand.
> /* get the CPU */
> policy = per_cpu(cpufreq_cpu_data, cpu);
> if (policy)
> @@ -1289,6 +1292,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> kobject_uevent(&policy->kobj, KOBJ_ADD);
> up_read(&cpufreq_rwsem);
>
> + atomic_set(&cpufreq_initialized, 1);
> +
__cpufreq_add_dev() can be run for many times. Why is the first one only
relevant?
> pr_debug("initialization complete\n");
>
> return 0;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get()
2014-10-29 21:24 ` Rafael J. Wysocki
@ 2014-10-31 8:53 ` Tomeu Vizoso
0 siblings, 0 replies; 8+ messages in thread
From: Tomeu Vizoso @ 2014-10-31 8:53 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-kernel@vger.kernel.org, Javier Martinez Canillas,
Viresh Kumar, linux-pm
On 29 October 2014 22:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, October 29, 2014 04:45:47 PM Tomeu Vizoso wrote:
>> There's a substantial window of opportunity from the time the policy objects
>> are created until they are initialized, causing this:
>>
>> WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
>> Modules linked in:
>> CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
>> [<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
>> [<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
>> [<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
>> [<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
>> [<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
>> [<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
>> [<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
>> [<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
>> [<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
>> [<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
>> [<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
>> [<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
>>
>> This commit checks that initialization has finished before trying to do
>> anything with the policy.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>> drivers/cpufreq/cpufreq.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 644b54e..7b84d1a 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -48,6 +48,9 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
>> /* Flag to suspend/resume CPUFreq governors */
>> static bool cpufreq_suspended;
>>
>> +/* Flag that tells whether initialization is completed */
>> +static atomic_t cpufreq_initialized = ATOMIC_INIT(0);
>> +
>> static inline bool has_target(void)
>> {
>> return cpufreq_driver->target_index || cpufreq_driver->target;
>> @@ -211,7 +214,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>> /* get the cpufreq driver */
>> read_lock_irqsave(&cpufreq_driver_lock, flags);
>>
>> - if (cpufreq_driver) {
>> + if (atomic_read(&cpufreq_initialized)) {
>
> The atomics don't help you here, because they don't make race conditions go
> away. Memory barriers would be needed for that, but then there should be an
> alternative way to address the problem at hand.
Yeah, now I'm not sure that atomic is even needed here, as
cpufreq_cpu_get is already returning NULL if the cpufreq_driver hasn't
been registered yet, so the callers should be already retrying.
>> /* get the CPU */
>> policy = per_cpu(cpufreq_cpu_data, cpu);
>> if (policy)
>> @@ -1289,6 +1292,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> kobject_uevent(&policy->kobj, KOBJ_ADD);
>> up_read(&cpufreq_rwsem);
>>
>> + atomic_set(&cpufreq_initialized, 1);
>> +
>
> __cpufreq_add_dev() can be run for many times. Why is the first one only
> relevant?
I see now. What about having the flag in struct cpufreq_policy instead?
TBH, I find the initialization sequence in cpufreq quite daunting, so
any guidance on this will be appreciated.
Regards,
Tomeu
>> pr_debug("initialization complete\n");
>>
>> return 0;
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get()
2014-10-29 15:45 ` [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get() Tomeu Vizoso
2014-10-29 21:24 ` Rafael J. Wysocki
@ 2014-11-10 9:54 ` Viresh Kumar
1 sibling, 0 replies; 8+ messages in thread
From: Viresh Kumar @ 2014-11-10 9:54 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Linux Kernel Mailing List, Javier Martinez Canillas,
Rafael J. Wysocki, linux-pm@vger.kernel.org
On 29 October 2014 21:15, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> There's a substantial window of opportunity from the time the policy objects
> are created until they are initialized, causing this:
>
> WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
> Modules linked in:
> CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
> [<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
> [<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
> [<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
> [<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
> [<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
> [<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
> [<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
> [<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
> [<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
> [<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
> [<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
> [<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
>
> This commit checks that initialization has finished before trying to do
> anything with the policy.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> drivers/cpufreq/cpufreq.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 644b54e..7b84d1a 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -48,6 +48,9 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
> /* Flag to suspend/resume CPUFreq governors */
> static bool cpufreq_suspended;
>
> +/* Flag that tells whether initialization is completed */
> +static atomic_t cpufreq_initialized = ATOMIC_INIT(0);
> +
> static inline bool has_target(void)
> {
> return cpufreq_driver->target_index || cpufreq_driver->target;
> @@ -211,7 +214,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> /* get the cpufreq driver */
> read_lock_irqsave(&cpufreq_driver_lock, flags);
>
> - if (cpufreq_driver) {
> + if (atomic_read(&cpufreq_initialized)) {
> /* get the CPU */
> policy = per_cpu(cpufreq_cpu_data, cpu);
> if (policy)
> @@ -1289,6 +1292,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> kobject_uevent(&policy->kobj, KOBJ_ADD);
> up_read(&cpufreq_rwsem);
>
> + atomic_set(&cpufreq_initialized, 1);
> +
> pr_debug("initialization complete\n");
Instead of all these:
Move:
ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
&dev->kobj, "cpufreq");
from cpufreq_add_dev_interface() to __cpufreq_add_dev() before
setting per_cpu(cpufreq_cpu_data, j) = policy;
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-11-10 9:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-28 15:26 [PATCH 1/2] kobject: Add kobject_initialized() function Tomeu Vizoso
2014-10-28 15:26 ` [PATCH 2/2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get Tomeu Vizoso
2014-10-29 2:44 ` Greg KH
2014-10-29 15:45 ` [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get() Tomeu Vizoso
2014-10-29 21:24 ` Rafael J. Wysocki
2014-10-31 8:53 ` Tomeu Vizoso
2014-11-10 9:54 ` Viresh Kumar
2014-10-29 2:43 ` [PATCH 1/2] kobject: Add kobject_initialized() function Greg Kroah-Hartman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.