PM: domains: Fix handling of unavailable/disabled idle states

Message ID 20221025123432.1227269-1-sudeep.holla@arm.com
State New
Headers
Series PM: domains: Fix handling of unavailable/disabled idle states |

Commit Message

Sudeep Holla Oct. 25, 2022, 12:34 p.m. UTC
  Platforms can provide the information about the availability of each
idle states via status flag. Platforms may have to disable one or more
idle states for various reasons like broken firmware or other unmet
dependencies.

Fix handling of such unavailable/disabled idle states by ignoring them
while parsing the states.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Fixes: a3381e3a65cb ("PM / domains: Fix up domain-idle-states OF parsing")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/base/power/domain.c | 4 ++++
 1 file changed, 4 insertions(+)

Hi Ulf,

As you already know, this change alone doesn't fix the issue reported here[1].
It also needs the fixes you have posted [2].

Regards,
Sudeep

[1] https://lore.kernel.org/all/20221018145348.4051809-1-amit.pundir@linaro.org
[2] https://lore.kernel.org/all/20221021151013.148457-1-ulf.hansson@linaro.org

--
2.38.1
  

Comments

Ulf Hansson Oct. 25, 2022, 12:58 p.m. UTC | #1
On Tue, 25 Oct 2022 at 14:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Platforms can provide the information about the availability of each
> idle states via status flag. Platforms may have to disable one or more
> idle states for various reasons like broken firmware or other unmet
> dependencies.
>
> Fix handling of such unavailable/disabled idle states by ignoring them
> while parsing the states.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Fixes: a3381e3a65cb ("PM / domains: Fix up domain-idle-states OF parsing")
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

I think this should be tagged for stable kernels too. Rafael, can you
pick this up as a fix for v6.1rc[n]?

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

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> Hi Ulf,
>
> As you already know, this change alone doesn't fix the issue reported here[1].
> It also needs the fixes you have posted [2].
>
> Regards,
> Sudeep
>
> [1] https://lore.kernel.org/all/20221018145348.4051809-1-amit.pundir@linaro.org
> [2] https://lore.kernel.org/all/20221021151013.148457-1-ulf.hansson@linaro.org

Well, I think it's better if we replace [1] with a patch that only
disables the cluster idle state. In that case, it would be sufficient
with $subject patch. In that case, we can just manage [2] separately.

Amit, can you submit a new version and test it together with $subject patch?

Kind regards
Uffe

>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ead135c7044c..6471b559230e 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2952,6 +2952,10 @@ static int genpd_iterate_idle_states(struct device_node *dn,
>                 np = it.node;
>                 if (!of_match_node(idle_state_match, np))
>                         continue;
> +
> +               if (!of_device_is_available(np))
> +                       continue;
> +
>                 if (states) {
>                         ret = genpd_parse_state(&states[i], np);
>                         if (ret) {
> --
> 2.38.1
>
  
Rafael J. Wysocki Oct. 26, 2022, 11:31 a.m. UTC | #2
On Tue, Oct 25, 2022 at 2:59 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 25 Oct 2022 at 14:34, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Platforms can provide the information about the availability of each
> > idle states via status flag. Platforms may have to disable one or more
> > idle states for various reasons like broken firmware or other unmet
> > dependencies.
> >
> > Fix handling of such unavailable/disabled idle states by ignoring them
> > while parsing the states.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Fixes: a3381e3a65cb ("PM / domains: Fix up domain-idle-states OF parsing")
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> I think this should be tagged for stable kernels too. Rafael, can you
> pick this up as a fix for v6.1rc[n]?
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Applied as 6.1-rc material, thanks!

>
> > ---
> >  drivers/base/power/domain.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > Hi Ulf,
> >
> > As you already know, this change alone doesn't fix the issue reported here[1].
> > It also needs the fixes you have posted [2].
> >
> > Regards,
> > Sudeep
> >
> > [1] https://lore.kernel.org/all/20221018145348.4051809-1-amit.pundir@linaro.org
> > [2] https://lore.kernel.org/all/20221021151013.148457-1-ulf.hansson@linaro.org
>
> Well, I think it's better if we replace [1] with a patch that only
> disables the cluster idle state. In that case, it would be sufficient
> with $subject patch. In that case, we can just manage [2] separately.
>
> Amit, can you submit a new version and test it together with $subject patch?
>
> Kind regards
> Uffe
>
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index ead135c7044c..6471b559230e 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -2952,6 +2952,10 @@ static int genpd_iterate_idle_states(struct device_node *dn,
> >                 np = it.node;
> >                 if (!of_match_node(idle_state_match, np))
> >                         continue;
> > +
> > +               if (!of_device_is_available(np))
> > +                       continue;
> > +
> >                 if (states) {
> >                         ret = genpd_parse_state(&states[i], np);
> >                         if (ret) {
> > --
> > 2.38.1
> >
  

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index ead135c7044c..6471b559230e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2952,6 +2952,10 @@  static int genpd_iterate_idle_states(struct device_node *dn,
 		np = it.node;
 		if (!of_match_node(idle_state_match, np))
 			continue;
+
+		if (!of_device_is_available(np))
+			continue;
+
 		if (states) {
 			ret = genpd_parse_state(&states[i], np);
 			if (ret) {