This patch add locks around regulator supply enable.
Signed-off-by: Mattias Wallin <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >> ---
> >> 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
> 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
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 <[email protected]>
Acked-by: Mark Brown <[email protected]>
> 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.
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