[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

Hi,

In case when hwmods are used in nested way the lockdep validator will print out
a warning message about possible deadlock situation:

[    4.514882] =============================================
[    4.520530] [ INFO: possible recursive locking detected ]
[    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
[    4.532285] ---------------------------------------------
[    4.537936] kworker/u4:1/18 is trying to acquire lock:
[    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
[    4.552109] 
[    4.552109] but task is already holding lock:
[    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
[    4.566999] 
[    4.566999] other info that might help us debug this:
[    4.573831]  Possible unsafe locking scenario:
[    4.573831] 
[    4.580025]        CPU0
[    4.582584]        ----
[    4.585142]   lock(&(&oh->_lock)->rlock);
[    4.589544]   lock(&(&oh->_lock)->rlock);
[    4.593945] 
[    4.593945]  *** DEADLOCK ***
[    4.593945] 
[    4.600146]  May be due to missing lock nesting notation

What lockdep did not realizes is that the two oh is not the same hwmod object
and we have taken different locks.

One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
configured to use ATL clock as it's functional clock. In this case the
pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
operation it will enable the needed clocks. Since ATL clock is needed, we will
have another pm_runtime operation from the ATL clock enable callback which
will take the atl hwmod's lock. This will be seen by lockdep as possible
deadlock situation.

In order to notify lockdep about this, we need to switch using _nested
version of locking function and assign different subclass to those hwmods which
could be used in nested way.

Regards,
Peter
---
Peter Ujfalusi (2):
  ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock
  ARM: DRA7: hwmod_data: Change locked_class for atl hwmod

 arch/arm/mach-omap2/omap_hwmod.c          | 16 ++++++++--------
 arch/arm/mach-omap2/omap_hwmod.h          |  5 +++++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c |  1 +
 3 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.2.2

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

Hi,

In case when hwmods are used in nested way the lockdep validator will print out
a warning message about possible deadlock situation:

[    4.514882] =============================================
[    4.520530] [ INFO: possible recursive locking detected ]
[    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
[    4.532285] ---------------------------------------------
[    4.537936] kworker/u4:1/18 is trying to acquire lock:
[    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
[    4.552109] 
[    4.552109] but task is already holding lock:
[    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
[    4.566999] 
[    4.566999] other info that might help us debug this:
[    4.573831]  Possible unsafe locking scenario:
[    4.573831] 
[    4.580025]        CPU0
[    4.582584]        ----
[    4.585142]   lock(&(&oh->_lock)->rlock);
[    4.589544]   lock(&(&oh->_lock)->rlock);
[    4.593945] 
[    4.593945]  *** DEADLOCK ***
[    4.593945] 
[    4.600146]  May be due to missing lock nesting notation

What lockdep did not realizes is that the two oh is not the same hwmod object
and we have taken different locks.

One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
configured to use ATL clock as it's functional clock. In this case the
pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
operation it will enable the needed clocks. Since ATL clock is needed, we will
have another pm_runtime operation from the ATL clock enable callback which
will take the atl hwmod's lock. This will be seen by lockdep as possible
deadlock situation.

In order to notify lockdep about this, we need to switch using _nested
version of locking function and assign different subclass to those hwmods which
could be used in nested way.

Regards,
Peter
---
Peter Ujfalusi (2):
  ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock
  ARM: DRA7: hwmod_data: Change locked_class for atl hwmod

 arch/arm/mach-omap2/omap_hwmod.c          | 16 ++++++++--------
 arch/arm/mach-omap2/omap_hwmod.h          |  5 +++++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c |  1 +
 3 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.2.2

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

Hi,

In case when hwmods are used in nested way the lockdep validator will print out
a warning message about possible deadlock situation:

[    4.514882] =============================================
[    4.520530] [ INFO: possible recursive locking detected ]
[    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
[    4.532285] ---------------------------------------------
[    4.537936] kworker/u4:1/18 is trying to acquire lock:
[    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
[    4.552109] 
[    4.552109] but task is already holding lock:
[    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
[    4.566999] 
[    4.566999] other info that might help us debug this:
[    4.573831]  Possible unsafe locking scenario:
[    4.573831] 
[    4.580025]        CPU0
[    4.582584]        ----
[    4.585142]   lock(&(&oh->_lock)->rlock);
[    4.589544]   lock(&(&oh->_lock)->rlock);
[    4.593945] 
[    4.593945]  *** DEADLOCK ***
[    4.593945] 
[    4.600146]  May be due to missing lock nesting notation

What lockdep did not realizes is that the two oh is not the same hwmod object
and we have taken different locks.

One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
configured to use ATL clock as it's functional clock. In this case the
pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
operation it will enable the needed clocks. Since ATL clock is needed, we will
have another pm_runtime operation from the ATL clock enable callback which
will take the atl hwmod's lock. This will be seen by lockdep as possible
deadlock situation.

In order to notify lockdep about this, we need to switch using _nested
version of locking function and assign different subclass to those hwmods which
could be used in nested way.

Regards,
Peter
---
Peter Ujfalusi (2):
  ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock
  ARM: DRA7: hwmod_data: Change locked_class for atl hwmod

 arch/arm/mach-omap2/omap_hwmod.c          | 16 ++++++++--------
 arch/arm/mach-omap2/omap_hwmod.h          |  5 +++++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c |  1 +
 3 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.2.2

[PATCH 1/2] ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock

[Peter Ujfalusi]

Add lockdep_class member to struct omap_hwmod and use this number to
specify the subclass of the given hwmod's lock.
When defining the hwmods the lockdep_level can be initialized to indicate
that this lock might be used in a nested fashion.
DRA7x's ATL hwmod is one example for this since McASP can select ATL clock
as functional clock, which will trigger nested oh->_lock usage. This will
trigger false warning from lockdep validator since it is dealing with
classes and for it all hwmod clocks are the same class.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++--------
 arch/arm/mach-omap2/omap_hwmod.h |  5 +++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 92afb723dcfc..b5afde560054 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -3334,7 +3334,7 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	r = _enable(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3355,7 +3355,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	_idle(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3377,7 +3377,7 @@ int omap_hwmod_shutdown(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	_shutdown(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3667,7 +3667,7 @@ int omap_hwmod_enable_wakeup(struct omap_hwmod *oh)
 	unsigned long flags;
 	u32 v;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->class->sysc &&
 	    (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3700,7 +3700,7 @@ int omap_hwmod_disable_wakeup(struct omap_hwmod *oh)
 	unsigned long flags;
 	u32 v;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->class->sysc &&
 	    (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3735,7 +3735,7 @@ int omap_hwmod_assert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	ret = _assert_hardreset(oh, name);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3762,7 +3762,7 @@ int omap_hwmod_deassert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	ret = _deassert_hardreset(oh, name);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3836,7 +3836,7 @@ int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state)
 	    state != _HWMOD_STATE_IDLE)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->_state != _HWMOD_STATE_REGISTERED) {
 		ret = -EINVAL;
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 9d4bec6ee742..260d3e9c9f6d 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -606,6 +606,9 @@ struct omap_hwmod_link {
 	struct list_head		node;
 };
 
+#define HWMOD_LOCKDEP_SUBCLASS_NORMAL	0
+#define HWMOD_LOCKDEP_SUBCLASS_CLASS1	1	/* might be used as nested */
+
 /**
  * struct omap_hwmod - integration data for OMAP hardware "modules" (IP blocks)
  * @name: name of the hwmod
@@ -632,6 +635,7 @@ struct omap_hwmod_link {
  * @_postsetup_state: internal-use state to leave the hwmod in after _setup()
  * @flags: hwmod flags (documented below)
  * @_lock: spinlock serializing operations on this hwmod
+ * @lockdep_class: subclass to use with spin_lock_irqsave_nested()
  * @node: list node for hwmod list (internal use)
  * @parent_hwmod: (temporary) a pointer to the hierarchical parent of this hwmod
  *
@@ -674,6 +678,7 @@ struct omap_hwmod {
 	u32				_sysc_cache;
 	void __iomem			*_mpu_rt_va;
 	spinlock_t			_lock;
+	unsigned int			lockdep_class;
 	struct list_head		node;
 	struct omap_hwmod_ocp_if	*_mpu_port;
 	unsigned int			(*xlate_irq)(unsigned int);
-- 
2.2.2

[PATCH 1/2] ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock

[Peter Ujfalusi]

Add lockdep_class member to struct omap_hwmod and use this number to
specify the subclass of the given hwmod's lock.
When defining the hwmods the lockdep_level can be initialized to indicate
that this lock might be used in a nested fashion.
DRA7x's ATL hwmod is one example for this since McASP can select ATL clock
as functional clock, which will trigger nested oh->_lock usage. This will
trigger false warning from lockdep validator since it is dealing with
classes and for it all hwmod clocks are the same class.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++--------
 arch/arm/mach-omap2/omap_hwmod.h |  5 +++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 92afb723dcfc..b5afde560054 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -3334,7 +3334,7 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	r = _enable(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3355,7 +3355,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	_idle(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3377,7 +3377,7 @@ int omap_hwmod_shutdown(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	_shutdown(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3667,7 +3667,7 @@ int omap_hwmod_enable_wakeup(struct omap_hwmod *oh)
 	unsigned long flags;
 	u32 v;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->class->sysc &&
 	    (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3700,7 +3700,7 @@ int omap_hwmod_disable_wakeup(struct omap_hwmod *oh)
 	unsigned long flags;
 	u32 v;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->class->sysc &&
 	    (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3735,7 +3735,7 @@ int omap_hwmod_assert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	ret = _assert_hardreset(oh, name);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3762,7 +3762,7 @@ int omap_hwmod_deassert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	ret = _deassert_hardreset(oh, name);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3836,7 +3836,7 @@ int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state)
 	    state != _HWMOD_STATE_IDLE)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->_state != _HWMOD_STATE_REGISTERED) {
 		ret = -EINVAL;
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 9d4bec6ee742..260d3e9c9f6d 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -606,6 +606,9 @@ struct omap_hwmod_link {
 	struct list_head		node;
 };
 
+#define HWMOD_LOCKDEP_SUBCLASS_NORMAL	0
+#define HWMOD_LOCKDEP_SUBCLASS_CLASS1	1	/* might be used as nested */
+
 /**
  * struct omap_hwmod - integration data for OMAP hardware "modules" (IP blocks)
  * @name: name of the hwmod
@@ -632,6 +635,7 @@ struct omap_hwmod_link {
  * @_postsetup_state: internal-use state to leave the hwmod in after _setup()
  * @flags: hwmod flags (documented below)
  * @_lock: spinlock serializing operations on this hwmod
+ * @lockdep_class: subclass to use with spin_lock_irqsave_nested()
  * @node: list node for hwmod list (internal use)
  * @parent_hwmod: (temporary) a pointer to the hierarchical parent of this hwmod
  *
@@ -674,6 +678,7 @@ struct omap_hwmod {
 	u32				_sysc_cache;
 	void __iomem			*_mpu_rt_va;
 	spinlock_t			_lock;
+	unsigned int			lockdep_class;
 	struct list_head		node;
 	struct omap_hwmod_ocp_if	*_mpu_port;
 	unsigned int			(*xlate_irq)(unsigned int);
-- 
2.2.2

[PATCH 1/2] ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock

[Peter Ujfalusi]

Add lockdep_class member to struct omap_hwmod and use this number to
specify the subclass of the given hwmod's lock.
When defining the hwmods the lockdep_level can be initialized to indicate
that this lock might be used in a nested fashion.
DRA7x's ATL hwmod is one example for this since McASP can select ATL clock
as functional clock, which will trigger nested oh->_lock usage. This will
trigger false warning from lockdep validator since it is dealing with
classes and for it all hwmod clocks are the same class.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++--------
 arch/arm/mach-omap2/omap_hwmod.h |  5 +++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 92afb723dcfc..b5afde560054 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -3334,7 +3334,7 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	r = _enable(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3355,7 +3355,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	_idle(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3377,7 +3377,7 @@ int omap_hwmod_shutdown(struct omap_hwmod *oh)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	_shutdown(oh);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3667,7 +3667,7 @@ int omap_hwmod_enable_wakeup(struct omap_hwmod *oh)
 	unsigned long flags;
 	u32 v;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->class->sysc &&
 	    (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3700,7 +3700,7 @@ int omap_hwmod_disable_wakeup(struct omap_hwmod *oh)
 	unsigned long flags;
 	u32 v;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->class->sysc &&
 	    (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3735,7 +3735,7 @@ int omap_hwmod_assert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	ret = _assert_hardreset(oh, name);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3762,7 +3762,7 @@ int omap_hwmod_deassert_hardreset(struct omap_hwmod *oh, const char *name)
 	if (!oh)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 	ret = _deassert_hardreset(oh, name);
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
@@ -3836,7 +3836,7 @@ int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state)
 	    state != _HWMOD_STATE_IDLE)
 		return -EINVAL;
 
-	spin_lock_irqsave(&oh->_lock, flags);
+	spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
 
 	if (oh->_state != _HWMOD_STATE_REGISTERED) {
 		ret = -EINVAL;
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 9d4bec6ee742..260d3e9c9f6d 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -606,6 +606,9 @@ struct omap_hwmod_link {
 	struct list_head		node;
 };
 
+#define HWMOD_LOCKDEP_SUBCLASS_NORMAL	0
+#define HWMOD_LOCKDEP_SUBCLASS_CLASS1	1	/* might be used as nested */
+
 /**
  * struct omap_hwmod - integration data for OMAP hardware "modules" (IP blocks)
  * @name: name of the hwmod
@@ -632,6 +635,7 @@ struct omap_hwmod_link {
  * @_postsetup_state: internal-use state to leave the hwmod in after _setup()
  * @flags: hwmod flags (documented below)
  * @_lock: spinlock serializing operations on this hwmod
+ * @lockdep_class: subclass to use with spin_lock_irqsave_nested()
  * @node: list node for hwmod list (internal use)
  * @parent_hwmod: (temporary) a pointer to the hierarchical parent of this hwmod
  *
@@ -674,6 +678,7 @@ struct omap_hwmod {
 	u32				_sysc_cache;
 	void __iomem			*_mpu_rt_va;
 	spinlock_t			_lock;
+	unsigned int			lockdep_class;
 	struct list_head		node;
 	struct omap_hwmod_ocp_if	*_mpu_port;
 	unsigned int			(*xlate_irq)(unsigned int);
-- 
2.2.2

[PATCH 2/2] ARM: DRA7: hwmod_data: Change locked_class for atl hwmod

[Peter Ujfalusi]

The ATL hwmod can be used in nested way when it is selected to be the
functional clock for McASP. For this lockdep validator will trigger false
positive warning.
By assigning separate class to atl locking will sort this out.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index e8692e7675b8..87f259e8c574 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -185,6 +185,7 @@ static struct omap_hwmod dra7xx_atl_hwmod = {
 	.class		= &dra7xx_atl_hwmod_class,
 	.clkdm_name	= "atl_clkdm",
 	.main_clk	= "atl_gfclk_mux",
+	.lockdep_class	= HWMOD_LOCKDEP_SUBCLASS_CLASS1,
 	.prcm = {
 		.omap4 = {
 			.clkctrl_offs = DRA7XX_CM_ATL_ATL_CLKCTRL_OFFSET,
-- 
2.2.2

[PATCH 2/2] ARM: DRA7: hwmod_data: Change locked_class for atl hwmod

[Peter Ujfalusi]

The ATL hwmod can be used in nested way when it is selected to be the
functional clock for McASP. For this lockdep validator will trigger false
positive warning.
By assigning separate class to atl locking will sort this out.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index e8692e7675b8..87f259e8c574 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -185,6 +185,7 @@ static struct omap_hwmod dra7xx_atl_hwmod = {
 	.class		= &dra7xx_atl_hwmod_class,
 	.clkdm_name	= "atl_clkdm",
 	.main_clk	= "atl_gfclk_mux",
+	.lockdep_class	= HWMOD_LOCKDEP_SUBCLASS_CLASS1,
 	.prcm = {
 		.omap4 = {
 			.clkctrl_offs = DRA7XX_CM_ATL_ATL_CLKCTRL_OFFSET,
-- 
2.2.2

[PATCH 2/2] ARM: DRA7: hwmod_data: Change locked_class for atl hwmod

[Peter Ujfalusi]

The ATL hwmod can be used in nested way when it is selected to be the
functional clock for McASP. For this lockdep validator will trigger false
positive warning.
By assigning separate class to atl locking will sort this out.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index e8692e7675b8..87f259e8c574 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -185,6 +185,7 @@ static struct omap_hwmod dra7xx_atl_hwmod = {
 	.class		= &dra7xx_atl_hwmod_class,
 	.clkdm_name	= "atl_clkdm",
 	.main_clk	= "atl_gfclk_mux",
+	.lockdep_class	= HWMOD_LOCKDEP_SUBCLASS_CLASS1,
 	.prcm = {
 		.omap4 = {
 			.clkctrl_offs = DRA7XX_CM_ATL_ATL_CLKCTRL_OFFSET,
-- 
2.2.2

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Zijlstra]

On Fri, Feb 06, 2015 at 02:48:34PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> In case when hwmods are used in nested way the lockdep validator will print out
> a warning message about possible deadlock situation:
> 
> [    4.514882] =============================================
> [    4.520530] [ INFO: possible recursive locking detected ]
> [    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
> [    4.532285] ---------------------------------------------
> [    4.537936] kworker/u4:1/18 is trying to acquire lock:
> [    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
> [    4.552109] 
> [    4.552109] but task is already holding lock:
> [    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
> [    4.566999] 
> [    4.566999] other info that might help us debug this:
> [    4.573831]  Possible unsafe locking scenario:
> [    4.573831] 
> [    4.580025]        CPU0
> [    4.582584]        ----
> [    4.585142]   lock(&(&oh->_lock)->rlock);
> [    4.589544]   lock(&(&oh->_lock)->rlock);
> [    4.593945] 
> [    4.593945]  *** DEADLOCK ***
> [    4.593945] 
> [    4.600146]  May be due to missing lock nesting notation
> 
> What lockdep did not realizes is that the two oh is not the same hwmod object
> and we have taken different locks.
> 
> One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
> configured to use ATL clock as it's functional clock. In this case the
> pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
> operation it will enable the needed clocks. Since ATL clock is needed, we will
> have another pm_runtime operation from the ATL clock enable callback which
> will take the atl hwmod's lock. This will be seen by lockdep as possible
> deadlock situation.
> 
> In order to notify lockdep about this, we need to switch using _nested
> version of locking function and assign different subclass to those hwmods which
> could be used in nested way.

IFF struct omap_hwmod is always statically allocated; as I think the
__init on _register() mandates, you can use the below annotation.

---
 arch/arm/mach-omap2/omap_hwmod.c | 1 +
 arch/arm/mach-omap2/omap_hwmod.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 9025ffffd2dc..222d654ad6fd 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh)
 	INIT_LIST_HEAD(&oh->master_ports);
 	INIT_LIST_HEAD(&oh->slave_ports);
 	spin_lock_init(&oh->_lock);
+	lockdep_set_class(&oh->_lock, &oh->hwmod_key);
 
 	oh->_state = _HWMOD_STATE_REGISTERED;
 
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 5b42fafcaf55..754bdfb3f811 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -689,6 +689,8 @@ struct omap_hwmod {
 	u8				_state;
 	u8				_postsetup_state;
 	struct omap_hwmod		*parent_hwmod;
+
+	struct lock_class_key		hwmod_key;
 };
 
 struct omap_hwmod *omap_hwmod_lookup(const char *name);

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Zijlstra]

On Fri, Feb 06, 2015 at 02:48:34PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> In case when hwmods are used in nested way the lockdep validator will print out
> a warning message about possible deadlock situation:
> 
> [    4.514882] =============================================
> [    4.520530] [ INFO: possible recursive locking detected ]
> [    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
> [    4.532285] ---------------------------------------------
> [    4.537936] kworker/u4:1/18 is trying to acquire lock:
> [    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
> [    4.552109] 
> [    4.552109] but task is already holding lock:
> [    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
> [    4.566999] 
> [    4.566999] other info that might help us debug this:
> [    4.573831]  Possible unsafe locking scenario:
> [    4.573831] 
> [    4.580025]        CPU0
> [    4.582584]        ----
> [    4.585142]   lock(&(&oh->_lock)->rlock);
> [    4.589544]   lock(&(&oh->_lock)->rlock);
> [    4.593945] 
> [    4.593945]  *** DEADLOCK ***
> [    4.593945] 
> [    4.600146]  May be due to missing lock nesting notation
> 
> What lockdep did not realizes is that the two oh is not the same hwmod object
> and we have taken different locks.
> 
> One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
> configured to use ATL clock as it's functional clock. In this case the
> pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
> operation it will enable the needed clocks. Since ATL clock is needed, we will
> have another pm_runtime operation from the ATL clock enable callback which
> will take the atl hwmod's lock. This will be seen by lockdep as possible
> deadlock situation.
> 
> In order to notify lockdep about this, we need to switch using _nested
> version of locking function and assign different subclass to those hwmods which
> could be used in nested way.

IFF struct omap_hwmod is always statically allocated; as I think the
__init on _register() mandates, you can use the below annotation.

---
 arch/arm/mach-omap2/omap_hwmod.c | 1 +
 arch/arm/mach-omap2/omap_hwmod.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 9025ffffd2dc..222d654ad6fd 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh)
 	INIT_LIST_HEAD(&oh->master_ports);
 	INIT_LIST_HEAD(&oh->slave_ports);
 	spin_lock_init(&oh->_lock);
+	lockdep_set_class(&oh->_lock, &oh->hwmod_key);
 
 	oh->_state = _HWMOD_STATE_REGISTERED;
 
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 5b42fafcaf55..754bdfb3f811 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -689,6 +689,8 @@ struct omap_hwmod {
 	u8				_state;
 	u8				_postsetup_state;
 	struct omap_hwmod		*parent_hwmod;
+
+	struct lock_class_key		hwmod_key;
 };
 
 struct omap_hwmod *omap_hwmod_lookup(const char *name);

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 04:13 PM, Peter Zijlstra wrote:
> On Fri, Feb 06, 2015 at 02:48:34PM +0200, Peter Ujfalusi wrote:
>> Hi,
>>
>> In case when hwmods are used in nested way the lockdep validator will print out
>> a warning message about possible deadlock situation:
>>
>> [    4.514882] =============================================
>> [    4.520530] [ INFO: possible recursive locking detected ]
>> [    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
>> [    4.532285] ---------------------------------------------
>> [    4.537936] kworker/u4:1/18 is trying to acquire lock:
>> [    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
>> [    4.552109] 
>> [    4.552109] but task is already holding lock:
>> [    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
>> [    4.566999] 
>> [    4.566999] other info that might help us debug this:
>> [    4.573831]  Possible unsafe locking scenario:
>> [    4.573831] 
>> [    4.580025]        CPU0
>> [    4.582584]        ----
>> [    4.585142]   lock(&(&oh->_lock)->rlock);
>> [    4.589544]   lock(&(&oh->_lock)->rlock);
>> [    4.593945] 
>> [    4.593945]  *** DEADLOCK ***
>> [    4.593945] 
>> [    4.600146]  May be due to missing lock nesting notation
>>
>> What lockdep did not realizes is that the two oh is not the same hwmod object
>> and we have taken different locks.
>>
>> One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
>> configured to use ATL clock as it's functional clock. In this case the
>> pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
>> operation it will enable the needed clocks. Since ATL clock is needed, we will
>> have another pm_runtime operation from the ATL clock enable callback which
>> will take the atl hwmod's lock. This will be seen by lockdep as possible
>> deadlock situation.
>>
>> In order to notify lockdep about this, we need to switch using _nested
>> version of locking function and assign different subclass to those hwmods which
>> could be used in nested way.
> 
> IFF struct omap_hwmod is always statically allocated; as I think the
> __init on _register() mandates, you can use the below annotation.
> 
> ---
>  arch/arm/mach-omap2/omap_hwmod.c | 1 +
>  arch/arm/mach-omap2/omap_hwmod.h | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 9025ffffd2dc..222d654ad6fd 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh)
>  	INIT_LIST_HEAD(&oh->master_ports);
>  	INIT_LIST_HEAD(&oh->slave_ports);
>  	spin_lock_init(&oh->_lock);
> +	lockdep_set_class(&oh->_lock, &oh->hwmod_key);
>  
>  	oh->_state = _HWMOD_STATE_REGISTERED;
>  
> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
> index 5b42fafcaf55..754bdfb3f811 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.h
> +++ b/arch/arm/mach-omap2/omap_hwmod.h
> @@ -689,6 +689,8 @@ struct omap_hwmod {
>  	u8				_state;
>  	u8				_postsetup_state;
>  	struct omap_hwmod		*parent_hwmod;
> +
> +	struct lock_class_key		hwmod_key;
>  };
>  
>  struct omap_hwmod *omap_hwmod_lookup(const char *name);

Certainly looks much simpler, but it adds quite a bit of data to the
omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.

ls -al vmlinux

w/o any the lockdep warning fixes:
110109168

With my series applied:
110112031 (base + 2863)

With setting individual lockdep class:
110114275 (base + 5107)

I certainly like the lockdep_set_class() way since it is cleaner, but it adds
almost double amount of bytes to the kernel.
I'll test the patch on my board tomorrow as first thing.

-- 
Péter

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 04:13 PM, Peter Zijlstra wrote:
> On Fri, Feb 06, 2015 at 02:48:34PM +0200, Peter Ujfalusi wrote:
>> Hi,
>>
>> In case when hwmods are used in nested way the lockdep validator will print out
>> a warning message about possible deadlock situation:
>>
>> [    4.514882] =============================================
>> [    4.520530] [ INFO: possible recursive locking detected ]
>> [    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
>> [    4.532285] ---------------------------------------------
>> [    4.537936] kworker/u4:1/18 is trying to acquire lock:
>> [    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
>> [    4.552109] 
>> [    4.552109] but task is already holding lock:
>> [    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
>> [    4.566999] 
>> [    4.566999] other info that might help us debug this:
>> [    4.573831]  Possible unsafe locking scenario:
>> [    4.573831] 
>> [    4.580025]        CPU0
>> [    4.582584]        ----
>> [    4.585142]   lock(&(&oh->_lock)->rlock);
>> [    4.589544]   lock(&(&oh->_lock)->rlock);
>> [    4.593945] 
>> [    4.593945]  *** DEADLOCK ***
>> [    4.593945] 
>> [    4.600146]  May be due to missing lock nesting notation
>>
>> What lockdep did not realizes is that the two oh is not the same hwmod object
>> and we have taken different locks.
>>
>> One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
>> configured to use ATL clock as it's functional clock. In this case the
>> pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
>> operation it will enable the needed clocks. Since ATL clock is needed, we will
>> have another pm_runtime operation from the ATL clock enable callback which
>> will take the atl hwmod's lock. This will be seen by lockdep as possible
>> deadlock situation.
>>
>> In order to notify lockdep about this, we need to switch using _nested
>> version of locking function and assign different subclass to those hwmods which
>> could be used in nested way.
> 
> IFF struct omap_hwmod is always statically allocated; as I think the
> __init on _register() mandates, you can use the below annotation.
> 
> ---
>  arch/arm/mach-omap2/omap_hwmod.c | 1 +
>  arch/arm/mach-omap2/omap_hwmod.h | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 9025ffffd2dc..222d654ad6fd 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh)
>  	INIT_LIST_HEAD(&oh->master_ports);
>  	INIT_LIST_HEAD(&oh->slave_ports);
>  	spin_lock_init(&oh->_lock);
> +	lockdep_set_class(&oh->_lock, &oh->hwmod_key);
>  
>  	oh->_state = _HWMOD_STATE_REGISTERED;
>  
> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
> index 5b42fafcaf55..754bdfb3f811 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.h
> +++ b/arch/arm/mach-omap2/omap_hwmod.h
> @@ -689,6 +689,8 @@ struct omap_hwmod {
>  	u8				_state;
>  	u8				_postsetup_state;
>  	struct omap_hwmod		*parent_hwmod;
> +
> +	struct lock_class_key		hwmod_key;
>  };
>  
>  struct omap_hwmod *omap_hwmod_lookup(const char *name);

Certainly looks much simpler, but it adds quite a bit of data to the
omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.

ls -al vmlinux

w/o any the lockdep warning fixes:
110109168

With my series applied:
110112031 (base + 2863)

With setting individual lockdep class:
110114275 (base + 5107)

I certainly like the lockdep_set_class() way since it is cleaner, but it adds
almost double amount of bytes to the kernel.
I'll test the patch on my board tomorrow as first thing.

-- 
Péter

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 04:13 PM, Peter Zijlstra wrote:
> On Fri, Feb 06, 2015 at 02:48:34PM +0200, Peter Ujfalusi wrote:
>> Hi,
>>
>> In case when hwmods are used in nested way the lockdep validator will print out
>> a warning message about possible deadlock situation:
>>
>> [    4.514882] =============================================
>> [    4.520530] [ INFO: possible recursive locking detected ]
>> [    4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted
>> [    4.532285] ---------------------------------------------
>> [    4.537936] kworker/u4:1/18 is trying to acquire lock:
>> [    4.543317]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
>> [    4.552109] 
>> [    4.552109] but task is already holding lock:
>> [    4.558216]  (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58
>> [    4.566999] 
>> [    4.566999] other info that might help us debug this:
>> [    4.573831]  Possible unsafe locking scenario:
>> [    4.573831] 
>> [    4.580025]        CPU0
>> [    4.582584]        ----
>> [    4.585142]   lock(&(&oh->_lock)->rlock);
>> [    4.589544]   lock(&(&oh->_lock)->rlock);
>> [    4.593945] 
>> [    4.593945]  *** DEADLOCK ***
>> [    4.593945] 
>> [    4.600146]  May be due to missing lock nesting notation
>>
>> What lockdep did not realizes is that the two oh is not the same hwmod object
>> and we have taken different locks.
>>
>> One example of nested hwmod usage is on DRA7xx platforms, where McASP can be
>> configured to use ATL clock as it's functional clock. In this case the
>> pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this
>> operation it will enable the needed clocks. Since ATL clock is needed, we will
>> have another pm_runtime operation from the ATL clock enable callback which
>> will take the atl hwmod's lock. This will be seen by lockdep as possible
>> deadlock situation.
>>
>> In order to notify lockdep about this, we need to switch using _nested
>> version of locking function and assign different subclass to those hwmods which
>> could be used in nested way.
> 
> IFF struct omap_hwmod is always statically allocated; as I think the
> __init on _register() mandates, you can use the below annotation.
> 
> ---
>  arch/arm/mach-omap2/omap_hwmod.c | 1 +
>  arch/arm/mach-omap2/omap_hwmod.h | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 9025ffffd2dc..222d654ad6fd 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh)
>  	INIT_LIST_HEAD(&oh->master_ports);
>  	INIT_LIST_HEAD(&oh->slave_ports);
>  	spin_lock_init(&oh->_lock);
> +	lockdep_set_class(&oh->_lock, &oh->hwmod_key);
>  
>  	oh->_state = _HWMOD_STATE_REGISTERED;
>  
> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
> index 5b42fafcaf55..754bdfb3f811 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.h
> +++ b/arch/arm/mach-omap2/omap_hwmod.h
> @@ -689,6 +689,8 @@ struct omap_hwmod {
>  	u8				_state;
>  	u8				_postsetup_state;
>  	struct omap_hwmod		*parent_hwmod;
> +
> +	struct lock_class_key		hwmod_key;
>  };
>  
>  struct omap_hwmod *omap_hwmod_lookup(const char *name);

Certainly looks much simpler, but it adds quite a bit of data to the
omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.

ls -al vmlinux

w/o any the lockdep warning fixes:
110109168

With my series applied:
110112031 (base + 2863)

With setting individual lockdep class:
110114275 (base + 5107)

I certainly like the lockdep_set_class() way since it is cleaner, but it adds
almost double amount of bytes to the kernel.
I'll test the patch on my board tomorrow as first thing.

-- 
P?ter

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Zijlstra]

On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote:
> Certainly looks much simpler, but it adds quite a bit of data to the
> omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.
> 
> ls -al vmlinux
> 
> w/o any the lockdep warning fixes:
> 110109168
> 
> With my series applied:
> 110112031 (base + 2863)
> 
> With setting individual lockdep class:
> 110114275 (base + 5107)
> 
> I certainly like the lockdep_set_class() way since it is cleaner, but it adds
> almost double amount of bytes to the kernel.

Yeah, I've never really bothered with data too much, its a debug
feature. So lock_class_key is 8 bytes, and strictly speaking you could
union them over other fields, all we really need is unique addresses, we
don't actually use the storage.

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Zijlstra]

On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote:
> Certainly looks much simpler, but it adds quite a bit of data to the
> omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.
> 
> ls -al vmlinux
> 
> w/o any the lockdep warning fixes:
> 110109168
> 
> With my series applied:
> 110112031 (base + 2863)
> 
> With setting individual lockdep class:
> 110114275 (base + 5107)
> 
> I certainly like the lockdep_set_class() way since it is cleaner, but it adds
> almost double amount of bytes to the kernel.

Yeah, I've never really bothered with data too much, its a debug
feature. So lock_class_key is 8 bytes, and strictly speaking you could
union them over other fields, all we really need is unique addresses, we
don't actually use the storage.

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 08:32 PM, Peter Zijlstra wrote:
> On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote:
>> Certainly looks much simpler, but it adds quite a bit of data to the
>> omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.
>>
>> ls -al vmlinux
>>
>> w/o any the lockdep warning fixes:
>> 110109168
>>
>> With my series applied:
>> 110112031 (base + 2863)
>>
>> With setting individual lockdep class:
>> 110114275 (base + 5107)
>>
>> I certainly like the lockdep_set_class() way since it is cleaner, but it adds
>> almost double amount of bytes to the kernel.
> 
> Yeah, I've never really bothered with data too much, its a debug
> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> union them over other fields, all we really need is unique addresses, we
> don't actually use the storage.

True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
add anything to the data when running default kernel.
I'll test the lockdep_set_class() method you suggested on Monday (not
tomorrow), but still as first thing.
If it is working as expected I'll send a patch with you as author.

Thanks,
Péter

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 08:32 PM, Peter Zijlstra wrote:
> On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote:
>> Certainly looks much simpler, but it adds quite a bit of data to the
>> omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.
>>
>> ls -al vmlinux
>>
>> w/o any the lockdep warning fixes:
>> 110109168
>>
>> With my series applied:
>> 110112031 (base + 2863)
>>
>> With setting individual lockdep class:
>> 110114275 (base + 5107)
>>
>> I certainly like the lockdep_set_class() way since it is cleaner, but it adds
>> almost double amount of bytes to the kernel.
> 
> Yeah, I've never really bothered with data too much, its a debug
> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> union them over other fields, all we really need is unique addresses, we
> don't actually use the storage.

True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
add anything to the data when running default kernel.
I'll test the lockdep_set_class() method you suggested on Monday (not
tomorrow), but still as first thing.
If it is working as expected I'll send a patch with you as author.

Thanks,
Péter

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 08:32 PM, Peter Zijlstra wrote:
> On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote:
>> Certainly looks much simpler, but it adds quite a bit of data to the
>> omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration.
>>
>> ls -al vmlinux
>>
>> w/o any the lockdep warning fixes:
>> 110109168
>>
>> With my series applied:
>> 110112031 (base + 2863)
>>
>> With setting individual lockdep class:
>> 110114275 (base + 5107)
>>
>> I certainly like the lockdep_set_class() way since it is cleaner, but it adds
>> almost double amount of bytes to the kernel.
> 
> Yeah, I've never really bothered with data too much, its a debug
> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> union them over other fields, all we really need is unique addresses, we
> don't actually use the storage.

True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
add anything to the data when running default kernel.
I'll test the lockdep_set_class() method you suggested on Monday (not
tomorrow), but still as first thing.
If it is working as expected I'll send a patch with you as author.

Thanks,
P?ter

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
>> Yeah, I've never really bothered with data too much, its a debug
>> feature. So lock_class_key is 8 bytes, and strictly speaking you could
>> union them over other fields, all we really need is unique addresses, we
>> don't actually use the storage.
> 
> True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> add anything to the data when running default kernel.
> I'll test the lockdep_set_class() method you suggested on Monday (not
> tomorrow), but still as first thing.
> If it is working as expected I'll send a patch with you as author.

With omap2plus_defconfig my build produces (vmlinux size):
Base: 				99905522
with my series:			99908385 (base + 2863)
with Peter Zijlstra's patch:	99910625 (base + 5103)

The reason for this is that we will only have
struct lock_class_key { };
in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
default, while the CONFIG_DEBUG_LOCKDEP is disabled.

So it does add more data to our default omap2plus config.

Tony: do you have preference on the way we fix this issue?

As I recall there is a plan to remove the hwmod static database and move it or
generate it from DT? Not sure when and how this will be done, but will it
affect the lockdep_set_class() way?

-- 
Péter

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
>> Yeah, I've never really bothered with data too much, its a debug
>> feature. So lock_class_key is 8 bytes, and strictly speaking you could
>> union them over other fields, all we really need is unique addresses, we
>> don't actually use the storage.
> 
> True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> add anything to the data when running default kernel.
> I'll test the lockdep_set_class() method you suggested on Monday (not
> tomorrow), but still as first thing.
> If it is working as expected I'll send a patch with you as author.

With omap2plus_defconfig my build produces (vmlinux size):
Base: 				99905522
with my series:			99908385 (base + 2863)
with Peter Zijlstra's patch:	99910625 (base + 5103)

The reason for this is that we will only have
struct lock_class_key { };
in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
default, while the CONFIG_DEBUG_LOCKDEP is disabled.

So it does add more data to our default omap2plus config.

Tony: do you have preference on the way we fix this issue?

As I recall there is a plan to remove the hwmod static database and move it or
generate it from DT? Not sure when and how this will be done, but will it
affect the lockdep_set_class() way?

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Ujfalusi]

On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
>> Yeah, I've never really bothered with data too much, its a debug
>> feature. So lock_class_key is 8 bytes, and strictly speaking you could
>> union them over other fields, all we really need is unique addresses, we
>> don't actually use the storage.
> 
> True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> add anything to the data when running default kernel.
> I'll test the lockdep_set_class() method you suggested on Monday (not
> tomorrow), but still as first thing.
> If it is working as expected I'll send a patch with you as author.

With omap2plus_defconfig my build produces (vmlinux size):
Base: 				99905522
with my series:			99908385 (base + 2863)
with Peter Zijlstra's patch:	99910625 (base + 5103)

The reason for this is that we will only have
struct lock_class_key { };
in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
default, while the CONFIG_DEBUG_LOCKDEP is disabled.

So it does add more data to our default omap2plus config.

Tony: do you have preference on the way we fix this issue?

As I recall there is a plan to remove the hwmod static database and move it or
generate it from DT? Not sure when and how this will be done, but will it
affect the lockdep_set_class() way?

-- 
P?ter

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Zijlstra]

On Mon, Feb 09, 2015 at 10:27:32AM +0200, Peter Ujfalusi wrote:
> As I recall there is a plan to remove the hwmod static database and move it or
> generate it from DT? Not sure when and how this will be done, but will it
> affect the lockdep_set_class() way?

Yes, struct lock_class_key wants to be in static storage.

So you could still allocate a few of those statically and then use them
where appropriate, but it'd going to be more work.

The advantage of having the 1:1 relation is that any parent hierarchy
works naturally.

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Peter Zijlstra]

On Mon, Feb 09, 2015 at 10:27:32AM +0200, Peter Ujfalusi wrote:
> As I recall there is a plan to remove the hwmod static database and move it or
> generate it from DT? Not sure when and how this will be done, but will it
> affect the lockdep_set_class() way?

Yes, struct lock_class_key wants to be in static storage.

So you could still allocate a few of those statically and then use them
where appropriate, but it'd going to be more work.

The advantage of having the 1:1 relation is that any parent hierarchy
works naturally.

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Paul Walmsley]

On Mon, 9 Feb 2015, Peter Ujfalusi wrote:

> On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> >> Yeah, I've never really bothered with data too much, its a debug
> >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> >> union them over other fields, all we really need is unique addresses, we
> >> don't actually use the storage.
> > 
> > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > add anything to the data when running default kernel.
> > I'll test the lockdep_set_class() method you suggested on Monday (not
> > tomorrow), but still as first thing.
> > If it is working as expected I'll send a patch with you as author.
> 
> With omap2plus_defconfig my build produces (vmlinux size):
> Base: 				99905522
> with my series:			99908385 (base + 2863)
> with Peter Zijlstra's patch:	99910625 (base + 5103)
> 
> The reason for this is that we will only have
> struct lock_class_key { };
> in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> 
> So it does add more data to our default omap2plus config.
> 
> Tony: do you have preference on the way we fix this issue?
> 
> As I recall there is a plan to remove the hwmod static database and move it or
> generate it from DT? Not sure when and how this will be done, but will it
> affect the lockdep_set_class() way?

Well I guess we could see what Tony says, but you do realize that the 
difference in sizes that you posted above is about .003% of the total 
binary size, right?  

If there's one thing we can say about the last few years of ARM kernel 
development, it's that those kind of size increases are utterly dwarfed by 
other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
fix and be done with it...


- Paul

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Paul Walmsley]

On Mon, 9 Feb 2015, Peter Ujfalusi wrote:

> On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> >> Yeah, I've never really bothered with data too much, its a debug
> >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> >> union them over other fields, all we really need is unique addresses, we
> >> don't actually use the storage.
> > 
> > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > add anything to the data when running default kernel.
> > I'll test the lockdep_set_class() method you suggested on Monday (not
> > tomorrow), but still as first thing.
> > If it is working as expected I'll send a patch with you as author.
> 
> With omap2plus_defconfig my build produces (vmlinux size):
> Base: 				99905522
> with my series:			99908385 (base + 2863)
> with Peter Zijlstra's patch:	99910625 (base + 5103)
> 
> The reason for this is that we will only have
> struct lock_class_key { };
> in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> 
> So it does add more data to our default omap2plus config.
> 
> Tony: do you have preference on the way we fix this issue?
> 
> As I recall there is a plan to remove the hwmod static database and move it or
> generate it from DT? Not sure when and how this will be done, but will it
> affect the lockdep_set_class() way?

Well I guess we could see what Tony says, but you do realize that the 
difference in sizes that you posted above is about .003% of the total 
binary size, right?  

If there's one thing we can say about the last few years of ARM kernel 
development, it's that those kind of size increases are utterly dwarfed by 
other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
fix and be done with it...


- Paul

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> 
> > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > >> Yeah, I've never really bothered with data too much, its a debug
> > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > >> union them over other fields, all we really need is unique addresses, we
> > >> don't actually use the storage.
> > > 
> > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > add anything to the data when running default kernel.
> > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > tomorrow), but still as first thing.
> > > If it is working as expected I'll send a patch with you as author.
> > 
> > With omap2plus_defconfig my build produces (vmlinux size):
> > Base: 				99905522
> > with my series:			99908385 (base + 2863)
> > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > 
> > The reason for this is that we will only have
> > struct lock_class_key { };
> > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > 
> > So it does add more data to our default omap2plus config.
> > 
> > Tony: do you have preference on the way we fix this issue?
> > 
> > As I recall there is a plan to remove the hwmod static database and move it or
> > generate it from DT? Not sure when and how this will be done, but will it
> > affect the lockdep_set_class() way?
> 
> Well I guess we could see what Tony says, but you do realize that the 
> difference in sizes that you posted above is about .003% of the total 
> binary size, right?  
> 
> If there's one thing we can say about the last few years of ARM kernel 
> development, it's that those kind of size increases are utterly dwarfed by 
> other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> fix and be done with it...

Well the thing to consider here is what Peter U is saying about
having struct omap_hwmod allocated based on the data from .dts
files. If the fix makes the dynamic allocation harder to do later on,
we should probably avoid it. If it's relatively easy to do later on,
then I don't have a problem with it.

Regards,

Tony

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> 
> > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > >> Yeah, I've never really bothered with data too much, its a debug
> > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > >> union them over other fields, all we really need is unique addresses, we
> > >> don't actually use the storage.
> > > 
> > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > add anything to the data when running default kernel.
> > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > tomorrow), but still as first thing.
> > > If it is working as expected I'll send a patch with you as author.
> > 
> > With omap2plus_defconfig my build produces (vmlinux size):
> > Base: 				99905522
> > with my series:			99908385 (base + 2863)
> > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > 
> > The reason for this is that we will only have
> > struct lock_class_key { };
> > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > 
> > So it does add more data to our default omap2plus config.
> > 
> > Tony: do you have preference on the way we fix this issue?
> > 
> > As I recall there is a plan to remove the hwmod static database and move it or
> > generate it from DT? Not sure when and how this will be done, but will it
> > affect the lockdep_set_class() way?
> 
> Well I guess we could see what Tony says, but you do realize that the 
> difference in sizes that you posted above is about .003% of the total 
> binary size, right?  
> 
> If there's one thing we can say about the last few years of ARM kernel 
> development, it's that those kind of size increases are utterly dwarfed by 
> other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> fix and be done with it...

Well the thing to consider here is what Peter U is saying about
having struct omap_hwmod allocated based on the data from .dts
files. If the fix makes the dynamic allocation harder to do later on,
we should probably avoid it. If it's relatively easy to do later on,
then I don't have a problem with it.

Regards,

Tony

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Paul Walmsley]

On Mon, 9 Feb 2015, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> > On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> > 
> > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > > >> Yeah, I've never really bothered with data too much, its a debug
> > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > > >> union them over other fields, all we really need is unique addresses, we
> > > >> don't actually use the storage.
> > > > 
> > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > > add anything to the data when running default kernel.
> > > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > > tomorrow), but still as first thing.
> > > > If it is working as expected I'll send a patch with you as author.
> > > 
> > > With omap2plus_defconfig my build produces (vmlinux size):
> > > Base: 				99905522
> > > with my series:			99908385 (base + 2863)
> > > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > > 
> > > The reason for this is that we will only have
> > > struct lock_class_key { };
> > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > > 
> > > So it does add more data to our default omap2plus config.
> > > 
> > > Tony: do you have preference on the way we fix this issue?
> > > 
> > > As I recall there is a plan to remove the hwmod static database and move it or
> > > generate it from DT? Not sure when and how this will be done, but will it
> > > affect the lockdep_set_class() way?
> > 
> > Well I guess we could see what Tony says, but you do realize that the 
> > difference in sizes that you posted above is about .003% of the total 
> > binary size, right?  
> > 
> > If there's one thing we can say about the last few years of ARM kernel 
> > development, it's that those kind of size increases are utterly dwarfed by 
> > other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> > fix and be done with it...
> 
> Well the thing to consider here is what Peter U is saying about
> having struct omap_hwmod allocated based on the data from .dts
> files. If the fix makes the dynamic allocation harder to do later on,
> we should probably avoid it. If it's relatively easy to do later on,
> then I don't have a problem with it.

The future destination for that code that makes the most sense to me is 
for it to become integrated with the OMAP Sonics & Arteris bus drivers and 
DT data.  So I wouldn't worry too much about it; I don't think the 
lockdep fix will affect that at all.


- Paul

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Paul Walmsley]

On Mon, 9 Feb 2015, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> > On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> > 
> > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > > >> Yeah, I've never really bothered with data too much, its a debug
> > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > > >> union them over other fields, all we really need is unique addresses, we
> > > >> don't actually use the storage.
> > > > 
> > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > > add anything to the data when running default kernel.
> > > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > > tomorrow), but still as first thing.
> > > > If it is working as expected I'll send a patch with you as author.
> > > 
> > > With omap2plus_defconfig my build produces (vmlinux size):
> > > Base: 				99905522
> > > with my series:			99908385 (base + 2863)
> > > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > > 
> > > The reason for this is that we will only have
> > > struct lock_class_key { };
> > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > > 
> > > So it does add more data to our default omap2plus config.
> > > 
> > > Tony: do you have preference on the way we fix this issue?
> > > 
> > > As I recall there is a plan to remove the hwmod static database and move it or
> > > generate it from DT? Not sure when and how this will be done, but will it
> > > affect the lockdep_set_class() way?
> > 
> > Well I guess we could see what Tony says, but you do realize that the 
> > difference in sizes that you posted above is about .003% of the total 
> > binary size, right?  
> > 
> > If there's one thing we can say about the last few years of ARM kernel 
> > development, it's that those kind of size increases are utterly dwarfed by 
> > other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> > fix and be done with it...
> 
> Well the thing to consider here is what Peter U is saying about
> having struct omap_hwmod allocated based on the data from .dts
> files. If the fix makes the dynamic allocation harder to do later on,
> we should probably avoid it. If it's relatively easy to do later on,
> then I don't have a problem with it.

The future destination for that code that makes the most sense to me is 
for it to become integrated with the OMAP Sonics & Arteris bus drivers and 
DT data.  So I wouldn't worry too much about it; I don't think the 
lockdep fix will affect that at all.


- Paul

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [150209 10:59]:
> On Mon, 9 Feb 2015, Tony Lindgren wrote:
> 
> > * Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> > > On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> > > 
> > > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > > > >> Yeah, I've never really bothered with data too much, its a debug
> > > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > > > >> union them over other fields, all we really need is unique addresses, we
> > > > >> don't actually use the storage.
> > > > > 
> > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > > > add anything to the data when running default kernel.
> > > > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > > > tomorrow), but still as first thing.
> > > > > If it is working as expected I'll send a patch with you as author.
> > > > 
> > > > With omap2plus_defconfig my build produces (vmlinux size):
> > > > Base: 				99905522
> > > > with my series:			99908385 (base + 2863)
> > > > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > > > 
> > > > The reason for this is that we will only have
> > > > struct lock_class_key { };
> > > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > > > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > > > 
> > > > So it does add more data to our default omap2plus config.
> > > > 
> > > > Tony: do you have preference on the way we fix this issue?
> > > > 
> > > > As I recall there is a plan to remove the hwmod static database and move it or
> > > > generate it from DT? Not sure when and how this will be done, but will it
> > > > affect the lockdep_set_class() way?
> > > 
> > > Well I guess we could see what Tony says, but you do realize that the 
> > > difference in sizes that you posted above is about .003% of the total 
> > > binary size, right?  
> > > 
> > > If there's one thing we can say about the last few years of ARM kernel 
> > > development, it's that those kind of size increases are utterly dwarfed by 
> > > other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> > > fix and be done with it...
> > 
> > Well the thing to consider here is what Peter U is saying about
> > having struct omap_hwmod allocated based on the data from .dts
> > files. If the fix makes the dynamic allocation harder to do later on,
> > we should probably avoid it. If it's relatively easy to do later on,
> > then I don't have a problem with it.
> 
> The future destination for that code that makes the most sense to me is 
> for it to become integrated with the OMAP Sonics & Arteris bus drivers and 
> DT data.  So I wouldn't worry too much about it; I don't think the 
> lockdep fix will affect that at all.

OK up to you guys then.

Regards,

Tony

Re: [PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [150209 10:59]:
> On Mon, 9 Feb 2015, Tony Lindgren wrote:
> 
> > * Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> > > On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> > > 
> > > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > > > >> Yeah, I've never really bothered with data too much, its a debug
> > > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > > > >> union them over other fields, all we really need is unique addresses, we
> > > > >> don't actually use the storage.
> > > > > 
> > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > > > add anything to the data when running default kernel.
> > > > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > > > tomorrow), but still as first thing.
> > > > > If it is working as expected I'll send a patch with you as author.
> > > > 
> > > > With omap2plus_defconfig my build produces (vmlinux size):
> > > > Base: 				99905522
> > > > with my series:			99908385 (base + 2863)
> > > > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > > > 
> > > > The reason for this is that we will only have
> > > > struct lock_class_key { };
> > > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > > > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > > > 
> > > > So it does add more data to our default omap2plus config.
> > > > 
> > > > Tony: do you have preference on the way we fix this issue?
> > > > 
> > > > As I recall there is a plan to remove the hwmod static database and move it or
> > > > generate it from DT? Not sure when and how this will be done, but will it
> > > > affect the lockdep_set_class() way?
> > > 
> > > Well I guess we could see what Tony says, but you do realize that the 
> > > difference in sizes that you posted above is about .003% of the total 
> > > binary size, right?  
> > > 
> > > If there's one thing we can say about the last few years of ARM kernel 
> > > development, it's that those kind of size increases are utterly dwarfed by 
> > > other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> > > fix and be done with it...
> > 
> > Well the thing to consider here is what Peter U is saying about
> > having struct omap_hwmod allocated based on the data from .dts
> > files. If the fix makes the dynamic allocation harder to do later on,
> > we should probably avoid it. If it's relatively easy to do later on,
> > then I don't have a problem with it.
> 
> The future destination for that code that makes the most sense to me is 
> for it to become integrated with the OMAP Sonics & Arteris bus drivers and 
> DT data.  So I wouldn't worry too much about it; I don't think the 
> lockdep fix will affect that at all.

OK up to you guys then.

Regards,

Tony

[PATCH 0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [150209 10:59]:
> On Mon, 9 Feb 2015, Tony Lindgren wrote:
> 
> > * Paul Walmsley <paul@pwsan.com> [150209 08:04]:
> > > On Mon, 9 Feb 2015, Peter Ujfalusi wrote:
> > > 
> > > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote:
> > > > >> Yeah, I've never really bothered with data too much, its a debug
> > > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could
> > > > >> union them over other fields, all we really need is unique addresses, we
> > > > >> don't actually use the storage.
> > > > > 
> > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not
> > > > > add anything to the data when running default kernel.
> > > > > I'll test the lockdep_set_class() method you suggested on Monday (not
> > > > > tomorrow), but still as first thing.
> > > > > If it is working as expected I'll send a patch with you as author.
> > > > 
> > > > With omap2plus_defconfig my build produces (vmlinux size):
> > > > Base: 				99905522
> > > > with my series:			99908385 (base + 2863)
> > > > with Peter Zijlstra's patch:	99910625 (base + 5103)
> > > > 
> > > > The reason for this is that we will only have
> > > > struct lock_class_key { };
> > > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by
> > > > default, while the CONFIG_DEBUG_LOCKDEP is disabled.
> > > > 
> > > > So it does add more data to our default omap2plus config.
> > > > 
> > > > Tony: do you have preference on the way we fix this issue?
> > > > 
> > > > As I recall there is a plan to remove the hwmod static database and move it or
> > > > generate it from DT? Not sure when and how this will be done, but will it
> > > > affect the lockdep_set_class() way?
> > > 
> > > Well I guess we could see what Tony says, but you do realize that the 
> > > difference in sizes that you posted above is about .003% of the total 
> > > binary size, right?  
> > > 
> > > If there's one thing we can say about the last few years of ARM kernel 
> > > development, it's that those kind of size increases are utterly dwarfed by 
> > > other changes in the kernel.  So I'd say, post a patch based on PeterZ's 
> > > fix and be done with it...
> > 
> > Well the thing to consider here is what Peter U is saying about
> > having struct omap_hwmod allocated based on the data from .dts
> > files. If the fix makes the dynamic allocation harder to do later on,
> > we should probably avoid it. If it's relatively easy to do later on,
> > then I don't have a problem with it.
> 
> The future destination for that code that makes the most sense to me is 
> for it to become integrated with the OMAP Sonics & Arteris bus drivers and 
> DT data.  So I wouldn't worry too much about it; I don't think the 
> lockdep fix will affect that at all.

OK up to you guys then.

Regards,

Tony