[PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Rakib Mullick]

Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status, 
acpi_os_acquire_lock holds the lock acpi_gbl_hardware_lock before calling acpi_hw_register_write(), then 
without releasing acpi_gbl_hardware_lock, this function calls acpi_ev_walk_gpe_list, which tries to hold 
acpi_gbl_gpe_lock and thus might causes possible recursive lock.

Following patch fixes this scenario by just releasing acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list.

 Changes since v0(https://lkml.org/lkml/2011/9/21/355):
	- Fix changelog, thanks to Lin Ming.

Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
Cc: Lin Ming <ming.m.lin@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Len Brown <len.brown@intel.com>
---

diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
index 55accb7..e3110ac 100644
--- a/drivers/acpi/acpica/hwregs.c
+++ b/drivers/acpi/acpica/hwregs.c
@@ -269,6 +269,9 @@ acpi_status acpi_hw_clear_acpi_status(void)
 
 	status = acpi_hw_register_write(ACPI_REGISTER_PM1_STATUS,
 					ACPI_BITMASK_ALL_FIXED_STATUS);
+
+	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
+
 	if (ACPI_FAILURE(status)) {
 		goto unlock_and_exit;
 	}
@@ -278,7 +281,6 @@ acpi_status acpi_hw_clear_acpi_status(void)
 	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
 
       unlock_and_exit:
-	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
 	return_ACPI_STATUS(status);
 }
 

Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Lin Ming]

On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status, 

As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
there is a recursive lock.

Or do you have a real test case to trigger the recursive lock you
mentioned?

Lin Ming

> acpi_os_acquire_lock holds the lock acpi_gbl_hardware_lock before calling acpi_hw_register_write(), then 
> without releasing acpi_gbl_hardware_lock, this function calls acpi_ev_walk_gpe_list, which tries to hold 
> acpi_gbl_gpe_lock and thus might causes possible recursive lock.
> 
> Following patch fixes this scenario by just releasing acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list.
> 
>  Changes since v0(https://lkml.org/lkml/2011/9/21/355):
> 	- Fix changelog, thanks to Lin Ming.
> 
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> Cc: Lin Ming <ming.m.lin@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Len Brown <len.brown@intel.com>
> ---
> 
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index 55accb7..e3110ac 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -269,6 +269,9 @@ acpi_status acpi_hw_clear_acpi_status(void)
>  
>  	status = acpi_hw_register_write(ACPI_REGISTER_PM1_STATUS,
>  					ACPI_BITMASK_ALL_FIXED_STATUS);
> +
> +	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
> +
>  	if (ACPI_FAILURE(status)) {
>  		goto unlock_and_exit;
>  	}
> @@ -278,7 +281,6 @@ acpi_status acpi_hw_clear_acpi_status(void)
>  	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>  
>        unlock_and_exit:
> -	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
>  	return_ACPI_STATUS(status);
>  }
>  
> 
> 

Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Rakib Mullick]

On 11/3/11, Lin Ming <ming.m.lin@intel.com> wrote:
> On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
>> Calling pm-suspend might trigger a recursive lock in it's code path. In
>> function acpi_hw_clear_acpi_status,
>
> As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
> there is a recursive lock.
>
> Or do you have a real test case to trigger the recursive lock you
> mentioned?
>
As I said before, recursive lock trigger's on a modified kernel (when
calling pm-suspend), not with the mainline kernel. If you think that,
since it's not happening with original kernel, so we shouldn't go for
fix - that's a different thing, I don't have any argue. But, the issue
is on.

Thanks,
Rakib

Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Srivatsa S. Bhat]

Hi,

On 11/03/2011 05:32 PM, Lin Ming wrote:
> On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
>> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status, 
> 
> As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
> there is a recursive lock.
> 

At first look, it definitely doesn't look like a recursive lock, as Lin said.
But, quoting Documentation/lockdep-design.txt:

"Multi-lock dependency rules:
----------------------------

The same lock-class must not be acquired twice, because this could lead
to lock recursion deadlocks."

So, Rakib, do the 2 locks belong to the same lock-class? If yes, then I think
that is the reason for the lockdep splat. Could you show the lockdep warning?

By the way, another way to look at this patch is as an optimization..
i.e., if acpi_gbl_hardware_lock doesn't need to be held to call
acpi_ev_walk_gpe_list(), then we can move from the coarse-grained locking
to finer-grained locking by releasing it earlier, as you did in your patch.
[Note that you will have to update the goto label also, i.e., rename it as
'exit' or something like that]

>> acpi_os_acquire_lock holds the lock acpi_gbl_hardware_lock before calling acpi_hw_register_write(), then 
>> without releasing acpi_gbl_hardware_lock, this function calls acpi_ev_walk_gpe_list, which tries to hold 
>> acpi_gbl_gpe_lock and thus might causes possible recursive lock.
>>
>> Following patch fixes this scenario by just releasing acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list.
>>
>>  Changes since v0(https://lkml.org/lkml/2011/9/21/355):
>> 	- Fix changelog, thanks to Lin Ming.
>>
>> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
>> Cc: Lin Ming <ming.m.lin@intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Len Brown <len.brown@intel.com>
>> ---
>>
>> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
>> index 55accb7..e3110ac 100644
>> --- a/drivers/acpi/acpica/hwregs.c
>> +++ b/drivers/acpi/acpica/hwregs.c
>> @@ -269,6 +269,9 @@ acpi_status acpi_hw_clear_acpi_status(void)
>>  
>>  	status = acpi_hw_register_write(ACPI_REGISTER_PM1_STATUS,
>>  					ACPI_BITMASK_ALL_FIXED_STATUS);
>> +
>> +	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
>> +
>>  	if (ACPI_FAILURE(status)) {
>>  		goto unlock_and_exit;
>>  	}
>> @@ -278,7 +281,6 @@ acpi_status acpi_hw_clear_acpi_status(void)
>>  	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>  
>>        unlock_and_exit:
>> -	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
>>  	return_ACPI_STATUS(status);
>>  }
>>  
>>


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Rakib Mullick]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Thu, Nov 3, 2011 at 11:40 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Hi,
>
> On 11/03/2011 05:32 PM, Lin Ming wrote:
>> On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
>>> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
>>
>> As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
>> there is a recursive lock.
>>
>
> At first look, it definitely doesn't look like a recursive lock, as Lin said.
> But, quoting Documentation/lockdep-design.txt:
>
> "Multi-lock dependency rules:
> ----------------------------
>
> The same lock-class must not be acquired twice, because this could lead
> to lock recursion deadlocks."
>
> So, Rakib, do the 2 locks belong to the same lock-class? If yes, then I think
> that is the reason for the lockdep splat. Could you show the lockdep warning?
>
Yes, same lock-class. And as per "Multi-lock dependency rules:", it
leads to lock recursion deadlocks.
Lockdep warning attached.

> By the way, another way to look at this patch is as an optimization..
> i.e., if acpi_gbl_hardware_lock doesn't need to be held to call
> acpi_ev_walk_gpe_list(), then we can move from the coarse-grained locking
> to finer-grained locking by releasing it earlier, as you did in your patch.
> [Note that you will have to update the goto label also, i.e., rename it as
> 'exit' or something like that]
>
I can do it, thanks for suggestions. But, what does Lin thinks? Lin
are you okay?

Thanks,
Rakib

[-- Attachment #2: lockdepwarning --]
[-- Type: application/octet-stream, Size: 3521 bytes --]

[ 5387.734079] =============================================
[ 5387.734082] [ INFO: possible recursive locking detected ]
[ 5387.734085] 3.0.0-rc3-tip-mr.barber+ #4
[ 5387.734087] ---------------------------------------------
[ 5387.734089] pm-suspend/9600 is trying to acquire lock:
[ 5387.734092]  (&(lock)->rlock){-.-...}, at: [<ffffffff8128e87d>] acpi_os_acquire_lock+0xe/0x10
[ 5387.734103] 
[ 5387.734103] but task is already holding lock:
[ 5387.734105]  (&(lock)->rlock){-.-...}, at: [<ffffffff8128e87d>] acpi_os_acquire_lock+0xe/0x10
[ 5387.734110] 
[ 5387.734111] other info that might help us debug this:
[ 5387.734113]  Possible unsafe locking scenario:
[ 5387.734114] 
[ 5387.734115]        CPU0
[ 5387.734117]        ----
[ 5387.734119]   lock(&(lock)->rlock);
[ 5387.734121]   lock(&(lock)->rlock);
[ 5387.734124] 
[ 5387.734125]  *** DEADLOCK ***
[ 5387.734126] 
[ 5387.734127]  May be due to missing lock nesting notation
[ 5387.734128] 
[ 5387.734130] 4 locks held by pm-suspend/9600:
[ 5387.734131]  #0:  (&buffer->mutex){+.+.+.}, at: [<ffffffff81186d0d>] sysfs_write_file+0x3c/0x15d
[ 5387.734141]  #1:  (s_active#89){.+.+.+}, at: [<ffffffff81186dd1>] sysfs_write_file+0x100/0x15d
[ 5387.734148]  #2:  (pm_mutex){+.+.+.}, at: [<ffffffff8108ed4e>] enter_state+0x39/0x13c
[ 5387.734156]  #3:  (&(lock)->rlock){-.-...}, at: [<ffffffff8128e87d>] acpi_os_acquire_lock+0xe/0x10
[ 5387.734162] 
[ 5387.734162] stack backtrace:
[ 5387.734166] Pid: 9600, comm: pm-suspend Not tainted 3.0.0-rc3-tip-mr.barber+ #4
[ 5387.734168] Call Trace:
[ 5387.734175]  [<ffffffff810810a4>] __lock_acquire+0x9a2/0xdc1
[ 5387.734180]  [<ffffffff8108022a>] ? check_irq_usage+0x78/0x89
[ 5387.734183]  [<ffffffff8128e87d>] ? acpi_os_acquire_lock+0xe/0x10
[ 5387.734187]  [<ffffffff8108196e>] lock_acquire+0xce/0xf9
[ 5387.734190]  [<ffffffff8128e87d>] ? acpi_os_acquire_lock+0xe/0x10
[ 5387.734195]  [<ffffffff812a653c>] ? acpi_hw_enable_runtime_gpe_block+0x48/0x48
[ 5387.734199]  [<ffffffff812a653c>] ? acpi_hw_enable_runtime_gpe_block+0x48/0x48
[ 5387.734207]  [<ffffffff815de275>] _raw_spin_lock_irqsave+0x3f/0x52
[ 5387.734210]  [<ffffffff8128e87d>] ? acpi_os_acquire_lock+0xe/0x10
[ 5387.734214]  [<ffffffff8128e87d>] acpi_os_acquire_lock+0xe/0x10
[ 5387.734220]  [<ffffffff8129facc>] acpi_ev_walk_gpe_list+0x28/0x92
[ 5387.734224]  [<ffffffff812a6c31>] acpi_hw_clear_acpi_status+0x3e/0x5d
[ 5387.734227]  [<ffffffff812a700c>] acpi_enter_sleep_state+0x90/0x1cd
[ 5387.734234]  [<ffffffff810119d2>] ? sched_clock+0x9/0xd
[ 5387.734238]  [<ffffffff81011eae>] ? save_sched_clock_state+0x12/0x1b
[ 5387.734244]  [<ffffffff8146f30e>] ? save_processor_state+0x19e/0x1ae
[ 5387.734248]  [<ffffffff815de84d>] ? _raw_spin_unlock_irqrestore+0x3e/0x52
[ 5387.734253]  [<ffffffff81022aaa>] do_suspend_lowlevel+0x9a/0x9c
[ 5387.734257]  [<ffffffff81022997>] ? acpi_suspend_lowlevel+0x1b7/0x1d0
[ 5387.734262]  [<ffffffff8128fb10>] acpi_suspend_enter+0x37/0x96
[ 5387.734266]  [<ffffffff8108ec60>] suspend_devices_and_enter+0x15e/0x213
[ 5387.734270]  [<ffffffff8108edfb>] enter_state+0xe6/0x13c
[ 5387.734273]  [<ffffffff8108e3cb>] state_store+0xae/0xcb
[ 5387.734277]  [<ffffffff81186dd1>] ? sysfs_write_file+0x100/0x15d
[ 5387.734283]  [<ffffffff81249ddb>] kobj_attr_store+0x17/0x19
[ 5387.734287]  [<ffffffff81186df2>] sysfs_write_file+0x121/0x15d
[ 5387.734293]  [<ffffffff81125de4>] vfs_write+0xe6/0x13d
[ 5387.734296]  [<ffffffff8112602d>] sys_write+0x4a/0x6e
[ 5387.734302]  [<ffffffff815e5a02>] system_call_fastpath+0x16/0x1b

Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Lin Ming]

On Fri, 2011-11-04 at 13:53 +0800, Rakib Mullick wrote:
> On Thu, Nov 3, 2011 at 11:40 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> > Hi,
> >
> > On 11/03/2011 05:32 PM, Lin Ming wrote:
> >> On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
> >>> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
> >>
> >> As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
> >> there is a recursive lock.
> >>
> >
> > At first look, it definitely doesn't look like a recursive lock, as Lin said.
> > But, quoting Documentation/lockdep-design.txt:
> >
> > "Multi-lock dependency rules:
> > ----------------------------
> >
> > The same lock-class must not be acquired twice, because this could lead
> > to lock recursion deadlocks."
> >
> > So, Rakib, do the 2 locks belong to the same lock-class? If yes, then I think
> > that is the reason for the lockdep splat. Could you show the lockdep warning?
> >
> Yes, same lock-class. And as per "Multi-lock dependency rules:", it
> leads to lock recursion deadlocks.
> Lockdep warning attached.
> 
> > By the way, another way to look at this patch is as an optimization..
> > i.e., if acpi_gbl_hardware_lock doesn't need to be held to call
> > acpi_ev_walk_gpe_list(), then we can move from the coarse-grained locking
> > to finer-grained locking by releasing it earlier, as you did in your patch.
> > [Note that you will have to update the goto label also, i.e., rename it as
> > 'exit' or something like that]
> >
> I can do it, thanks for suggestions. But, what does Lin thinks? Lin
> are you okay?

I'm OK.

We need to figure out why the dead lock happens.
Could you also paste the patch which trigger this dead lock?

Thanks,
Lin Ming

> 
> Thanks,
> Rakib

Re: [PATCH -v1] acpi: Fix possible recursive locking in hwregs.c

[Rakib Mullick]

On Sat, Nov 5, 2011 at 8:50 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Fri, 2011-11-04 at 13:53 +0800, Rakib Mullick wrote:
>> On Thu, Nov 3, 2011 at 11:40 PM, Srivatsa S. Bhat
>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> > Hi,
>> >
>> > On 11/03/2011 05:32 PM, Lin Ming wrote:
>> >> On Thu, 2011-11-03 at 18:48 +0800, Rakib Mullick wrote:
>> >>> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
>> >>
>> >> As I replied at https://lkml.org/lkml/2011/9/22/6, I still don't think
>> >> there is a recursive lock.
>> >>
>> >
>> > At first look, it definitely doesn't look like a recursive lock, as Lin said.
>> > But, quoting Documentation/lockdep-design.txt:
>> >
>> > "Multi-lock dependency rules:
>> > ----------------------------
>> >
>> > The same lock-class must not be acquired twice, because this could lead
>> > to lock recursion deadlocks."
>> >
>> > So, Rakib, do the 2 locks belong to the same lock-class? If yes, then I think
>> > that is the reason for the lockdep splat. Could you show the lockdep warning?
>> >
>> Yes, same lock-class. And as per "Multi-lock dependency rules:", it
>> leads to lock recursion deadlocks.
>> Lockdep warning attached.
>>
>> > By the way, another way to look at this patch is as an optimization..
>> > i.e., if acpi_gbl_hardware_lock doesn't need to be held to call
>> > acpi_ev_walk_gpe_list(), then we can move from the coarse-grained locking
>> > to finer-grained locking by releasing it earlier, as you did in your patch.
>> > [Note that you will have to update the goto label also, i.e., rename it as
>> > 'exit' or something like that]
>> >
>> I can do it, thanks for suggestions. But, what does Lin thinks? Lin
>> are you okay?
>
> I'm OK.
>
Thanks. Then, I'll come up with a follow up patch includes the
suggestion from Srivatsa.

> We need to figure out why the dead lock happens.

I think its pretty clear from the lockdep warning, the reason of the
possible (since not happened with mainline kernel) dead lock.

> Could you also paste the patch which trigger this dead lock?
>
It's basically a modification of kernel scheduler, it does load
distribution differently. Since it's different approach and needs to
introduce properly. I wish I'll show it some day, if I can make some
fair improvement. Hope you understand.

Thanks,
Rakib