[REGRESSION] [3.9-rc1] BUG: ktpacpi_nvramd/446 still has locks held!

[REGRESSION] [3.9-rc1] BUG: ktpacpi_nvramd/446 still has locks held!

[Maciej Rutecki]

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

Re: [REGRESSION] [3.9-rc1] BUG: ktpacpi_nvramd/446 still has locks held!

[Aaron Lu]

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 <msb@chromium.org>  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

Re: [REGRESSION] [3.9-rc1] BUG: ktpacpi_nvramd/446 still has locks held!

[Mandeep Singh Baines]

On Mon, Mar 4, 2013 at 11:28 PM, Aaron Lu <aaron.lu@intel.com> 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 <msb@chromium.org>  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
>

[PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Mandeep Singh Baines]

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 <maciej.rutecki@gmail.com>
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Aaron Lu <aaron.lu@intel.com>
CC: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
CC: Tejun Heo <tj@kernel.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Andrew Morton <akpm@linux-foundation.org>
---
 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

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Oleg Nesterov]

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.

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Mandeep Singh Baines]

On Tue, Mar 5, 2013 at 9:48 AM, Oleg Nesterov <oleg@redhat.com> 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.
>

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Oleg Nesterov]

On 03/05, Mandeep Singh Baines wrote:
>
> On Tue, Mar 5, 2013 at 9:48 AM, Oleg Nesterov <oleg@redhat.com> 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.

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Maciej Rutecki]

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 <maciej.rutecki@gmail.com>
> Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
> CC: Aaron Lu <aaron.lu@intel.com>
> CC: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> CC: Tejun Heo <tj@kernel.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Patch fixes the problem. Thanks!

Regards

-- 
Maciej Rutecki
http://www.mrutecki.pl

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Mandeep Singh Baines]

On Tue, Mar 5, 2013 at 10:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/05, Mandeep Singh Baines wrote:
>>
>> On Tue, Mar 5, 2013 at 9:48 AM, Oleg Nesterov <oleg@redhat.com> 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.
>

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Andrew Morton]

On Tue,  5 Mar 2013 09:34:43 -0800 Mandeep Singh Baines <msb@chromium.org> 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.

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Henrique de Moraes Holschuh]

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

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Oleg Nesterov]

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.

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Oleg Nesterov]

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.

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Artem Savkov]

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

Re: [PATCH] thinkpad-acpi: fix potential suspend blocking issue

[Henrique de Moraes Holschuh]

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

[PATCH 0/1] thinkpad-acpi: kill hotkey_thread_mutex

[Oleg Nesterov]

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.

[PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

[Oleg Nesterov]

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 <artem.savkov@gmail.com>
Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 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
 

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

[Mandeep Singh Baines]

On Thu, Mar 7, 2013 at 9:53 AM, Oleg Nesterov <oleg@redhat.com> 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 <artem.savkov@gmail.com>
> Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>

Reviewed-by: Mandeep Singh Baines <msb@chromium.org>

> --- 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
>
>

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

[Henrique de Moraes Holschuh]

On Thu, 07 Mar 2013, Mandeep Singh Baines wrote:
> On Thu, Mar 7, 2013 at 9:53 AM, Oleg Nesterov <oleg@redhat.com> 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 <artem.savkov@gmail.com>
> > Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> >
> 
> Reviewed-by: Mandeep Singh Baines <msb@chromium.org>

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  "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

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

[Oleg Nesterov]

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 <artem.savkov@gmail.com>
> > Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

[Rafael J. Wysocki]

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 <artem.savkov@gmail.com>
> Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/1] thinkpad-acpi: kill hotkey_thread_mutex

[Rafael J. Wysocki]

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 <artem.savkov@gmail.com>
> > > Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> >
> > 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 majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.