Last known good: 3.8.0
Bad version: 3.9-rc1
[ 6.116492] =====================================
[ 6.116614] [ BUG: ktpacpi_nvramd/446 still has locks held! ]
[ 6.116737] 3.9.0-rc1 #1 Not tainted
[ 6.116821] -------------------------------------
[ 6.116900] 1 lock held by ktpacpi_nvramd/446:
[ 6.116973] #0: (&hotkey_thread_mutex){+.+...}, at: [<ffffffffa024e87e>]
hotkey_kthread+0x1f/0x354 [thinkpad_acpi]
[ 6.117193]
[ 6.117193] stack backtrace:
[ 6.117268] Pid: 446, comm: ktpacpi_nvramd Not tainted 3.9.0-rc1 #1
[ 6.117381] Call Trace:
[ 6.117445] [<ffffffff810877ea>] debug_check_no_locks_held+0x8f/0x93
[ 6.117600] [<ffffffff8107d8ac>] set_freezable+0x3e/0x64
[ 6.117703] input: ThinkPad Extra Buttons as
/devices/platform/thinkpad_acpi/input/input5
[ 6.117918] [<ffffffffa024e890>] hotkey_kthread+0x31/0x354 [thinkpad_acpi]
[ 6.118088] [<ffffffffa024e85f>] ? issue_volchange.29885+0x54/0x54
[thinkpad_acpi]
[ 6.118250] [<ffffffff8105e296>] kthread+0xac/0xb4
[ 6.118356] [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
[ 6.118491] [<ffffffff814b243c>] ret_from_fork+0x7c/0xb0
[ 6.118614] [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
Config:
http://mrutecki.pl/download/kernel/3.9.0-rc1/config-3.9.0-rc1
full dmesg:
http://mrutecki.pl/download/kernel/3.9.0-rc1/dmesg-3.9.0-rc1.txt
--
Maciej Rutecki
http://www.mrutecki.pl
On 03/05/2013 03:55 AM, Maciej Rutecki wrote:
> Last known good: 3.8.0
> Bad version: 3.9-rc1
>
> [ 6.116492] =====================================
> [ 6.116614] [ BUG: ktpacpi_nvramd/446 still has locks held! ]
> [ 6.116737] 3.9.0-rc1 #1 Not tainted
> [ 6.116821] -------------------------------------
> [ 6.116900] 1 lock held by ktpacpi_nvramd/446:
> [ 6.116973] #0: (&hotkey_thread_mutex){+.+...}, at: [<ffffffffa024e87e>]
> hotkey_kthread+0x1f/0x354 [thinkpad_acpi]
> [ 6.117193]
> [ 6.117193] stack backtrace:
> [ 6.117268] Pid: 446, comm: ktpacpi_nvramd Not tainted 3.9.0-rc1 #1
> [ 6.117381] Call Trace:
> [ 6.117445] [<ffffffff810877ea>] debug_check_no_locks_held+0x8f/0x93
> [ 6.117600] [<ffffffff8107d8ac>] set_freezable+0x3e/0x64
> [ 6.117703] input: ThinkPad Extra Buttons as
> /devices/platform/thinkpad_acpi/input/input5
> [ 6.117918] [<ffffffffa024e890>] hotkey_kthread+0x31/0x354 [thinkpad_acpi]
> [ 6.118088] [<ffffffffa024e85f>] ? issue_volchange.29885+0x54/0x54
> [thinkpad_acpi]
> [ 6.118250] [<ffffffff8105e296>] kthread+0xac/0xb4
> [ 6.118356] [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
> [ 6.118491] [<ffffffff814b243c>] ret_from_fork+0x7c/0xb0
> [ 6.118614] [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
>
> Config:
> http://mrutecki.pl/download/kernel/3.9.0-rc1/config-3.9.0-rc1
>
> full dmesg:
> http://mrutecki.pl/download/kernel/3.9.0-rc1/dmesg-3.9.0-rc1.txt
>
Thanks for the report!
Looks like the following commit is related:
commit 6aa9707099c4b25700940eb3d016f16c4434360d
Author: Mandeep Singh Baines <[email protected]> Thu Feb 28 09:03:18 2013
lockdep: check that no locks held at freeze time
And the code to trigger this problem is here:
static int hotkey_kthread(void *data)
{
struct tp_nvram_state s[2];
u32 poll_mask, event_mask;
unsigned int si, so;
unsigned long t;
unsigned int change_detector;
unsigned int poll_freq;
bool was_frozen;
mutex_lock(&hotkey_thread_mutex);
if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
goto exit;
set_freezable();
~~~~~~~~~~~~~~~~~~~~~~~~~~
in thinkpad_acpi.c.
I don't know much about freeze, I've no idea what is the problem.
So Mandeep and Henrique, can you please kindly take a look? Thanks.
-Aaron
On Mon, Mar 4, 2013 at 11:28 PM, Aaron Lu <[email protected]> wrote:
> On 03/05/2013 03:55 AM, Maciej Rutecki wrote:
>> Last known good: 3.8.0
>> Bad version: 3.9-rc1
>>
>> [ 6.116492] =====================================
>> [ 6.116614] [ BUG: ktpacpi_nvramd/446 still has locks held! ]
>> [ 6.116737] 3.9.0-rc1 #1 Not tainted
>> [ 6.116821] -------------------------------------
>> [ 6.116900] 1 lock held by ktpacpi_nvramd/446:
>> [ 6.116973] #0: (&hotkey_thread_mutex){+.+...}, at: [<ffffffffa024e87e>]
>> hotkey_kthread+0x1f/0x354 [thinkpad_acpi]
>> [ 6.117193]
>> [ 6.117193] stack backtrace:
>> [ 6.117268] Pid: 446, comm: ktpacpi_nvramd Not tainted 3.9.0-rc1 #1
>> [ 6.117381] Call Trace:
>> [ 6.117445] [<ffffffff810877ea>] debug_check_no_locks_held+0x8f/0x93
>> [ 6.117600] [<ffffffff8107d8ac>] set_freezable+0x3e/0x64
>> [ 6.117703] input: ThinkPad Extra Buttons as
>> /devices/platform/thinkpad_acpi/input/input5
>> [ 6.117918] [<ffffffffa024e890>] hotkey_kthread+0x31/0x354 [thinkpad_acpi]
>> [ 6.118088] [<ffffffffa024e85f>] ? issue_volchange.29885+0x54/0x54
>> [thinkpad_acpi]
>> [ 6.118250] [<ffffffff8105e296>] kthread+0xac/0xb4
>> [ 6.118356] [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
>> [ 6.118491] [<ffffffff814b243c>] ret_from_fork+0x7c/0xb0
>> [ 6.118614] [<ffffffff8105e1ea>] ? __kthread_parkme+0x60/0x60
>>
>> Config:
>> http://mrutecki.pl/download/kernel/3.9.0-rc1/config-3.9.0-rc1
>>
>> full dmesg:
>> http://mrutecki.pl/download/kernel/3.9.0-rc1/dmesg-3.9.0-rc1.txt
>>
>
> Thanks for the report!
>
> Looks like the following commit is related:
> commit 6aa9707099c4b25700940eb3d016f16c4434360d
> Author: Mandeep Singh Baines <[email protected]> Thu Feb 28 09:03:18 2013
>
> lockdep: check that no locks held at freeze time
>
> And the code to trigger this problem is here:
> static int hotkey_kthread(void *data)
> {
> struct tp_nvram_state s[2];
> u32 poll_mask, event_mask;
> unsigned int si, so;
> unsigned long t;
> unsigned int change_detector;
> unsigned int poll_freq;
> bool was_frozen;
>
> mutex_lock(&hotkey_thread_mutex);
>
> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> goto exit;
>
> set_freezable();
> ~~~~~~~~~~~~~~~~~~~~~~~~~~
> in thinkpad_acpi.c.
>
> I don't know much about freeze, I've no idea what is the problem.
> So Mandeep and Henrique, can you please kindly take a look? Thanks.
>
Hi Aaron,
I will take a look at this.
In general, you don't want to grab a lock and freeze. You will block
suspend because another thread may try go grab the lock. In the case
of cgroup freezer, you could cause a deadlock.
There is also discussion happening on this thread:
https://lkml.org/lkml/2013/3/4/221
Regards,
Mandeep
> -Aaron
>
Fixes the following lockdep error:
[ BUG: ktpacpi_nvramd/446 still has locks held! ]
hotkey_kthread() calls set_freezable() after acquiring the
hotkey_kthread_mutex(). set_freezable() calls try_to_freeze().
This could block suspend if we were to freeze at this point
and another task were to block on the mutex, potentially via
writing to one of the sysfs attrs. This race is unlikely but
can be easily fixed by moving the set_freezable() call.
Reported-by: Maciej Rutecki <[email protected]>
Signed-off-by: Mandeep Singh Baines <[email protected]>
CC: Aaron Lu <[email protected]>
CC: Henrique de Moraes Holschuh <[email protected]>
CC: Tejun Heo <[email protected]>
CC: Oleg Nesterov <[email protected]>
CC: Andrew Morton <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9a90756..69870a841 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
unsigned int poll_freq;
bool was_frozen;
+ set_freezable();
+
mutex_lock(&hotkey_thread_mutex);
if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
goto exit;
- set_freezable();
-
so = 0;
si = 1;
t = 0;
--
1.7.12.4
On 03/05, Mandeep Singh Baines wrote:
>
> @@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
> unsigned int poll_freq;
> bool was_frozen;
>
> + set_freezable();
> +
> mutex_lock(&hotkey_thread_mutex);
>
> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> goto exit;
>
> - set_freezable();
> -
I don't understand this code... but don't we have the same problem
with kthread_freezable_should_stop() below? It can call __refrigerator()
too under the same lock.
Oleg.
On Tue, Mar 5, 2013 at 9:48 AM, Oleg Nesterov <[email protected]> wrote:
> On 03/05, Mandeep Singh Baines wrote:
>>
>> @@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
>> unsigned int poll_freq;
>> bool was_frozen;
>>
>> + set_freezable();
>> +
>> mutex_lock(&hotkey_thread_mutex);
>>
>> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
>> goto exit;
>>
>> - set_freezable();
>> -
>
> I don't understand this code... but don't we have the same problem
> with kthread_freezable_should_stop() below? It can call __refrigerator()
> too under the same lock.
>
I don't think the lock is held at that point. There is an unlock right
before entering the while loop and at the bottom of the loop.
> Oleg.
>
On 03/05, Mandeep Singh Baines wrote:
>
> On Tue, Mar 5, 2013 at 9:48 AM, Oleg Nesterov <[email protected]> wrote:
> > On 03/05, Mandeep Singh Baines wrote:
> >>
> >> @@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
> >> unsigned int poll_freq;
> >> bool was_frozen;
> >>
> >> + set_freezable();
> >> +
> >> mutex_lock(&hotkey_thread_mutex);
> >>
> >> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> >> goto exit;
> >>
> >> - set_freezable();
> >> -
> >
> > I don't understand this code... but don't we have the same problem
> > with kthread_freezable_should_stop() below? It can call __refrigerator()
> > too under the same lock.
> >
>
> I don't think the lock is held at that point. There is an unlock right
> before entering the while loop and at the bottom of the loop.
Hmm... Afaics this is another lock, hotkey_thread_data_mutex. But
hotkey_thread_mutex is still held.
Oleg.
On wtorek, 5 marca 2013 o 18:34:43 Mandeep Singh Baines wrote:
> Fixes the following lockdep error:
>
> [ BUG: ktpacpi_nvramd/446 still has locks held! ]
>
> hotkey_kthread() calls set_freezable() after acquiring the
> hotkey_kthread_mutex(). set_freezable() calls try_to_freeze().
> This could block suspend if we were to freeze at this point
> and another task were to block on the mutex, potentially via
> writing to one of the sysfs attrs. This race is unlikely but
> can be easily fixed by moving the set_freezable() call.
>
> Reported-by: Maciej Rutecki <[email protected]>
> Signed-off-by: Mandeep Singh Baines <[email protected]>
> CC: Aaron Lu <[email protected]>
> CC: Henrique de Moraes Holschuh <[email protected]>
> CC: Tejun Heo <[email protected]>
> CC: Oleg Nesterov <[email protected]>
> CC: Andrew Morton <[email protected]>
Tested-by: Maciej Rutecki <[email protected]>
Patch fixes the problem. Thanks!
Regards
--
Maciej Rutecki
http://www.mrutecki.pl
On Tue, Mar 5, 2013 at 10:05 AM, Oleg Nesterov <[email protected]> wrote:
> On 03/05, Mandeep Singh Baines wrote:
>>
>> On Tue, Mar 5, 2013 at 9:48 AM, Oleg Nesterov <[email protected]> wrote:
>> > On 03/05, Mandeep Singh Baines wrote:
>> >>
>> >> @@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
>> >> unsigned int poll_freq;
>> >> bool was_frozen;
>> >>
>> >> + set_freezable();
>> >> +
>> >> mutex_lock(&hotkey_thread_mutex);
>> >>
>> >> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
>> >> goto exit;
>> >>
>> >> - set_freezable();
>> >> -
>> >
>> > I don't understand this code... but don't we have the same problem
>> > with kthread_freezable_should_stop() below? It can call __refrigerator()
>> > too under the same lock.
>> >
>>
>> I don't think the lock is held at that point. There is an unlock right
>> before entering the while loop and at the bottom of the loop.
>
> Hmm... Afaics this is another lock, hotkey_thread_data_mutex. But
> hotkey_thread_mutex is still held.
>
Ah. You're right. The two names were similar so that confused me. I'm
also looking at this code for the first time:)
This mutex seems wrong. Its held the entire time the kthread is
running. I think its used to synchronize on the exit of the kthread. A
completion would more appropriate in that case.
Regards,
Mandeep
> Oleg.
>
On Tue, 5 Mar 2013 09:34:43 -0800 Mandeep Singh Baines <[email protected]> wrote:
> Fixes the following lockdep error:
>
> [ BUG: ktpacpi_nvramd/446 still has locks held! ]
>
> hotkey_kthread() calls set_freezable() after acquiring the
> hotkey_kthread_mutex(). set_freezable() calls try_to_freeze().
> This could block suspend if we were to freeze at this point
> and another task were to block on the mutex, potentially via
> writing to one of the sysfs attrs. This race is unlikely but
> can be easily fixed by moving the set_freezable() call.
>
> ...
>
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -2462,13 +2462,13 @@ static int hotkey_kthread(void *data)
> unsigned int poll_freq;
> bool was_frozen;
>
> + set_freezable();
> +
> mutex_lock(&hotkey_thread_mutex);
>
> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> goto exit;
>
> - set_freezable();
> -
> so = 0;
> si = 1;
> t = 0;
Basically the same as
http://ozlabs.org/~akpm/mmots/broken-out/drivers-platform-x86-thinkpad_acpic-move-hotkey_thread_mutex-lock-after-set_freezable.patch.
I think Artem's patch is a little better. There doesn't appear to be
any locking protocol for tpacpi_lifecycle.
I'll move Artem's patch into my for-3.9-rc2 queue.
On Tue, 05 Mar 2013, Mandeep Singh Baines wrote:
> This mutex seems wrong. Its held the entire time the kthread is
> running. I think its used to synchronize on the exit of the kthread. A
> completion would more appropriate in that case.
>From the top of the driver source:
/* Acquired while the poller kthread is running, use to sync start/stop */
static struct mutex hotkey_thread_mutex;
/*
* Acquire mutex to write poller control variables as an
* atomic block.
*
* Increment hotkey_config_change when changing them if you
* want the kthread to forget old state.
*
* See HOTKEY_CONFIG_CRITICAL_START/HOTKEY_CONFIG_CRITICAL_END
*/
static struct mutex hotkey_thread_data_mutex;
static unsigned int hotkey_config_change;
#define HOTKEY_CONFIG_CRITICAL_START \
do { \
mutex_lock(&hotkey_thread_data_mutex); \
hotkey_config_change++; \
} while (0);
#define HOTKEY_CONFIG_CRITICAL_END \
mutex_unlock(&hotkey_thread_data_mutex);
This can likely be modernized a lot. This code is from 2008, I think it
first shipped in 2.6.25-rc1.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On 03/05, Henrique de Moraes Holschuh wrote:
>
> On Tue, 05 Mar 2013, Mandeep Singh Baines wrote:
> > This mutex seems wrong. Its held the entire time the kthread is
> > running. I think its used to synchronize on the exit of the kthread. A
> > completion would more appropriate in that case.
>
> From the top of the driver source:
>
> /* Acquired while the poller kthread is running, use to sync start/stop */
> static struct mutex hotkey_thread_mutex;
I simply can't understand what this "sync start/stop" means...
Ignoring hotkey_kthread(), the only user is
static void hotkey_poll_stop_sync(void)
{
if (tpacpi_hotkey_task) {
kthread_stop(tpacpi_hotkey_task);
tpacpi_hotkey_task = NULL;
mutex_lock(&hotkey_thread_mutex);
/* at this point, the thread did exit */
mutex_unlock(&hotkey_thread_mutex);
}
}
And I simply do not understand the comment. This thread has already exited
when kthread_stop() returns (OK, it can be running do_exit() paths but this
doesn't matter). So this mutex_lock() buys nothing afaics.
As for serializing with hotkey_poll_setup/etc, looks like this code relies
on hotkey_mutex.
So I think hotkey_thread_mutex can be simply removed?
Oleg.
On 03/05, Andrew Morton wrote:
>
> Basically the same as
> http://ozlabs.org/~akpm/mmots/broken-out/drivers-platform-x86-thinkpad_acpic-move-hotkey_thread_mutex-lock-after-set_freezable.patch.
> I think Artem's patch is a little better. There doesn't appear to be
> any locking protocol for tpacpi_lifecycle.
Which seems to have the same problem, hotkey_kthread() still calls
kthread_freezable_should_stop() under hotkey_thread_mutex.
IOW, we have two try_to_freeze's here, the patch moves only one of
them outside of the hotkey_thread_mutex.
Oleg.
On Wed, Mar 06, 2013 at 04:50:39PM +0100, Oleg Nesterov wrote:
> On 03/05, Andrew Morton wrote:
> >
> > Basically the same as
> > http://ozlabs.org/~akpm/mmots/broken-out/drivers-platform-x86-thinkpad_acpic-move-hotkey_thread_mutex-lock-after-set_freezable.patch.
> > I think Artem's patch is a little better. There doesn't appear to be
> > any locking protocol for tpacpi_lifecycle.
>
> Which seems to have the same problem, hotkey_kthread() still calls
> kthread_freezable_should_stop() under hotkey_thread_mutex.
>
> IOW, we have two try_to_freeze's here, the patch moves only one of
> them outside of the hotkey_thread_mutex.
It's hard for me to judge but this lock does indeed look like it has
been used to block until the thread exits. I'm trying out the "remove
hotkey_thread_mutex completely" approach and everything looks fine so
far.
--
Regards,
Artem
On Wed, 06 Mar 2013, Oleg Nesterov wrote:
> On 03/05, Henrique de Moraes Holschuh wrote:
> > On Tue, 05 Mar 2013, Mandeep Singh Baines wrote:
> > > This mutex seems wrong. Its held the entire time the kthread is
> > > running. I think its used to synchronize on the exit of the kthread. A
> > > completion would more appropriate in that case.
> >
> > From the top of the driver source:
> >
> > /* Acquired while the poller kthread is running, use to sync start/stop */
> > static struct mutex hotkey_thread_mutex;
>
> I simply can't understand what this "sync start/stop" means...
>
> Ignoring hotkey_kthread(), the only user is
>
> static void hotkey_poll_stop_sync(void)
> {
> if (tpacpi_hotkey_task) {
> kthread_stop(tpacpi_hotkey_task);
> tpacpi_hotkey_task = NULL;
> mutex_lock(&hotkey_thread_mutex);
> /* at this point, the thread did exit */
> mutex_unlock(&hotkey_thread_mutex);
> }
> }
>
> And I simply do not understand the comment. This thread has already exited
> when kthread_stop() returns (OK, it can be running do_exit() paths but this
> doesn't matter). So this mutex_lock() buys nothing afaics.
It was added due to an oops, waaaaay back then. If it is not needed
anymore, and there is zero chance of the kthread still being active when
hotkey_poll_stop_sync() ends, hotkey_thread_mutex can be simply removed.
Note that hotkey_thread_data_mutex is still required.
> As for serializing with hotkey_poll_setup/etc, looks like this code relies
> on hotkey_mutex.
>
> So I think hotkey_thread_mutex can be simply removed?
Looks like it, if the current semanthics of ktread_stop() are syncronous.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On 03/06, Henrique de Moraes Holschuh wrote:
>
> On Wed, 06 Mar 2013, Oleg Nesterov wrote:
> >
> > static void hotkey_poll_stop_sync(void)
> > {
> > if (tpacpi_hotkey_task) {
> > kthread_stop(tpacpi_hotkey_task);
> > tpacpi_hotkey_task = NULL;
> > mutex_lock(&hotkey_thread_mutex);
> > /* at this point, the thread did exit */
> > mutex_unlock(&hotkey_thread_mutex);
> > }
> > }
> >
> > And I simply do not understand the comment. This thread has already exited
> > when kthread_stop() returns (OK, it can be running do_exit() paths but this
> > doesn't matter). So this mutex_lock() buys nothing afaics.
>
> It was added due to an oops, waaaaay back then. If it is not needed
> anymore, and there is zero chance of the kthread still being active when
> hotkey_poll_stop_sync() ends, hotkey_thread_mutex can be simply removed.
Well, there could be another bug. Say, hotkey_poll_stop_sync() can block
on hotkey_thread_mutex if another thread was started. But at first glance
this can't happen (hotkey_mutex), and even _if_ it can this needs another
fix.
> Looks like it, if the current semanthics of ktread_stop() are syncronous.
IIRC, it always was... But at least currently it is certainly syncronous.
kthread_stop(t) does wait_for_completion(t->vfork_done), complete(vfork_done)
can't happen unless this task calls do_exit().
Hmm. I just noticed that the recent changes in kthread_stop() are not correct...
But this is offtopic and doesn't affect thinkpad_acpi.c, I'll write another
email later.
So, what do you think about (UNTESTED) 1/1 ?
Oleg.
hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
We can simply kill this mutex, hotkey_poll_stop_sync() does not need
to serialize with hotkey_kthread(). When kthread_stop() returns the
thread is already dead, it called do_exit()->complete_vfork_done().
Reported-by: Artem Savkov <[email protected]>
Reported-by: Maciej Rutecki <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
--- x/drivers/platform/x86/thinkpad_acpi.c
+++ x/drivers/platform/x86/thinkpad_acpi.c
@@ -1965,9 +1965,6 @@ struct tp_nvram_state {
/* kthread for the hotkey poller */
static struct task_struct *tpacpi_hotkey_task;
-/* Acquired while the poller kthread is running, use to sync start/stop */
-static struct mutex hotkey_thread_mutex;
-
/*
* Acquire mutex to write poller control variables as an
* atomic block.
@@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
unsigned int poll_freq;
bool was_frozen;
- mutex_lock(&hotkey_thread_mutex);
-
if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
goto exit;
@@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
}
exit:
- mutex_unlock(&hotkey_thread_mutex);
return 0;
}
@@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
if (tpacpi_hotkey_task) {
kthread_stop(tpacpi_hotkey_task);
tpacpi_hotkey_task = NULL;
- mutex_lock(&hotkey_thread_mutex);
- /* at this point, the thread did exit */
- mutex_unlock(&hotkey_thread_mutex);
}
}
@@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
mutex_init(&hotkey_mutex);
#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
- mutex_init(&hotkey_thread_mutex);
mutex_init(&hotkey_thread_data_mutex);
#endif
On Thu, Mar 7, 2013 at 9:53 AM, Oleg Nesterov <[email protected]> wrote:
> hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
>
> We can simply kill this mutex, hotkey_poll_stop_sync() does not need
> to serialize with hotkey_kthread(). When kthread_stop() returns the
> thread is already dead, it called do_exit()->complete_vfork_done().
>
> Reported-by: Artem Savkov <[email protected]>
> Reported-by: Maciej Rutecki <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
>
Reviewed-by: Mandeep Singh Baines <[email protected]>
> --- x/drivers/platform/x86/thinkpad_acpi.c
> +++ x/drivers/platform/x86/thinkpad_acpi.c
> @@ -1965,9 +1965,6 @@ struct tp_nvram_state {
> /* kthread for the hotkey poller */
> static struct task_struct *tpacpi_hotkey_task;
>
> -/* Acquired while the poller kthread is running, use to sync start/stop */
> -static struct mutex hotkey_thread_mutex;
> -
> /*
> * Acquire mutex to write poller control variables as an
> * atomic block.
> @@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
> unsigned int poll_freq;
> bool was_frozen;
>
> - mutex_lock(&hotkey_thread_mutex);
> -
> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> goto exit;
>
> @@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
> }
>
> exit:
> - mutex_unlock(&hotkey_thread_mutex);
> return 0;
> }
>
> @@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
> if (tpacpi_hotkey_task) {
> kthread_stop(tpacpi_hotkey_task);
> tpacpi_hotkey_task = NULL;
> - mutex_lock(&hotkey_thread_mutex);
> - /* at this point, the thread did exit */
> - mutex_unlock(&hotkey_thread_mutex);
> }
> }
>
> @@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
> mutex_init(&hotkey_mutex);
>
> #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> - mutex_init(&hotkey_thread_mutex);
> mutex_init(&hotkey_thread_data_mutex);
> #endif
>
>
On Thu, 07 Mar 2013, Mandeep Singh Baines wrote:
> On Thu, Mar 7, 2013 at 9:53 AM, Oleg Nesterov <[email protected]> wrote:
> > hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
> >
> > We can simply kill this mutex, hotkey_poll_stop_sync() does not need
> > to serialize with hotkey_kthread(). When kthread_stop() returns the
> > thread is already dead, it called do_exit()->complete_vfork_done().
> >
> > Reported-by: Artem Savkov <[email protected]>
> > Reported-by: Maciej Rutecki <[email protected]>
> > Signed-off-by: Oleg Nesterov <[email protected]>
> >
>
> Reviewed-by: Mandeep Singh Baines <[email protected]>
Acked-by: Henrique de Moraes Holschuh <[email protected]>
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Thursday, March 07, 2013 06:53:59 PM Oleg Nesterov wrote:
> hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
>
> We can simply kill this mutex, hotkey_poll_stop_sync() does not need
> to serialize with hotkey_kthread(). When kthread_stop() returns the
> thread is already dead, it called do_exit()->complete_vfork_done().
>
> Reported-by: Artem Savkov <[email protected]>
> Reported-by: Maciej Rutecki <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
I wonder if anyone is taking care of this patch?
Rafael
> --- x/drivers/platform/x86/thinkpad_acpi.c
> +++ x/drivers/platform/x86/thinkpad_acpi.c
> @@ -1965,9 +1965,6 @@ struct tp_nvram_state {
> /* kthread for the hotkey poller */
> static struct task_struct *tpacpi_hotkey_task;
>
> -/* Acquired while the poller kthread is running, use to sync start/stop */
> -static struct mutex hotkey_thread_mutex;
> -
> /*
> * Acquire mutex to write poller control variables as an
> * atomic block.
> @@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
> unsigned int poll_freq;
> bool was_frozen;
>
> - mutex_lock(&hotkey_thread_mutex);
> -
> if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> goto exit;
>
> @@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
> }
>
> exit:
> - mutex_unlock(&hotkey_thread_mutex);
> return 0;
> }
>
> @@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
> if (tpacpi_hotkey_task) {
> kthread_stop(tpacpi_hotkey_task);
> tpacpi_hotkey_task = NULL;
> - mutex_lock(&hotkey_thread_mutex);
> - /* at this point, the thread did exit */
> - mutex_unlock(&hotkey_thread_mutex);
> }
> }
>
> @@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
> mutex_init(&hotkey_mutex);
>
> #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> - mutex_init(&hotkey_thread_mutex);
> mutex_init(&hotkey_thread_data_mutex);
> #endif
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 03/26, Rafael J. Wysocki wrote:
>
> On Thursday, March 07, 2013 06:53:59 PM Oleg Nesterov wrote:
> > hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
> >
> > We can simply kill this mutex, hotkey_poll_stop_sync() does not need
> > to serialize with hotkey_kthread(). When kthread_stop() returns the
> > thread is already dead, it called do_exit()->complete_vfork_done().
> >
> > Reported-by: Artem Savkov <[email protected]>
> > Reported-by: Maciej Rutecki <[email protected]>
> > Signed-off-by: Oleg Nesterov <[email protected]>
>
> I wonder if anyone is taking care of this patch?
IIRC, it is already in -mm tree.
> Rafael
>
>
> > --- x/drivers/platform/x86/thinkpad_acpi.c
> > +++ x/drivers/platform/x86/thinkpad_acpi.c
> > @@ -1965,9 +1965,6 @@ struct tp_nvram_state {
> > /* kthread for the hotkey poller */
> > static struct task_struct *tpacpi_hotkey_task;
> >
> > -/* Acquired while the poller kthread is running, use to sync start/stop */
> > -static struct mutex hotkey_thread_mutex;
> > -
> > /*
> > * Acquire mutex to write poller control variables as an
> > * atomic block.
> > @@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
> > unsigned int poll_freq;
> > bool was_frozen;
> >
> > - mutex_lock(&hotkey_thread_mutex);
> > -
> > if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> > goto exit;
> >
> > @@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
> > }
> >
> > exit:
> > - mutex_unlock(&hotkey_thread_mutex);
> > return 0;
> > }
> >
> > @@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
> > if (tpacpi_hotkey_task) {
> > kthread_stop(tpacpi_hotkey_task);
> > tpacpi_hotkey_task = NULL;
> > - mutex_lock(&hotkey_thread_mutex);
> > - /* at this point, the thread did exit */
> > - mutex_unlock(&hotkey_thread_mutex);
> > }
> > }
> >
> > @@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
> > mutex_init(&hotkey_mutex);
> >
> > #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > - mutex_init(&hotkey_thread_mutex);
> > mutex_init(&hotkey_thread_data_mutex);
> > #endif
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
On Tuesday, March 26, 2013 02:09:19 PM Oleg Nesterov wrote:
> On 03/26, Rafael J. Wysocki wrote:
> >
> > On Thursday, March 07, 2013 06:53:59 PM Oleg Nesterov wrote:
> > > hotkey_kthread() does try_to_freeze() under hotkey_thread_mutex.
> > >
> > > We can simply kill this mutex, hotkey_poll_stop_sync() does not need
> > > to serialize with hotkey_kthread(). When kthread_stop() returns the
> > > thread is already dead, it called do_exit()->complete_vfork_done().
> > >
> > > Reported-by: Artem Savkov <[email protected]>
> > > Reported-by: Maciej Rutecki <[email protected]>
> > > Signed-off-by: Oleg Nesterov <[email protected]>
> >
> > I wonder if anyone is taking care of this patch?
>
> IIRC, it is already in -mm tree.
OK, thanks for letting me know.
> > > --- x/drivers/platform/x86/thinkpad_acpi.c
> > > +++ x/drivers/platform/x86/thinkpad_acpi.c
> > > @@ -1965,9 +1965,6 @@ struct tp_nvram_state {
> > > /* kthread for the hotkey poller */
> > > static struct task_struct *tpacpi_hotkey_task;
> > >
> > > -/* Acquired while the poller kthread is running, use to sync start/stop */
> > > -static struct mutex hotkey_thread_mutex;
> > > -
> > > /*
> > > * Acquire mutex to write poller control variables as an
> > > * atomic block.
> > > @@ -2463,8 +2460,6 @@ static int hotkey_kthread(void *data)
> > > unsigned int poll_freq;
> > > bool was_frozen;
> > >
> > > - mutex_lock(&hotkey_thread_mutex);
> > > -
> > > if (tpacpi_lifecycle == TPACPI_LIFE_EXITING)
> > > goto exit;
> > >
> > > @@ -2524,7 +2519,6 @@ static int hotkey_kthread(void *data)
> > > }
> > >
> > > exit:
> > > - mutex_unlock(&hotkey_thread_mutex);
> > > return 0;
> > > }
> > >
> > > @@ -2534,9 +2528,6 @@ static void hotkey_poll_stop_sync(void)
> > > if (tpacpi_hotkey_task) {
> > > kthread_stop(tpacpi_hotkey_task);
> > > tpacpi_hotkey_task = NULL;
> > > - mutex_lock(&hotkey_thread_mutex);
> > > - /* at this point, the thread did exit */
> > > - mutex_unlock(&hotkey_thread_mutex);
> > > }
> > > }
> > >
> > > @@ -3235,7 +3226,6 @@ static int __init hotkey_init(struct ibm
> > > mutex_init(&hotkey_mutex);
> > >
> > > #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > > - mutex_init(&hotkey_thread_mutex);
> > > mutex_init(&hotkey_thread_data_mutex);
> > > #endif
> > >
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.