[PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[Javier Martinez Canillas]

Commit 6261b06de565 ("regulator: Defer lookup of supply to regulator_get")
moved the regulator supplies lookup logic from the regulators registration
to the regulators get time.

Unfortunately, that changed the behavior of the regulator core since now a
parent supply with a child regulator marked as always-on, won't be enabled
unless a client driver attempts to get the child regulator during boot.

This patch makes the unresolved parent supplies to be looked up before the
regulators late cleanup, so those with a child marked as always on will be
enabled regardless if a driver attempted to get the child regulator or not.

That was the behavior before the mentioned commit, since parent supplies
were looked up at regulator registration time instead of during child get.

Cc: <stable@vger.kernel.org> # 4.3+
Fixes: 6261b06de565 ("regulator: Defer lookup of supply to regulator_get")
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Hello,

The commit that caused this issue landed into v4.1 but $SUBJECT can't be
cherry-picked to older kernel versions than v4.3 without causing a merge
conflict. So I added v4.3+ to stable, please let me know if isn't right.

Best regards,
Javier

 drivers/regulator/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6dd63523bcfe..15dbb771e1d8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4376,6 +4376,11 @@ static int __init regulator_init(void)
 /* init early to allow our consumers to complete system booting */
 core_initcall(regulator_init);
 
+static int __init regulator_late_resolve_supply(struct device *dev, void *data)
+{
+	return regulator_resolve_supply(dev_to_rdev(dev));
+}
+
 static int __init regulator_late_cleanup(struct device *dev, void *data)
 {
 	struct regulator_dev *rdev = dev_to_rdev(dev);
@@ -4436,6 +4441,14 @@ static int __init regulator_init_complete(void)
 	if (of_have_populated_dt())
 		has_full_constraints = true;
 
+	/* At this point there may be regulators that were not looked
+	 * up by a client driver, so its parent supply was not resolved
+	 * and could be wrongly disabled when needed to remain enabled
+	 * to meet their child constraints.
+	 */
+	class_for_each_device(&regulator_class, NULL, NULL,
+			      regulator_late_resolve_supply);
+
 	/* If we have a full configuration then disable any regulators
 	 * we have permission to change the status for and which are
 	 * not in use or always_on.  This is effectively the default
-- 
2.5.0

Re: [PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[Mark Brown]

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

On Sun, Mar 20, 2016 at 11:39:46PM -0300, Javier Martinez Canillas wrote:

> Unfortunately, that changed the behavior of the regulator core since now a
> parent supply with a child regulator marked as always-on, won't be enabled
> unless a client driver attempts to get the child regulator during boot.

> This patch makes the unresolved parent supplies to be looked up before the
> regulators late cleanup, so those with a child marked as always on will be
> enabled regardless if a driver attempted to get the child regulator or not.

This doesn't make much sense to me as a fix - it feels like we're doing
a fragile hack.  Surely it's better to do this as we register the
devices, that way we're also protected against any similar issues with
this that might occur after late probe if things are built modular?  Or
is there a strong reason for doing this only at late_initcall?

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

Re: [PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[Javier Martinez Canillas]

[adding Bjorn Andersson who is the author of commit 6261b06de565]

Hello Mark,

Thanks a lot for your feedback.

On 03/21/2016 08:11 AM, Mark Brown wrote:
> On Sun, Mar 20, 2016 at 11:39:46PM -0300, Javier Martinez Canillas wrote:
> 
>> Unfortunately, that changed the behavior of the regulator core since now a
>> parent supply with a child regulator marked as always-on, won't be enabled
>> unless a client driver attempts to get the child regulator during boot.
> 
>> This patch makes the unresolved parent supplies to be looked up before the
>> regulators late cleanup, so those with a child marked as always on will be
>> enabled regardless if a driver attempted to get the child regulator or not.
> 
> This doesn't make much sense to me as a fix - it feels like we're doing
> a fragile hack.  Surely it's better to do this as we register the
> devices, that way we're also protected against any similar issues with

Sorry, not sure if I understood correctly. You mean to do it when the
drivers register the regulators, so at regulator_register() ?

That's basically what was done before Bjorn's patch but that doesn't
handle the case of out of order registration when having circular
dependencies between regulators.

> this that might occur after late probe if things are built modular?  Or

Someone told me once that modules are always a special case :)

> is there a strong reason for doing this only at late_initcall?
> 

The reason why I did in late_initcall / regulator_init_complete is that
the problem for me is that unused regulators are disabled on cleanup but
parents whose childrens are marked as always on should be keep enabled.

But these are disabled anyways just because the regulator core didn't know
about that dependency. So doing it before the late cleanup sounded like a
good solution for me.

Now if you think that's a hack and have another approach in mind, then I'll
gladly try to implement it instead, if you could please elaborate on that.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[Javier Martinez Canillas]

On 03/21/2016 09:13 AM, Javier Martinez Canillas wrote:
> [adding Bjorn Andersson who is the author of commit 6261b06de565]
>

I got a user invalid error when sending to the email address that Bjorn used
in that commit, so I'm re-sending to his latest address. Sorry for the spam.

Bjorn, here is the email archive in case you need more context:

https://lkml.org/lkml/2016/3/20/277
 
> Hello Mark,
> 
> Thanks a lot for your feedback.
> 
> On 03/21/2016 08:11 AM, Mark Brown wrote:
>> On Sun, Mar 20, 2016 at 11:39:46PM -0300, Javier Martinez Canillas wrote:
>>
>>> Unfortunately, that changed the behavior of the regulator core since now a
>>> parent supply with a child regulator marked as always-on, won't be enabled
>>> unless a client driver attempts to get the child regulator during boot.
>>
>>> This patch makes the unresolved parent supplies to be looked up before the
>>> regulators late cleanup, so those with a child marked as always on will be
>>> enabled regardless if a driver attempted to get the child regulator or not.
>>
>> This doesn't make much sense to me as a fix - it feels like we're doing
>> a fragile hack.  Surely it's better to do this as we register the
>> devices, that way we're also protected against any similar issues with
> 
> Sorry, not sure if I understood correctly. You mean to do it when the
> drivers register the regulators, so at regulator_register() ?
> 
> That's basically what was done before Bjorn's patch but that doesn't
> handle the case of out of order registration when having circular
> dependencies between regulators.
> 
>> this that might occur after late probe if things are built modular?  Or
> 
> Someone told me once that modules are always a special case :)
> 
>> is there a strong reason for doing this only at late_initcall?
>>
> 
> The reason why I did in late_initcall / regulator_init_complete is that
> the problem for me is that unused regulators are disabled on cleanup but
> parents whose childrens are marked as always on should be keep enabled.
> 
> But these are disabled anyways just because the regulator core didn't know
> about that dependency. So doing it before the late cleanup sounded like a
> good solution for me.
> 
> Now if you think that's a hack and have another approach in mind, then I'll
> gladly try to implement it instead, if you could please elaborate on that.
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[Mark Brown]

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

On Mon, Mar 21, 2016 at 09:13:55AM -0300, Javier Martinez Canillas wrote:
> On 03/21/2016 08:11 AM, Mark Brown wrote:
> > On Sun, Mar 20, 2016 at 11:39:46PM -0300, Javier Martinez Canillas wrote:

> >> This patch makes the unresolved parent supplies to be looked up before the
> >> regulators late cleanup, so those with a child marked as always on will be
> >> enabled regardless if a driver attempted to get the child regulator or not.

> > This doesn't make much sense to me as a fix - it feels like we're doing
> > a fragile hack.  Surely it's better to do this as we register the
> > devices, that way we're also protected against any similar issues with

> Sorry, not sure if I understood correctly. You mean to do it when the
> drivers register the regulators, so at regulator_register() ?

> That's basically what was done before Bjorn's patch but that doesn't
> handle the case of out of order registration when having circular
> dependencies between regulators.

We used to look for the parent at registration time, we didn't look for
the children.  What you're trying to do here is look for the children;
we can do that at registration time.

> > this that might occur after late probe if things are built modular?  Or

> Someone told me once that modules are always a special case :)

That doesn't mean we should actively go out of our way to break them.

> The reason why I did in late_initcall / regulator_init_complete is that
> the problem for me is that unused regulators are disabled on cleanup but
> parents whose childrens are marked as always on should be keep enabled.

> But these are disabled anyways just because the regulator core didn't know
> about that dependency. So doing it before the late cleanup sounded like a
> good solution for me.

Regulators also get disabled if some consumer disables them, that can
happen separately to late_initcall() and often happens during consumer
device probe.

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

Re: [PATCH] regulator: Lookup unresolved parent supplies before regulators cleanup

[Javier Martinez Canillas]

Hello Mark,

On 03/21/2016 09:37 AM, Mark Brown wrote:
> On Mon, Mar 21, 2016 at 09:13:55AM -0300, Javier Martinez Canillas wrote:
>> On 03/21/2016 08:11 AM, Mark Brown wrote:
>>> On Sun, Mar 20, 2016 at 11:39:46PM -0300, Javier Martinez Canillas wrote:
> 
>>>> This patch makes the unresolved parent supplies to be looked up before the
>>>> regulators late cleanup, so those with a child marked as always on will be
>>>> enabled regardless if a driver attempted to get the child regulator or not.
> 
>>> This doesn't make much sense to me as a fix - it feels like we're doing
>>> a fragile hack.  Surely it's better to do this as we register the
>>> devices, that way we're also protected against any similar issues with
> 
>> Sorry, not sure if I understood correctly. You mean to do it when the
>> drivers register the regulators, so at regulator_register() ?
> 
>> That's basically what was done before Bjorn's patch but that doesn't
>> handle the case of out of order registration when having circular
>> dependencies between regulators.
> 
> We used to look for the parent at registration time, we didn't look for
> the children.  What you're trying to do here is look for the children;
> we can do that at registration time.
>

Oh, now I understand what you meant. I thought you said to lookup the
parents at registration time, not the childrens. Great, I'll take a
look and give a try to your suggestion.

Thanks a lot for your help.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America