ACPI: Fix lockdep false positives in acpi_power_off()

Commit Message

Rafael Wysocki July 4, 2011, 11:32 p.m. UTC

On Monday, July 04, 2011, Andrea Righi wrote:
> On Mon, Jul 04, 2011 at 12:28:23AM +0200, Rafael J. Wysocki wrote:
> > On Monday, July 04, 2011, Rafael J. Wysocki wrote:
> > > On Sunday, July 03, 2011, Rafael J. Wysocki wrote:
> > > > On Sunday, July 03, 2011, Andrea Righi wrote:
> > > > > Implement acpi_os_create_lock() as a C-preprocessor macro to suppress
> > > > > lockdep false positive.
> > > > > 
> > > > > When lockdep is enabled the spin_lock_init macro stringifies it's
> > > > > argument and uses that as a name for the lock in the debugging.
> > > > > 
> > > > > By executing spin_lock_init in a macro the key changes from "lock" for
> > > > > all locks to the actual argument of acpi_os_create_lock()
> > > > > ("&acpi_gbl_global_lock_pending_lock", "&acpi_gbl_gpe_lock" or
> > > > > "&acpi_gbl_hardware_lock" for now).
> > > > > 
> > > > > This fixes:
> > > > > https://bugzilla.kernel.org/show_bug.cgi?id=38152
> > > > > 
> > > > > ChangeLog (v1 -> v2):
> > > > >  - avoid to call spin_lock_init multiple times on the same lock
> > > > >  - rewrite patch description (thanks to Florian for providing a better
> > > > >    description of the patch)
> > > > > 
> > > > > Reported-by: Borislav Petkov <[e-mail]>
> > > > > CC: Florian Mickler <[e-mail]>
> > > > > Signed-off-by: Andrea Righi <[e-mail]>
> > > > > ---
> > > > >  drivers/acpi/osl.c      |    3 +--
> > > > >  include/acpi/acpiosxf.h |   12 +++++++++++-
> > > > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > > > > index 52ca964..4c985d3 100644
> > > > > --- a/drivers/acpi/osl.c
> > > > > +++ b/drivers/acpi/osl.c
> > > > > @@ -1336,14 +1336,13 @@ EXPORT_SYMBOL(acpi_resources_are_enforced);
> > > > >   * Create and initialize a spinlock.
> > > > >   */
> > > > >  acpi_status
> > > > > -acpi_os_create_lock(acpi_spinlock *out_handle)
> > > > > +__acpi_os_create_lock(acpi_spinlock *out_handle)
> > > > >  {
> > > > 
> > > > I would rename this to acpi_os_allocate_lock() or acpi_os_alloc_lock(),
> > > > so that it doesn't suggest the lock is initialized by this function.
> > > > 
> > > > Hmm.  There's one more thing we need to take into account here.  Namely,
> > > > include/acpi/acpiosxf.h is used by other OSes, so we shouldn't put
> > > > Linux-specific stuff into it.
> > > > 
> > > > I'm not sure how to work around that at the moment.
> > > 
> > > OK, the patch below builds for me and seems to work even, although I haven't
> > > tested it with lockdep on.
> > 
> > Below is a cleaned-up version, still untested with lockdep on.
> 
> Tested. Works fine for me.

OK, below is an official version.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[e-mail]>
Subject: ACPI: Fix lockdep false positives in acpi_power_off()

All ACPICA locks are allocated and initialized by the same function,
acpi_os_create_lock(), with the help of a local variable called
"lock".  Thus, when lockdep is enabled, it uses "lock" as the
name of all those locks and regards them as instances of the same
lock, which causes it to report possible locking problems with them
when there aren't any.

To work around this problem, define acpi_os_create_lock() as a macro
and make it pass its argument to spin_lock_init(), so that lockdep
uses it as the name of the new lock.  Define this macron in a
Linux-specific file to minimize the resulting modifications of
the OS-independent ACPICA parts.

This change is based on an earlier patch from Andrea Righi and it
addresses a regression from 2.6.39 tracked as
https://bugzilla.kernel.org/show_bug.cgi?id=38152

Signed-off-by: Rafael J. Wysocki <[e-mail]>
Reported-by: Borislav Petkov <[e-mail]>
Tested-by: Andrea Righi <[e-mail]>
---
 drivers/acpi/osl.c              |   17 -----------------
 include/acpi/acpiosxf.h         |    3 +++
 include/acpi/platform/aclinux.h |   18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Borislav Petkov July 5, 2011, 7:40 a.m. UTC | #1

On Tue, Jul 05, 2011 at 01:32:11AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> Subject: ACPI: Fix lockdep false positives in acpi_power_off()
> 
> All ACPICA locks are allocated and initialized by the same function,
> acpi_os_create_lock(), with the help of a local variable called
> "lock".  Thus, when lockdep is enabled, it uses "lock" as the
> name of all those locks and regards them as instances of the same
> lock, which causes it to report possible locking problems with them
> when there aren't any.
> 
> To work around this problem, define acpi_os_create_lock() as a macro
> and make it pass its argument to spin_lock_init(), so that lockdep
> uses it as the name of the new lock.  Define this macron in a
> Linux-specific file to minimize the resulting modifications of
> the OS-independent ACPICA parts.
> 
> This change is based on an earlier patch from Andrea Righi and it
> addresses a regression from 2.6.39 tracked as
> https://bugzilla.kernel.org/show_bug.cgi?id=38152
> 
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> Reported-by: Borislav Petkov <[e-mail]>
> Tested-by: Andrea Righi <[e-mail]>

Tested-by: Borislav Petkov <[e-mail]>

Florian Mickler July 5, 2011, 8:03 a.m. UTC | #2

On Tue, 5 Jul 2011 01:32:11 +0200
"Rafael J. Wysocki" <[e-mail]> wrote:
> 
> OK, below is an official version.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <[e-mail]>
> Subject: ACPI: Fix lockdep false positives in acpi_power_off()
> 
> All ACPICA locks are allocated and initialized by the same function,
> acpi_os_create_lock(), with the help of a local variable called
> "lock".  Thus, when lockdep is enabled, it uses "lock" as the
> name of all those locks and regards them as instances of the same
> lock, which causes it to report possible locking problems with them
> when there aren't any.
> 
> To work around this problem, define acpi_os_create_lock() as a macro
> and make it pass its argument to spin_lock_init(), so that lockdep
> uses it as the name of the new lock.  Define this macron in a
> Linux-specific file to minimize the resulting modifications of
> the OS-independent ACPICA parts.
> 
> This change is based on an earlier patch from Andrea Righi and it
> addresses a regression from 2.6.39 tracked as
> https://bugzilla.kernel.org/show_bug.cgi?id=38152
> 
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> Reported-by: Borislav Petkov <[e-mail]>
> Tested-by: Andrea Righi <[e-mail]>
> ---
>  drivers/acpi/osl.c              |   17 -----------------
>  include/acpi/acpiosxf.h         |    3 +++
>  include/acpi/platform/aclinux.h |   18 ++++++++++++++++++
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/osl.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/osl.c
> +++ linux-2.6/drivers/acpi/osl.c
> @@ -1333,23 +1333,6 @@ int acpi_resources_are_enforced(void)
>  EXPORT_SYMBOL(acpi_resources_are_enforced);
>  
>  /*
> - * Create and initialize a spinlock.
> - */
> -acpi_status
> -acpi_os_create_lock(acpi_spinlock *out_handle)
> -{
> -	spinlock_t *lock;
> -
> -	lock = ACPI_ALLOCATE(sizeof(spinlock_t));
> -	if (!lock)
> -		return AE_NO_MEMORY;
> -	spin_lock_init(lock);
> -	*out_handle = lock;
> -
> -	return AE_OK;
> -}
> -
> -/*
>   * Deallocate the memory for a spinlock.
>   */
>  void acpi_os_delete_lock(acpi_spinlock handle)
> Index: linux-2.6/include/acpi/acpiosxf.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/acpiosxf.h
> +++ linux-2.6/include/acpi/acpiosxf.h
> @@ -98,8 +98,11 @@ acpi_os_table_override(struct acpi_table
>  /*
>   * Spinlock primitives
>   */
> +
> +#ifndef acpi_os_create_lock
>  acpi_status
>  acpi_os_create_lock(acpi_spinlock *out_handle);
> +#endif
>  
>  void acpi_os_delete_lock(acpi_spinlock handle);
>  
> Index: linux-2.6/include/acpi/platform/aclinux.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/platform/aclinux.h
> +++ linux-2.6/include/acpi/platform/aclinux.h
> @@ -159,6 +159,24 @@ static inline void *acpi_os_acquire_obje
>  	} while (0)
>  #endif
>  
> +/*
> + * When lockdep is enabled, the spin_lock_init() macro stringifies it's
> + * argument and uses that as a name for the lock in debugging.
> + * By executing spin_lock_init() in a macro the key changes from "lock" for
> + * all locks to the name of the argument of acpi_os_create_lock(), which
> + * prevents lockdep from reporting false positives for ACPICA locks.
> + */
> +#define acpi_os_create_lock(__handle)				\
> +({								\
> +	spinlock_t *lock = ACPI_ALLOCATE(sizeof(*lock));	\
> +								\
> +	if (lock) {						\
> +		*(__handle) = lock;				\
> +		spin_lock_init(*(__handle));			\
> +	}							\
> +	lock ? AE_OK : AE_NO_MEMORY;				\
> +})
> +
>  #endif /* __KERNEL__ */
>  
>  #endif /* __ACLINUX_H__ */

Since I made the effort of digging into the lockdep code, you can
actually add my 
 Reviewed-by: Florian Mickler <[e-mail]>

Regards,
Flo

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: linux-2.6/drivers/acpi/osl.c
===================================================================
--- linux-2.6.orig/drivers/acpi/osl.c
+++ linux-2.6/drivers/acpi/osl.c
@@ -1333,23 +1333,6 @@  int acpi_resources_are_enforced(void)
 EXPORT_SYMBOL(acpi_resources_are_enforced);
 
 /*
- * Create and initialize a spinlock.
- */
-acpi_status
-acpi_os_create_lock(acpi_spinlock *out_handle)
-{
-	spinlock_t *lock;
-
-	lock = ACPI_ALLOCATE(sizeof(spinlock_t));
-	if (!lock)
-		return AE_NO_MEMORY;
-	spin_lock_init(lock);
-	*out_handle = lock;
-
-	return AE_OK;
-}
-
-/*
  * Deallocate the memory for a spinlock.
  */
 void acpi_os_delete_lock(acpi_spinlock handle)
Index: linux-2.6/include/acpi/acpiosxf.h
===================================================================
--- linux-2.6.orig/include/acpi/acpiosxf.h
+++ linux-2.6/include/acpi/acpiosxf.h
@@ -98,8 +98,11 @@  acpi_os_table_override(struct acpi_table
 /*
  * Spinlock primitives
  */
+
+#ifndef acpi_os_create_lock
 acpi_status
 acpi_os_create_lock(acpi_spinlock *out_handle);
+#endif
 
 void acpi_os_delete_lock(acpi_spinlock handle);
 
Index: linux-2.6/include/acpi/platform/aclinux.h
===================================================================
--- linux-2.6.orig/include/acpi/platform/aclinux.h
+++ linux-2.6/include/acpi/platform/aclinux.h
@@ -159,6 +159,24 @@  static inline void *acpi_os_acquire_obje
 	} while (0)
 #endif
 
+/*
+ * When lockdep is enabled, the spin_lock_init() macro stringifies it's
+ * argument and uses that as a name for the lock in debugging.
+ * By executing spin_lock_init() in a macro the key changes from "lock" for
+ * all locks to the name of the argument of acpi_os_create_lock(), which
+ * prevents lockdep from reporting false positives for ACPICA locks.
+ */
+#define acpi_os_create_lock(__handle)				\
+({								\
+	spinlock_t *lock = ACPI_ALLOCATE(sizeof(*lock));	\
+								\
+	if (lock) {						\
+		*(__handle) = lock;				\
+		spin_lock_init(*(__handle));			\
+	}							\
+	lock ? AE_OK : AE_NO_MEMORY;				\
+})
+
 #endif /* __KERNEL__ */
 
 #endif /* __ACLINUX_H__ */