regulator: core: Fix resolve supply

[PATCH] regulator: core: Fix resolve supply

Posted by Kory Maincent 1 week, 2 days ago

The regulator should not use the device parent to resolve the regulator
supply. It fails to resolve the correct supply when the of_node
variable in the regulator_config structure is not within the parent
node.

Fixes: 6261b06de565 ("regulator: Defer lookup of supply to regulator_get")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

It is weird that it wasn't seen before, maybe there was not any case
were it can't find the supply_name from the parent device.
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 76d3cd5ae6ea..ee5bc070b5bb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2023,7 +2023,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 static int regulator_resolve_supply(struct regulator_dev *rdev)
 {
 	struct regulator_dev *r;
-	struct device *dev = rdev->dev.parent;
+	struct device *dev = &rdev->dev;
 	struct ww_acquire_ctx ww_ctx;
 	int ret = 0;
 
-- 
2.34.1

Re: [PATCH] regulator: core: Fix resolve supply

Posted by Mark Brown 1 week, 2 days ago

On Wed, Nov 13, 2024 at 04:36:14PM +0100, Kory Maincent wrote:

> The regulator should not use the device parent to resolve the regulator
> supply. It fails to resolve the correct supply when the of_node
> variable in the regulator_config structure is not within the parent
> node.

I can't understand what you are saying here at all.  What is "it", why
and in what way does it fail and why would we expect it to succeed?  How
does your proposed change fix whatever the issue is?  The DT binding is
against the actual device, not the virtual device.  Please describe both
the problem and the fix more specifically.

> Fixes: 6261b06de565 ("regulator: Defer lookup of supply to regulator_get")

>  static int regulator_resolve_supply(struct regulator_dev *rdev)
>  {
>  	struct regulator_dev *r;
> -	struct device *dev = rdev->dev.parent;
> +	struct device *dev = &rdev->dev;

The rdev is a virtual device, it's not going to have any OF
configuration, and given that prior to the refactoring in the commit
you're referencing in the Fixes: we were using the struct device passed
into regulator_register() which should be the same device as we're using
here so if there is an issue it doesn't look like it was introduced in
the refactoring.  What makes you believe that there is an issue in htat
commit?

Re: [PATCH] regulator: core: Fix resolve supply

Posted by Kory Maincent 1 week, 2 days ago

On Wed, 13 Nov 2024 16:05:46 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Nov 13, 2024 at 04:36:14PM +0100, Kory Maincent wrote:
> 
> > The regulator should not use the device parent to resolve the regulator
> > supply. It fails to resolve the correct supply when the of_node
> > variable in the regulator_config structure is not within the parent
> > node.  
> 
> I can't understand what you are saying here at all.  What is "it", why
> and in what way does it fail and why would we expect it to succeed?  How
> does your proposed change fix whatever the issue is?  The DT binding is
> against the actual device, not the virtual device.  Please describe both
> the problem and the fix more specifically.

Sorry, I was not explicit enough.
 
> > Fixes: 6261b06de565 ("regulator: Defer lookup of supply to regulator_get")  
> 
> >  static int regulator_resolve_supply(struct regulator_dev *rdev)
> >  {
> >  	struct regulator_dev *r;
> > -	struct device *dev = rdev->dev.parent;
> > +	struct device *dev = &rdev->dev;  
> 
> The rdev is a virtual device, it's not going to have any OF
> configuration

It does:
https://elixir.bootlin.com/linux/v6.11.7/source/drivers/regulator/core.c#L5687

My issue is that it does not look for the regulator supply in the OF node
described in the regulator_config structure:
https://elixir.bootlin.com/linux/v6.11.7/source/include/linux/regulator/driver.h#L445
It looks at the parent device OF node instead.

My use case is that a PSE controller have several PIs (Power Interface) which
can have different regulator supply. A regulator device is registered for each
PIs. The OF node used by the regulator core to lookup for the regulator supply
is the PSE controller node and its children instead of the node of the PI which
is described by the regulator_config->of_node.

> , and given that prior to the refactoring in the commit
> you're referencing in the Fixes: we were using the struct device passed
> into regulator_register() which should be the same device as we're using
> here so if there is an issue it doesn't look like it was introduced in
> the refactoring.  What makes you believe that there is an issue in htat
> commit?

Yes you are right I looked at it too quickly sorry.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] regulator: core: Fix resolve supply

Posted by Mark Brown 1 week, 1 day ago

On Wed, Nov 13, 2024 at 05:36:42PM +0100, Kory Maincent wrote:

> My issue is that it does not look for the regulator supply in the OF node
> described in the regulator_config structure:
> https://elixir.bootlin.com/linux/v6.11.7/source/include/linux/regulator/driver.h#L445
> It looks at the parent device OF node instead.
> 
> My use case is that a PSE controller have several PIs (Power Interface) which
> can have different regulator supply. A regulator device is registered for each
> PIs. The OF node used by the regulator core to lookup for the regulator supply
> is the PSE controller node and its children instead of the node of the PI which
> is described by the regulator_config->of_node.

Please resubmit with a clearer commit log.

Re: [PATCH] regulator: core: Fix resolve supply

Posted by Kory Maincent 1 week, 1 day ago

On Thu, 14 Nov 2024 13:27:55 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Nov 13, 2024 at 05:36:42PM +0100, Kory Maincent wrote:
> 
> > My issue is that it does not look for the regulator supply in the OF node
> > described in the regulator_config structure:
> > https://elixir.bootlin.com/linux/v6.11.7/source/include/linux/regulator/driver.h#L445
> > It looks at the parent device OF node instead.
> > 
> > My use case is that a PSE controller have several PIs (Power Interface)
> > which can have different regulator supply. A regulator device is registered
> > for each PIs. The OF node used by the regulator core to lookup for the
> > regulator supply is the PSE controller node and its children instead of the
> > node of the PI which is described by the regulator_config->of_node.  
> 
> Please resubmit with a clearer commit log.

Still I am not sure it is the right way to do it.
In regulator_dev_lookup function if the regulator is not found in the dt it uses
the dev_name string to look for it.
https://elixir.bootlin.com/linux/v6.11.7/source/drivers/regulator/core.c#L2035

The dev name of the rdev->dev.parent device or the rdev->dev virtual device is
not the same. I am not regulator expert therefore I am afraid that the suggested
change could break things.

Maybe this change is safer: https://termbin.com/g0xi

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] regulator: core: Fix resolve supply

Posted by Mark Brown 1 week, 1 day ago

On Thu, Nov 14, 2024 at 03:36:45PM +0100, Kory Maincent wrote:

> Maybe this change is safer: https://termbin.com/g0xi

Yes, I think so.