[PATCH] regulator: lock supply in regulator enable

[PATCH] regulator: lock supply in regulator enable

[Mattias Wallin]

This patch add locks around regulator supply enable.

Signed-off-by: Mattias Wallin <mattias.wallin@stericsson.com>
---
The previous patch I sent solves a problem seen in our system.
This patch does not solve a problem I have seen but I still think
it should be there. Or at least some locking of the supply in regulator enable.
What do you guys think?
---
 drivers/regulator/core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c625633..d10ad4a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1270,7 +1270,9 @@ static int _regulator_enable(struct regulator_dev *rdev)
 
 	/* do we need to enable the supply regulator first */
 	if (rdev->supply) {
+		mutex_lock(&rdev->supply->mutex);
 		ret = _regulator_enable(rdev->supply);
+		mutex_unlock(&rdev->supply->mutex);
 		if (ret < 0) {
 			printk(KERN_ERR "%s: failed to enable %s: %d\n",
 			       __func__, rdev_get_name(rdev), ret);
-- 
1.6.3.3

Re: [PATCH] regulator: lock supply in regulator enable

[Liam Girdwood]

On Thu, 2010-11-04 at 11:01 +0100, Mattias Wallin wrote:
> This patch add locks around regulator supply enable.
> 
> Signed-off-by: Mattias Wallin <mattias.wallin@stericsson.com>
> ---
> The previous patch I sent solves a problem seen in our system.
> This patch does not solve a problem I have seen but I still think
> it should be there. Or at least some locking of the supply in regulator enable.
> What do you guys think?

This sounds like guesswork. What exactly is the problem in your system ?

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] regulator: lock supply in regulator enable

[Mattias Wallin]

As I wrote, the problem that I had is solved with my previous patch.
Right now I have no visible problem but I still think there is locks missing
and would like your opinion on it.

/Wallin

On 11/04/2010 11:18 AM, Liam Girdwood wrote:
> On Thu, 2010-11-04 at 11:01 +0100, Mattias Wallin wrote:
>> This patch add locks around regulator supply enable.
>>
>> Signed-off-by: Mattias Wallin <mattias.wallin@stericsson.com>
>> ---
>> The previous patch I sent solves a problem seen in our system.
>> This patch does not solve a problem I have seen but I still think
>> it should be there. Or at least some locking of the supply in regulator enable.
>> What do you guys think?
> 
> This sounds like guesswork. What exactly is the problem in your system ?
> 
> Liam

Re: [PATCH] regulator: lock supply in regulator enable

[Liam Girdwood]

On Thu, 2010-11-04 at 11:49 +0100, Mattias Wallin wrote:
> As I wrote, the problem that I had is solved with my previous patch.
> Right now I have no visible problem but I still think there is locks missing
> and would like your opinion on it.
> 
> /Wallin
> 
> On 11/04/2010 11:18 AM, Liam Girdwood wrote:
> > On Thu, 2010-11-04 at 11:01 +0100, Mattias Wallin wrote:
> >> This patch add locks around regulator supply enable.
> >>
> >> Signed-off-by: Mattias Wallin <mattias.wallin@stericsson.com>
> >> ---
> >> The previous patch I sent solves a problem seen in our system.
> >> This patch does not solve a problem I have seen but I still think
> >> it should be there. Or at least some locking of the supply in regulator enable.
> >> What do you guys think?
> > 
> > This sounds like guesswork. What exactly is the problem in your system ?
> > 

Sorry, got a busy schedule atm. Can you give us your reasoning behind
why you think we need a lock here ?

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] regulator: lock supply in regulator enable

[Mattias Wallin]

> Sorry, got a busy schedule atm. Can you give us your reasoning behind
> why you think we need a lock here ?
Yes, when we enter regulator_enable() we take the lock only for that specific regulator rdev->mutex
and calls the locked function _regulator_enable().
This locked function then checks if we have a supply and recursively calls the locked _regulator_enable() again but with the supply rdev as argument.
The supply rdev regulator is however not locked at this stage, only the "original" supplied regulator is locked.
So if we have two regulators with the same supply regulator trying to enable at approx. the same time they will both
enter the _regulator_enable without locks and we could get a race condition.
This would probably result in reference counting error and unbalanced regulator warnings.

In our system we make use the regulator hierarchy quite heavily.
> 
> Thanks
> 
> Liam

/Mattias Wallin

Re: [PATCH] regulator: lock supply in regulator enable

[Liam Girdwood]

On Thu, 2010-11-04 at 12:43 +0100, Mattias Wallin wrote:
> > Sorry, got a busy schedule atm. Can you give us your reasoning behind
> > why you think we need a lock here ?
> Yes, when we enter regulator_enable() we take the lock only for that specific regulator rdev->mutex
> and calls the locked function _regulator_enable().
> This locked function then checks if we have a supply and recursively calls the locked _regulator_enable() again but with the supply rdev as argument.
> The supply rdev regulator is however not locked at this stage, only the "original" supplied regulator is locked.
> So if we have two regulators with the same supply regulator trying to enable at approx. the same time they will both
> enter the _regulator_enable without locks and we could get a race condition.
> This would probably result in reference counting error and unbalanced regulator warnings.
> 
> In our system we make use the regulator hierarchy quite heavily.
> > 

Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] regulator: lock supply in regulator enable

[Mark Brown]

On Thu, Nov 04, 2010 at 11:01:31AM +0100, Mattias Wallin wrote:
> This patch add locks around regulator supply enable.
> 
> Signed-off-by: Mattias Wallin <mattias.wallin@stericsson.com>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

> The previous patch I sent solves a problem seen in our system.
> This patch does not solve a problem I have seen but I still think
> it should be there. Or at least some locking of the supply in regulator enable.
> What do you guys think?

If we're locking the regulator when it's directly enabled we need to do
the same when enabling it via other ways.