[v2,2/4] cpuidle: psci: Fixup support for domain idle states being zero

Message ID 20200303203559.23995-3-ulf.hansson@linaro.org
State New
Headers show
Series
  • cpuidle: psci: Some fixes when using the hierarchical layout
Related show

Commit Message

Ulf Hansson March 3, 2020, 8:35 p.m.
The current code intends to allow a PSCI PM domain to have none domain idle
states defined in DT. However, a few minor things needs to be fixed to make
this correctly supported, so let's do that.

Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/cpuidle/cpuidle-psci-domain.c | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.20.1

Comments

Sudeep Holla March 4, 2020, 10:50 a.m. | #1
On Tue, Mar 03, 2020 at 09:35:57PM +0100, Ulf Hansson wrote:
> The current code intends to allow a PSCI PM domain to have none domain idle
> states defined in DT. However, a few minor things needs to be fixed to make
> this correctly supported, so let's do that.
>
> Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 423f03bbeb74..c34b12c4069a 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -49,6 +49,9 @@ static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
>  	int i, ret;
>  	u32 psci_state, *psci_state_buf;
>
> +	if (!states)
> +		return 0;
> +

Was any issue found ? Or just code inspection ? If states = NULL,
state_count = 0, and I don't see anything blowing up. It may save couple
of extra instruction execution.

>  	for (i = 0; i < state_count; i++) {
>  		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
>  					&psci_state);
> @@ -96,6 +99,9 @@ static void psci_pd_free_states(struct genpd_power_state *states,
>  {
>  	int i;
>
> +	if (!states)
> +		return;
> +

Same here and kfree(NULL) is also valid.

--
Regards,
Sudeep
Ulf Hansson March 4, 2020, 12:17 p.m. | #2
On Wed, 4 Mar 2020 at 11:50, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Mar 03, 2020 at 09:35:57PM +0100, Ulf Hansson wrote:
> > The current code intends to allow a PSCI PM domain to have none domain idle
> > states defined in DT. However, a few minor things needs to be fixed to make
> > this correctly supported, so let's do that.
> >
> > Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> > Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/cpuidle/cpuidle-psci-domain.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index 423f03bbeb74..c34b12c4069a 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -49,6 +49,9 @@ static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
> >       int i, ret;
> >       u32 psci_state, *psci_state_buf;
> >
> > +     if (!states)
> > +             return 0;
> > +
>
> Was any issue found ? Or just code inspection ? If states = NULL,
> state_count = 0, and I don't see anything blowing up. It may save couple
> of extra instruction execution.

Code inspection, the real problem was fixed in patch 1.

>
> >       for (i = 0; i < state_count; i++) {
> >               ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
> >                                       &psci_state);
> > @@ -96,6 +99,9 @@ static void psci_pd_free_states(struct genpd_power_state *states,
> >  {
> >       int i;
> >
> > +     if (!states)
> > +             return;
> > +
>
> Same here and kfree(NULL) is also valid.

Yep, let's drop $subject patch from the series, it's not needed.

Thanks for reviewing!

Kind regards
Uffe

Patch

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 423f03bbeb74..c34b12c4069a 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -49,6 +49,9 @@  static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
 	int i, ret;
 	u32 psci_state, *psci_state_buf;
 
+	if (!states)
+		return 0;
+
 	for (i = 0; i < state_count; i++) {
 		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
 					&psci_state);
@@ -96,6 +99,9 @@  static void psci_pd_free_states(struct genpd_power_state *states,
 {
 	int i;
 
+	if (!states)
+		return;
+
 	for (i = 0; i < state_count; i++)
 		kfree(states[i].data);
 	kfree(states);