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

[PATCH] 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_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);
 }
 

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

[Lin Ming]

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

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

[Lin Ming]

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

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

[Rakib Mullick]

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

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

[Lin Ming]

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

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

[Rakib Mullick]

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

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

[Rakib Mullick]

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