[PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Ulf Hansson]

According to the common power domain DT bindings, a power domain provider
must have a "#power-domain-cells" property in its OF node. Additionally, if
a provider has a "power-domains" property, it means that it has a parent
domain.

It has turned out that some OF nodes that represents a genpd provider may
also be compatible with a regular platform device. This leads to, during
probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
to the genpd provider's parent domain, which is wrong. Instead the genpd
provider should only assign a parent domain, through
pm_genpd_add_subdomain() or of_genpd_add_subdomain().

Let's fix this problem in genpd by checking for the "#power-domain-cells"
property in the OF node for the struct device that is about to be attached.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

This turned up when I was reviewing a couple of genpd realted Qcom patches [1],
from Dmitry Baryshkov (on cc).

I haven't been able to run much tests, so help with that would be greatly
appreciated. If someone encounter issues, I am happy to help to fix those.

Kind regards
Ulf Hansson

[1]
https://lkml.org/lkml/2021/7/9/7
https://lkml.org/lkml/2021/7/9/8

---
 drivers/base/power/domain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a934c679e6ce..21991fb56a19 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2699,6 +2699,10 @@ int genpd_dev_pm_attach(struct device *dev)
 	if (!dev->of_node)
 		return 0;
 
+	/* Don't try to attach a genpd provider. */
+	if (of_find_property(dev->of_node, "#power-domain-cells", NULL))
+		return NULL;
+
 	/*
 	 * Devices with multiple PM domains must be attached separately, as we
 	 * can only attach one PM domain per device.
@@ -2737,6 +2741,10 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 	if (!dev->of_node)
 		return NULL;
 
+	/* Don't try to attach a genpd provider. */
+	if (of_find_property(dev->of_node, "#power-domain-cells", NULL))
+		return NULL;
+
 	/* Verify that the index is within a valid range. */
 	num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
 						 "#power-domain-cells");
-- 
2.25.1

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Geert Uytterhoeven]

Hi Ulf,

Thanks for your patch!

On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> According to the common power domain DT bindings, a power domain provider
> must have a "#power-domain-cells" property in its OF node. Additionally, if
> a provider has a "power-domains" property, it means that it has a parent
> domain.

OK.

> It has turned out that some OF nodes that represents a genpd provider may
> also be compatible with a regular platform device. This leads to, during
> probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> to the genpd provider's parent domain, which is wrong. Instead the genpd

Why is that wrong?

> provider should only assign a parent domain, through
> pm_genpd_add_subdomain() or of_genpd_add_subdomain().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Ulf Hansson]

On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> Thanks for your patch!
>
> On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > According to the common power domain DT bindings, a power domain provider
> > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > a provider has a "power-domains" property, it means that it has a parent
> > domain.
>
> OK.
>
> > It has turned out that some OF nodes that represents a genpd provider may
> > also be compatible with a regular platform device. This leads to, during
> > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > to the genpd provider's parent domain, which is wrong. Instead the genpd
>
> Why is that wrong?

It may lead to that the struct device that corresponds to a genpd
provider may be attached to the parent domain. In other words, the
parent domain will not only be controlled by a child domain
(corresponding to the provider), but also through the provider's
struct device. As far as I can tell, this has never been the intent
for how things should work in genpd.

So wrong or not, I guess it depends on what you expect to happen.

Do you see an issue with changing this?

>
> > provider should only assign a parent domain, through
> > pm_genpd_add_subdomain() or of_genpd_add_subdomain().

Kind regards
Uffe

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Jul 9, 2021 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > According to the common power domain DT bindings, a power domain provider
> > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > a provider has a "power-domains" property, it means that it has a parent
> > > domain.
> >
> > OK.
> >
> > > It has turned out that some OF nodes that represents a genpd provider may
> > > also be compatible with a regular platform device. This leads to, during
> > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> >
> > Why is that wrong?
>
> It may lead to that the struct device that corresponds to a genpd
> provider may be attached to the parent domain. In other words, the
> parent domain will not only be controlled by a child domain
> (corresponding to the provider), but also through the provider's
> struct device. As far as I can tell, this has never been the intent
> for how things should work in genpd.

Ah, you're worried about the case where the subdomain is a child of
the parent domain, but the actual subdomain controller (represented
by the platform device) isn't?

> So wrong or not, I guess it depends on what you expect to happen.
>
> Do you see an issue with changing this?

I don't have any hardware where the DTS describes a node as being
both a power domain consumer and a provider.
SH/R-Mobile uses a nested domain description in DT, but there are
no platform devices created for the nested child domains, only for
the top domain.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Ulf Hansson]

On Fri, 9 Jul 2021 at 15:35, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, Jul 9, 2021 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > According to the common power domain DT bindings, a power domain provider
> > > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > > a provider has a "power-domains" property, it means that it has a parent
> > > > domain.
> > >
> > > OK.
> > >
> > > > It has turned out that some OF nodes that represents a genpd provider may
> > > > also be compatible with a regular platform device. This leads to, during
> > > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> > >
> > > Why is that wrong?
> >
> > It may lead to that the struct device that corresponds to a genpd
> > provider may be attached to the parent domain. In other words, the
> > parent domain will not only be controlled by a child domain
> > (corresponding to the provider), but also through the provider's
> > struct device. As far as I can tell, this has never been the intent
> > for how things should work in genpd.
>
> Ah, you're worried about the case where the subdomain is a child of
> the parent domain, but the actual subdomain controller (represented
> by the platform device) isn't?

Well, even if the platform device represents a subdomain controller,
should it really be attached to the parent domain?

In any case, it means that the provider needs to manage runtime PM,
etc for its struct device to not prevent the parent domain from being
powered off.

Hmm, in the end this is just a matter of expectation of what will
happen during the attach. Perhaps it's just my worries that are wrong.
:-)

>
> > So wrong or not, I guess it depends on what you expect to happen.
> >
> > Do you see an issue with changing this?
>
> I don't have any hardware where the DTS describes a node as being
> both a power domain consumer and a provider.
> SH/R-Mobile uses a nested domain description in DT, but there are
> no platform devices created for the nested child domains, only for
> the top domain.

Okay, thanks for reviewing!

Kind regards
Uffe

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Jul 9, 2021 at 3:48 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Fri, 9 Jul 2021 at 15:35, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Jul 9, 2021 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > According to the common power domain DT bindings, a power domain provider
> > > > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > > > a provider has a "power-domains" property, it means that it has a parent
> > > > > domain.
> > > >
> > > > OK.
> > > >
> > > > > It has turned out that some OF nodes that represents a genpd provider may
> > > > > also be compatible with a regular platform device. This leads to, during
> > > > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> > > >
> > > > Why is that wrong?
> > >
> > > It may lead to that the struct device that corresponds to a genpd
> > > provider may be attached to the parent domain. In other words, the
> > > parent domain will not only be controlled by a child domain
> > > (corresponding to the provider), but also through the provider's
> > > struct device. As far as I can tell, this has never been the intent
> > > for how things should work in genpd.
> >
> > Ah, you're worried about the case where the subdomain is a child of
> > the parent domain, but the actual subdomain controller (represented
> > by the platform device) isn't?
>
> Well, even if the platform device represents a subdomain controller,
> should it really be attached to the parent domain?

That's what the presence of the "power-domains" property means,
isn't it?
If the subdomain controller itself is not part of the parent power
domain, there should not be a "power-domains" property.  So perhaps
we need a new property ("power-domain-parent"?) to indicate what is
the parent domain for the subdomains in this case?

> In any case, it means that the provider needs to manage runtime PM,
> etc for its struct device to not prevent the parent domain from being
> powered off.

Shouldn't all drivers for devices that can be somewhere in a PM Domain
hierarchy do that anyway? :-)  See e.g. commit 3a611e26e958b037
("net/smsc911x: Add minimal runtime PM support").

If "simple-bus" would do that, we could get rid of "simple-pm-bus"...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Ulf Hansson]

On Fri, 9 Jul 2021 at 15:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, Jul 9, 2021 at 3:48 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 9 Jul 2021 at 15:35, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Jul 9, 2021 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > According to the common power domain DT bindings, a power domain provider
> > > > > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > > > > a provider has a "power-domains" property, it means that it has a parent
> > > > > > domain.
> > > > >
> > > > > OK.
> > > > >
> > > > > > It has turned out that some OF nodes that represents a genpd provider may
> > > > > > also be compatible with a regular platform device. This leads to, during
> > > > > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > > > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > > > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> > > > >
> > > > > Why is that wrong?
> > > >
> > > > It may lead to that the struct device that corresponds to a genpd
> > > > provider may be attached to the parent domain. In other words, the
> > > > parent domain will not only be controlled by a child domain
> > > > (corresponding to the provider), but also through the provider's
> > > > struct device. As far as I can tell, this has never been the intent
> > > > for how things should work in genpd.
> > >
> > > Ah, you're worried about the case where the subdomain is a child of
> > > the parent domain, but the actual subdomain controller (represented
> > > by the platform device) isn't?
> >
> > Well, even if the platform device represents a subdomain controller,
> > should it really be attached to the parent domain?
>
> That's what the presence of the "power-domains" property means,
> isn't it?
> If the subdomain controller itself is not part of the parent power
> domain, there should not be a "power-domains" property.  So perhaps
> we need a new property ("power-domain-parent"?) to indicate what is
> the parent domain for the subdomains in this case?

Hmm, but perhaps it's just a matter of expectations of what will
happen during attach.

On the other hand a new binding for the parent would make it more
clear. I wouldn't mind.

>
> > In any case, it means that the provider needs to manage runtime PM,
> > etc for its struct device to not prevent the parent domain from being
> > powered off.
>
> Shouldn't all drivers for devices that can be somewhere in a PM Domain
> hierarchy do that anyway? :-)  See e.g. commit 3a611e26e958b037
> ("net/smsc911x: Add minimal runtime PM support").
>
> If "simple-bus" would do that, we could get rid of "simple-pm-bus"...

Right, I totally forgot about that. Thanks for reminding me.

As I said in the other thread, let's drop this patch for now.

Kind regards
Uffe

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Bjorn Andersson]

On Fri 09 Jul 08:22 CDT 2021, Ulf Hansson wrote:

> On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Ulf,
> >
> > Thanks for your patch!
> >
> > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > According to the common power domain DT bindings, a power domain provider
> > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > a provider has a "power-domains" property, it means that it has a parent
> > > domain.
> >
> > OK.
> >
> > > It has turned out that some OF nodes that represents a genpd provider may
> > > also be compatible with a regular platform device. This leads to, during
> > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> >
> > Why is that wrong?
> 
> It may lead to that the struct device that corresponds to a genpd
> provider may be attached to the parent domain. In other words, the
> parent domain will not only be controlled by a child domain
> (corresponding to the provider), but also through the provider's
> struct device. As far as I can tell, this has never been the intent
> for how things should work in genpd.
> 
> So wrong or not, I guess it depends on what you expect to happen.
> 
> Do you see an issue with changing this?
> 

But this exactly what we have in the case of the "dispcc" in the
Qualcomm platform that Dmitry is working on.

The provider driver needs the parent power-domain to be powered in order
to poke the registers and then it is the parent of the power-domains
exposed.

If I understand your proposed patch we'll have to manually attach the
parent domain to the struct device of the controller with this patch?

Is the Qualcomm case unique or will this change cut power do other genpd
providers assuming the same?



Worth mentioning as we discuss this is that we have another genpd
provider, where I think the exposed genpds are parented by a few
different (each one with a specific) parent domains. In this case we'd
be forced to manually attach the genpd provider to the parent domain
that it actually is powered by (as no automatic attachment happens when
multiple domains are specified).

Regards,
Bjorn

> >
> > > provider should only assign a parent domain, through
> > > pm_genpd_add_subdomain() or of_genpd_add_subdomain().
> 
> Kind regards
> Uffe

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Ulf Hansson]

On Fri, 9 Jul 2021 at 15:47, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Fri 09 Jul 08:22 CDT 2021, Ulf Hansson wrote:
>
> > On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >
> > > Hi Ulf,
> > >
> > > Thanks for your patch!
> > >
> > > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > According to the common power domain DT bindings, a power domain provider
> > > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > > a provider has a "power-domains" property, it means that it has a parent
> > > > domain.
> > >
> > > OK.
> > >
> > > > It has turned out that some OF nodes that represents a genpd provider may
> > > > also be compatible with a regular platform device. This leads to, during
> > > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> > >
> > > Why is that wrong?
> >
> > It may lead to that the struct device that corresponds to a genpd
> > provider may be attached to the parent domain. In other words, the
> > parent domain will not only be controlled by a child domain
> > (corresponding to the provider), but also through the provider's
> > struct device. As far as I can tell, this has never been the intent
> > for how things should work in genpd.
> >
> > So wrong or not, I guess it depends on what you expect to happen.
> >
> > Do you see an issue with changing this?
> >
>
> But this exactly what we have in the case of the "dispcc" in the
> Qualcomm platform that Dmitry is working on.
>
> The provider driver needs the parent power-domain to be powered in order
> to poke the registers and then it is the parent of the power-domains
> exposed.
>
> If I understand your proposed patch we'll have to manually attach the
> parent domain to the struct device of the controller with this patch?

Not even that would work after $subject patch, as it prevents
providers from being attached to a domain.

It sure sounds like you need to control power for the parent domain,
not only by registering a child domain to it.

>
> Is the Qualcomm case unique or will this change cut power do other genpd
> providers assuming the same?

I think the Qualcomm case is a bit unique or at least the first I
heard of. However, this change would affect all and of course we must
not break things.

>
>
>
> Worth mentioning as we discuss this is that we have another genpd
> provider, where I think the exposed genpds are parented by a few
> different (each one with a specific) parent domains. In this case we'd
> be forced to manually attach the genpd provider to the parent domain
> that it actually is powered by (as no automatic attachment happens when
> multiple domains are specified).

Yes, that's correct (assuming we don't apply $subject patch).

To sum up:

Rafael I am withdrawing the $subject patch, it seems like it may break
existing expectations of what will happen during attach.

Moreover, it may actually be beneficial to allow the attach to succeed
for the Qcom case, so let's leave this as is.

Kind regards
Uffe

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[Bjorn Andersson]

On Fri 09 Jul 08:58 CDT 2021, Ulf Hansson wrote:

> On Fri, 9 Jul 2021 at 15:47, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 09 Jul 08:22 CDT 2021, Ulf Hansson wrote:
> >
> > > On Fri, 9 Jul 2021 at 15:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > >
> > > > Hi Ulf,
> > > >
> > > > Thanks for your patch!
> > > >
> > > > On Fri, Jul 9, 2021 at 2:56 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > According to the common power domain DT bindings, a power domain provider
> > > > > must have a "#power-domain-cells" property in its OF node. Additionally, if
> > > > > a provider has a "power-domains" property, it means that it has a parent
> > > > > domain.
> > > >
> > > > OK.
> > > >
> > > > > It has turned out that some OF nodes that represents a genpd provider may
> > > > > also be compatible with a regular platform device. This leads to, during
> > > > > probe, genpd_dev_pm_attach(), genpd_dev_pm_attach_by_name() and
> > > > > genpd_dev_pm_attach_by_id() tries to attach the corresponding struct device
> > > > > to the genpd provider's parent domain, which is wrong. Instead the genpd
> > > >
> > > > Why is that wrong?
> > >
> > > It may lead to that the struct device that corresponds to a genpd
> > > provider may be attached to the parent domain. In other words, the
> > > parent domain will not only be controlled by a child domain
> > > (corresponding to the provider), but also through the provider's
> > > struct device. As far as I can tell, this has never been the intent
> > > for how things should work in genpd.
> > >
> > > So wrong or not, I guess it depends on what you expect to happen.
> > >
> > > Do you see an issue with changing this?
> > >
> >
> > But this exactly what we have in the case of the "dispcc" in the
> > Qualcomm platform that Dmitry is working on.
> >
> > The provider driver needs the parent power-domain to be powered in order
> > to poke the registers and then it is the parent of the power-domains
> > exposed.
> >
> > If I understand your proposed patch we'll have to manually attach the
> > parent domain to the struct device of the controller with this patch?
> 
> Not even that would work after $subject patch, as it prevents
> providers from being attached to a domain.
> 

That's definitely going to be a problem.

> It sure sounds like you need to control power for the parent domain,
> not only by registering a child domain to it.
> 

Yes, we certainly need power to the genpd provider.

> >
> > Is the Qualcomm case unique or will this change cut power do other genpd
> > providers assuming the same?
> 
> I think the Qualcomm case is a bit unique or at least the first I
> heard of. However, this change would affect all and of course we must
> not break things.
> 

I'm surprised that we'd be alone one that needs power to our genpd
provider. Does everyone else have their genpd providers in some
always-on power domain?

> >
> >
> >
> > Worth mentioning as we discuss this is that we have another genpd
> > provider, where I think the exposed genpds are parented by a few
> > different (each one with a specific) parent domains. In this case we'd
> > be forced to manually attach the genpd provider to the parent domain
> > that it actually is powered by (as no automatic attachment happens when
> > multiple domains are specified).
> 
> Yes, that's correct (assuming we don't apply $subject patch).
> 

Afaict this patch wouldn't change the case where the genpd provider has
multiple power-domains, as it wouldn't automatically attach the device
to any one of them anyways.

Regards,
Bjorn

> To sum up:
> 
> Rafael I am withdrawing the $subject patch, it seems like it may break
> existing expectations of what will happen during attach.
> 
> Moreover, it may actually be beneficial to allow the attach to succeed
> for the Qcom case, so let's leave this as is.
> 
> Kind regards
> Uffe

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[kernel test robot]

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

Hi Ulf,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/PM-domains-Don-t-attach-a-device-to-genpd-that-corresponds-to-a-provider/20210709-205736
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-s001-20210709 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/5f3ae7877b4b3a9c2c4d96c2106c41b16b981958
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ulf-Hansson/PM-domains-Don-t-attach-a-device-to-genpd-that-corresponds-to-a-provider/20210709-205736
        git checkout 5f3ae7877b4b3a9c2c4d96c2106c41b16b981958
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/base/power/domain.c:2704:24: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got void * @@
   drivers/base/power/domain.c:2704:24: sparse:     expected int
   drivers/base/power/domain.c:2704:24: sparse:     got void *

vim +2704 drivers/base/power/domain.c

  2683	
  2684	/**
  2685	 * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
  2686	 * @dev: Device to attach.
  2687	 *
  2688	 * Parse device's OF node to find a PM domain specifier. If such is found,
  2689	 * attaches the device to retrieved pm_domain ops.
  2690	 *
  2691	 * Returns 1 on successfully attached PM domain, 0 when the device don't need a
  2692	 * PM domain or when multiple power-domains exists for it, else a negative error
  2693	 * code. Note that if a power-domain exists for the device, but it cannot be
  2694	 * found or turned on, then return -EPROBE_DEFER to ensure that the device is
  2695	 * not probed and to re-try again later.
  2696	 */
  2697	int genpd_dev_pm_attach(struct device *dev)
  2698	{
  2699		if (!dev->of_node)
  2700			return 0;
  2701	
  2702		/* Don't try to attach a genpd provider. */
  2703		if (of_find_property(dev->of_node, "#power-domain-cells", NULL))
> 2704			return NULL;
  2705	
  2706		/*
  2707		 * Devices with multiple PM domains must be attached separately, as we
  2708		 * can only attach one PM domain per device.
  2709		 */
  2710		if (of_count_phandle_with_args(dev->of_node, "power-domains",
  2711					       "#power-domain-cells") != 1)
  2712			return 0;
  2713	
  2714		return __genpd_dev_pm_attach(dev, dev, 0, true);
  2715	}
  2716	EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
  2717	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48549 bytes --]

Re: [PATCH] PM: domains: Don't attach a device to genpd that corresponds to a provider

[kernel test robot]

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

Hi Ulf,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/PM-domains-Don-t-attach-a-device-to-genpd-that-corresponds-to-a-provider/20210709-205736
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: arm-randconfig-r005-20210709 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8d69635ed9ecf36fd0ca85906bfde17949671cbe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/5f3ae7877b4b3a9c2c4d96c2106c41b16b981958
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ulf-Hansson/PM-domains-Don-t-attach-a-device-to-genpd-that-corresponds-to-a-provider/20210709-205736
        git checkout 5f3ae7877b4b3a9c2c4d96c2106c41b16b981958
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/base/power/domain.c:2704:10: warning: incompatible pointer to integer conversion returning 'void *' from a function with result type 'int' [-Wint-conversion]
                   return NULL;
                          ^~~~
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
   #define NULL ((void *)0)
                ^~~~~~~~~~~
   1 warning generated.


vim +2704 drivers/base/power/domain.c

  2683	
  2684	/**
  2685	 * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
  2686	 * @dev: Device to attach.
  2687	 *
  2688	 * Parse device's OF node to find a PM domain specifier. If such is found,
  2689	 * attaches the device to retrieved pm_domain ops.
  2690	 *
  2691	 * Returns 1 on successfully attached PM domain, 0 when the device don't need a
  2692	 * PM domain or when multiple power-domains exists for it, else a negative error
  2693	 * code. Note that if a power-domain exists for the device, but it cannot be
  2694	 * found or turned on, then return -EPROBE_DEFER to ensure that the device is
  2695	 * not probed and to re-try again later.
  2696	 */
  2697	int genpd_dev_pm_attach(struct device *dev)
  2698	{
  2699		if (!dev->of_node)
  2700			return 0;
  2701	
  2702		/* Don't try to attach a genpd provider. */
  2703		if (of_find_property(dev->of_node, "#power-domain-cells", NULL))
> 2704			return NULL;
  2705	
  2706		/*
  2707		 * Devices with multiple PM domains must be attached separately, as we
  2708		 * can only attach one PM domain per device.
  2709		 */
  2710		if (of_count_phandle_with_args(dev->of_node, "power-domains",
  2711					       "#power-domain-cells") != 1)
  2712			return 0;
  2713	
  2714		return __genpd_dev_pm_attach(dev, dev, 0, true);
  2715	}
  2716	EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
  2717	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38046 bytes --]