* [PATCH] acpi: Fix possible recursive locking in hwregs.c
@ 2011-09-21 18:09 Rakib Mullick
2011-09-22 0:59 ` Lin Ming
0 siblings, 1 reply; 5+ messages in thread
From: Rakib Mullick @ 2011-09-21 18:09 UTC (permalink / raw)
To: linux-kernel; +Cc: Len Brown, Andrew Morton, linux-acpi
Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
acpi_os_release_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 also tries to
hold acpi_gbl_hardware_lock and thus causes possible recursive lock.
Following patch fixes this scenario by just releasing acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list.
[This patch was created against 3.0-rc3. Since kernel.org is down, I don't have the updated tree. So, before applying
it might require some adjustment.]
Signed-off-by: Rakib Mullick <rakib.mullick@gmail.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);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] acpi: Fix possible recursive locking in hwregs.c
2011-09-21 18:09 [PATCH] acpi: Fix possible recursive locking in hwregs.c Rakib Mullick
@ 2011-09-22 0:59 ` Lin Ming
2011-09-22 3:35 ` Rakib Mullick
0 siblings, 1 reply; 5+ messages in thread
From: Lin Ming @ 2011-09-22 0:59 UTC (permalink / raw)
To: Rakib Mullick
Cc: linux-kernel, Len Brown, Andrew Morton, linux-acpi, Bob Moore
On Thu, Sep 22, 2011 at 2:09 AM, Rakib Mullick <rakib.mullick@gmail.com> wrote:
> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
> acpi_os_release_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 also tries to
> hold acpi_gbl_hardware_lock and thus causes possible recursive lock.
No, acpi_ev_walk_gpe_list holds acpi_gbl_gpe_lock.
Lin Ming
>
> Following patch fixes this scenario by just releasing acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list.
>
> [This patch was created against 3.0-rc3. Since kernel.org is down, I don't have the updated tree. So, before applying
> it might require some adjustment.]
>
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.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);
> }
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] acpi: Fix possible recursive locking in hwregs.c
2011-09-22 0:59 ` Lin Ming
@ 2011-09-22 3:35 ` Rakib Mullick
2011-09-22 4:28 ` Lin Ming
0 siblings, 1 reply; 5+ messages in thread
From: Rakib Mullick @ 2011-09-22 3:35 UTC (permalink / raw)
To: Lin Ming; +Cc: linux-kernel, Len Brown, Andrew Morton, linux-acpi, Bob Moore
On Thu, Sep 22, 2011 at 6:59 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Thu, Sep 22, 2011 at 2:09 AM, Rakib Mullick <rakib.mullick@gmail.com> wrote:
>> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
>> acpi_os_release_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 also tries to
>> hold acpi_gbl_hardware_lock and thus causes possible recursive lock.
>
> No, acpi_ev_walk_gpe_list holds acpi_gbl_gpe_lock.
>
Yes, right. Actually, acpi_os_release_lock() tries to hold
acpi_gbl_gpe_lock without releasing acpi_gbl_hardware_lock. Right?
Thanks,
Rakib
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] acpi: Fix possible recursive locking in hwregs.c
2011-09-22 3:35 ` Rakib Mullick
@ 2011-09-22 4:28 ` Lin Ming
2011-09-22 5:14 ` Rakib Mullick
0 siblings, 1 reply; 5+ messages in thread
From: Lin Ming @ 2011-09-22 4:28 UTC (permalink / raw)
To: Rakib Mullick
Cc: linux-kernel@vger.kernel.org, Brown, Len, Andrew Morton,
linux-acpi@vger.kernel.org, Moore, Robert
On Thu, 2011-09-22 at 11:35 +0800, Rakib Mullick wrote:
> On Thu, Sep 22, 2011 at 6:59 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> > On Thu, Sep 22, 2011 at 2:09 AM, Rakib Mullick <rakib.mullick@gmail.com> wrote:
> >> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
> >> acpi_os_release_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 also tries to
> >> hold acpi_gbl_hardware_lock and thus causes possible recursive lock.
> >
> > No, acpi_ev_walk_gpe_list holds acpi_gbl_gpe_lock.
> >
> Yes, right. Actually, acpi_os_release_lock() tries to hold
> acpi_gbl_gpe_lock without releasing acpi_gbl_hardware_lock. Right?
Do you mean "acpi_ev_walk_gpe_list() tries to hold
acpi_gbl_gpe_lock..."?
We have below locks sequence. I don't see the problem.
acpi_hw_clear_acpi_status:
<acquire acpi_gbl_hardware_lock>
acpi_ev_walk_gpe_list:
<acquire acpi_gbl_gpe_lock>
...
<release acpi_gbl_gpe_lock>
...
<release acpi_gbl_hardware_lock>
>
> Thanks,
> Rakib
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] acpi: Fix possible recursive locking in hwregs.c
2011-09-22 4:28 ` Lin Ming
@ 2011-09-22 5:14 ` Rakib Mullick
0 siblings, 0 replies; 5+ messages in thread
From: Rakib Mullick @ 2011-09-22 5:14 UTC (permalink / raw)
To: Lin Ming
Cc: linux-kernel@vger.kernel.org, Brown, Len, Andrew Morton,
linux-acpi@vger.kernel.org, Moore, Robert
On Thu, Sep 22, 2011 at 10:28 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Thu, 2011-09-22 at 11:35 +0800, Rakib Mullick wrote:
>> On Thu, Sep 22, 2011 at 6:59 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> > On Thu, Sep 22, 2011 at 2:09 AM, Rakib Mullick <rakib.mullick@gmail.com> wrote:
>> >> Calling pm-suspend might trigger a recursive lock in it's code path. In function acpi_hw_clear_acpi_status,
>> >> acpi_os_release_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 also tries to
>> >> hold acpi_gbl_hardware_lock and thus causes possible recursive lock.
>> >
>> > No, acpi_ev_walk_gpe_list holds acpi_gbl_gpe_lock.
>> >
>> Yes, right. Actually, acpi_os_release_lock() tries to hold
>> acpi_gbl_gpe_lock without releasing acpi_gbl_hardware_lock. Right?
>
> Do you mean "acpi_ev_walk_gpe_list() tries to hold
> acpi_gbl_gpe_lock..."?
>
I wanted to mean, acpi_os_acquire_lock() tries to hold
acpi_gbl_gpe_lock without releasing acpi_gbl_hardware_lock. Actually,
this recursive locking scenario showed up on my modified kernel and
fails to suspend. But, never happened on mainline kernel. Looking at
the warning it seems to me that, it's safe to release the
acpi_gbl_hardware_lock before calling acpi_ev_walk_gpe_list() and also
solves the problem. Calling acpi_ev_walk_gpe_list() doesn't require
acpi_gbl_hardware_lock to be held. This is the reason behind the
patch.
> We have below locks sequence. I don't see the problem.
>
> acpi_hw_clear_acpi_status:
>
> <acquire acpi_gbl_hardware_lock>
>
> acpi_ev_walk_gpe_list:
> <acquire acpi_gbl_gpe_lock>
> ...
> <release acpi_gbl_gpe_lock>
> ...
> <release acpi_gbl_hardware_lock>
>
>>
>> Thanks,
>> Rakib
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-09-22 5:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-21 18:09 [PATCH] acpi: Fix possible recursive locking in hwregs.c Rakib Mullick
2011-09-22 0:59 ` Lin Ming
2011-09-22 3:35 ` Rakib Mullick
2011-09-22 4:28 ` Lin Ming
2011-09-22 5:14 ` Rakib Mullick
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).