[PATCH, once again] regulator: core: avoid unused variable warning

[PATCH, once again] regulator: core: avoid unused variable warning

[Arnd Bergmann]

The second argument of the mutex_lock_nested() helper is only
evaluated if CONFIG_DEBUG_LOCK_ALLOC is set. Otherwise we
get this build warning for the new regulator_lock_supply
function:

drivers/regulator/core.c: In function 'regulator_lock_supply':
drivers/regulator/core.c:142:6: warning: unused variable 'i' [-Wunused-variable]

To avoid the warning, this patch moves the postincrement outside
of the call mutex_lock_nested(), which is enough to shut up
gcc about it.

We had some discussion about changing mutex_lock_nested to an
inline function, which would make the code do the right thing here,
but in the end decided against it, in order to guarantee that
mutex_lock_nested() does not introduced overhead without
CONFIG_DEBUG_LOCK_ALLOC.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 9f01cd4a915 ("regulator: core: introduce function to lock regulators and its supplies")
Link: http://permalink.gmane.org/gmane.linux.kernel/2068900
---
The patch that introduced the warning is now in 4.4-rc1, and I think this
patch is still the least ugly workaround we found.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4cf1390784e5..cf5371ee0be4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -142,7 +142,9 @@ static void regulator_lock_supply(struct regulator_dev *rdev)
 	int i = 0;
 
 	while (1) {
-		mutex_lock_nested(&rdev->mutex, i++);
+		mutex_lock_nested(&rdev->mutex, i);
+		i++;
+
 		supply = rdev->supply;
 
 		if (!rdev->supply)

[PATCH, once again] regulator: core: avoid unused variable warning

[Arnd Bergmann]

The second argument of the mutex_lock_nested() helper is only
evaluated if CONFIG_DEBUG_LOCK_ALLOC is set. Otherwise we
get this build warning for the new regulator_lock_supply
function:

drivers/regulator/core.c: In function 'regulator_lock_supply':
drivers/regulator/core.c:142:6: warning: unused variable 'i' [-Wunused-variable]

To avoid the warning, this patch moves the postincrement outside
of the call mutex_lock_nested(), which is enough to shut up
gcc about it.

We had some discussion about changing mutex_lock_nested to an
inline function, which would make the code do the right thing here,
but in the end decided against it, in order to guarantee that
mutex_lock_nested() does not introduced overhead without
CONFIG_DEBUG_LOCK_ALLOC.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 9f01cd4a915 ("regulator: core: introduce function to lock regulators and its supplies")
Link: http://permalink.gmane.org/gmane.linux.kernel/2068900
---
The patch that introduced the warning is now in 4.4-rc1, and I think this
patch is still the least ugly workaround we found.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4cf1390784e5..cf5371ee0be4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -142,7 +142,9 @@ static void regulator_lock_supply(struct regulator_dev *rdev)
 	int i = 0;
 
 	while (1) {
-		mutex_lock_nested(&rdev->mutex, i++);
+		mutex_lock_nested(&rdev->mutex, i);
+		i++;
+
 		supply = rdev->supply;
 
 		if (!rdev->supply)

Re: [PATCH, once again] regulator: core: avoid unused variable warning

[Mark Brown]

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

On Fri, Nov 20, 2015 at 12:30:24PM +0100, Arnd Bergmann wrote:

> The patch that introduced the warning is now in 4.4-rc1, and I think this
> patch is still the least ugly workaround we found.

Can we please at least have a comment explaining that this is working
around lockdep limitations?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH, once again] regulator: core: avoid unused variable warning

[Mark Brown]

On Fri, Nov 20, 2015 at 12:30:24PM +0100, Arnd Bergmann wrote:

> The patch that introduced the warning is now in 4.4-rc1, and I think this
> patch is still the least ugly workaround we found.

Can we please at least have a comment explaining that this is working
around lockdep limitations?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151120/faef10f5/attachment.sig>

Re: [PATCH, once again] regulator: core: avoid unused variable warning

[Arnd Bergmann]

On Friday 20 November 2015 11:41:27 Mark Brown wrote:
> On Fri, Nov 20, 2015 at 12:30:24PM +0100, Arnd Bergmann wrote:
> 
> > The patch that introduced the warning is now in 4.4-rc1, and I think this
> > patch is still the least ugly workaround we found.
> 
> Can we please at least have a comment explaining that this is working
> around lockdep limitations?

Not sure which limitation you are referring to. Maybe you could just
modify the changelog text as you like when applying the patch?

The limitation we talked about in the previous thread was about the maximum
of 8 nesting levels, but my patch here doesn't address that at all.

I tried to capture the fact that mutex_lock_nested() intentionally
doesn't evaluate its second argument when CONFIG_DEBUG_LOCK_ALLOC
is not set, but that appears to be less of a limitation than a
choice of the interface.

	Arnd

[PATCH, once again] regulator: core: avoid unused variable warning

[Arnd Bergmann]

On Friday 20 November 2015 11:41:27 Mark Brown wrote:
> On Fri, Nov 20, 2015 at 12:30:24PM +0100, Arnd Bergmann wrote:
> 
> > The patch that introduced the warning is now in 4.4-rc1, and I think this
> > patch is still the least ugly workaround we found.
> 
> Can we please at least have a comment explaining that this is working
> around lockdep limitations?

Not sure which limitation you are referring to. Maybe you could just
modify the changelog text as you like when applying the patch?

The limitation we talked about in the previous thread was about the maximum
of 8 nesting levels, but my patch here doesn't address that at all.

I tried to capture the fact that mutex_lock_nested() intentionally
doesn't evaluate its second argument when CONFIG_DEBUG_LOCK_ALLOC
is not set, but that appears to be less of a limitation than a
choice of the interface.

	Arnd

Re: [PATCH, once again] regulator: core: avoid unused variable warning

[Mark Brown]

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

On Fri, Nov 20, 2015 at 01:12:00PM +0100, Arnd Bergmann wrote:
> On Friday 20 November 2015 11:41:27 Mark Brown wrote:

> > Can we please at least have a comment explaining that this is working
> > around lockdep limitations?

> Not sure which limitation you are referring to. Maybe you could just
> modify the changelog text as you like when applying the patch?

> I tried to capture the fact that mutex_lock_nested() intentionally
> doesn't evaluate its second argument when CONFIG_DEBUG_LOCK_ALLOC
> is not set, but that appears to be less of a limitation than a
> choice of the interface.

That's the limitation (or intereface choice or whatever) that I'm
talking about - the code looks like a function call so not evaulating
the second argument is surprising.  I'm looking for something in the
code rather than the changelog so it doesn't get cleaned up later.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH, once again] regulator: core: avoid unused variable warning

[Mark Brown]

On Fri, Nov 20, 2015 at 01:12:00PM +0100, Arnd Bergmann wrote:
> On Friday 20 November 2015 11:41:27 Mark Brown wrote:

> > Can we please at least have a comment explaining that this is working
> > around lockdep limitations?

> Not sure which limitation you are referring to. Maybe you could just
> modify the changelog text as you like when applying the patch?

> I tried to capture the fact that mutex_lock_nested() intentionally
> doesn't evaluate its second argument when CONFIG_DEBUG_LOCK_ALLOC
> is not set, but that appears to be less of a limitation than a
> choice of the interface.

That's the limitation (or intereface choice or whatever) that I'm
talking about - the code looks like a function call so not evaulating
the second argument is surprising.  I'm looking for something in the
code rather than the changelog so it doesn't get cleaned up later.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151120/5b9d2824/attachment.sig>

Re: [PATCH, once again] regulator: core: avoid unused variable warning

[Arnd Bergmann]

On Friday 20 November 2015 12:24:00 Mark Brown wrote:
> On Fri, Nov 20, 2015 at 01:12:00PM +0100, Arnd Bergmann wrote:
> > On Friday 20 November 2015 11:41:27 Mark Brown wrote:
> 
> > > Can we please at least have a comment explaining that this is working
> > > around lockdep limitations?
> 
> > Not sure which limitation you are referring to. Maybe you could just
> > modify the changelog text as you like when applying the patch?
> 
> > I tried to capture the fact that mutex_lock_nested() intentionally
> > doesn't evaluate its second argument when CONFIG_DEBUG_LOCK_ALLOC
> > is not set, but that appears to be less of a limitation than a
> > choice of the interface.
> 
> That's the limitation (or intereface choice or whatever) that I'm
> talking about - the code looks like a function call so not evaulating
> the second argument is surprising.  I'm looking for something in the
> code rather than the changelog so it doesn't get cleaned up later.
> 

Got it. Will send a new version soon.

	Arnd

[PATCH, once again] regulator: core: avoid unused variable warning

[Arnd Bergmann]

On Friday 20 November 2015 12:24:00 Mark Brown wrote:
> On Fri, Nov 20, 2015 at 01:12:00PM +0100, Arnd Bergmann wrote:
> > On Friday 20 November 2015 11:41:27 Mark Brown wrote:
> 
> > > Can we please at least have a comment explaining that this is working
> > > around lockdep limitations?
> 
> > Not sure which limitation you are referring to. Maybe you could just
> > modify the changelog text as you like when applying the patch?
> 
> > I tried to capture the fact that mutex_lock_nested() intentionally
> > doesn't evaluate its second argument when CONFIG_DEBUG_LOCK_ALLOC
> > is not set, but that appears to be less of a limitation than a
> > choice of the interface.
> 
> That's the limitation (or intereface choice or whatever) that I'm
> talking about - the code looks like a function call so not evaulating
> the second argument is surprising.  I'm looking for something in the
> code rather than the changelog so it doesn't get cleaned up later.
> 

Got it. Will send a new version soon.

	Arnd