| Message ID | 20190227195836.24739-2-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) | expand |
On 27/02/2019 20:58, Ulf Hansson wrote: > Let's add a data pointer to the genpd_power_state struct, to allow a genpd > backend driver to store per state specific data. To introduce the pointer, > we need to change the way genpd deals with freeing of the corresponding > allocated data. > > More precisely, let's clarify the responsibility of whom that shall free > the data, by adding a ->free_states() callback to the struct > generic_pm_domain. The one allocating the data shall assign the callback, > to allow genpd to invoke it from genpd_remove(). > > Cc: Lina Iyer <ilina@codeaurora.org> > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > > Changes in v12: > - None. > > --- > drivers/base/power/domain.c | 12 ++++++++++-- > include/linux/pm_domain.h | 4 +++- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 2c334c01fc43..03885c003c6a 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1685,6 +1685,12 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, > } > EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain); > > +static void genpd_free_default_power_state(struct genpd_power_state *states, > + unsigned int state_count) > +{ > + kfree(states); > +} > + > static int genpd_set_default_power_state(struct generic_pm_domain *genpd) > { > struct genpd_power_state *state; > @@ -1695,7 +1701,7 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd) > > genpd->states = state; > genpd->state_count = 1; > - genpd->free = state; > + genpd->free_states = genpd_free_default_power_state; > > return 0; > } > @@ -1811,7 +1817,9 @@ static int genpd_remove(struct generic_pm_domain *genpd) > list_del(&genpd->gpd_list_node); > genpd_unlock(genpd); > cancel_work_sync(&genpd->power_off_work); > - kfree(genpd->free); > + if (genpd->free_states) Is this test necessary as the free_states function is initialized with the genpd_set_default_power_state() in any case? > + genpd->free_states(genpd->states, genpd->state_count); > + > pr_debug("%s: removed %s\n", __func__, genpd->name); > > return 0; > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 1ed5874bcee0..8e1399231753 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -69,6 +69,7 @@ struct genpd_power_state { > s64 residency_ns; > struct fwnode_handle *fwnode; > ktime_t idle_time; > + void *data; > }; > > struct genpd_lock_ops; > @@ -110,9 +111,10 @@ struct generic_pm_domain { > struct device *dev); > unsigned int flags; /* Bit field of configs for genpd */ > struct genpd_power_state *states; > + void (*free_states)(struct genpd_power_state *states, > + unsigned int state_count); > unsigned int state_count; /* number of states */ > unsigned int state_idx; /* state that genpd will go to when off */ > - void *free; /* Free the state that was allocated for default */ > ktime_t on_time; > ktime_t accounting_time; > const struct genpd_lock_ops *lock_ops; > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Thu, 21 Mar 2019 at 08:47, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 27/02/2019 20:58, Ulf Hansson wrote: > > Let's add a data pointer to the genpd_power_state struct, to allow a genpd > > backend driver to store per state specific data. To introduce the pointer, > > we need to change the way genpd deals with freeing of the corresponding > > allocated data. > > > > More precisely, let's clarify the responsibility of whom that shall free > > the data, by adding a ->free_states() callback to the struct > > generic_pm_domain. The one allocating the data shall assign the callback, > > to allow genpd to invoke it from genpd_remove(). > > > > Cc: Lina Iyer <ilina@codeaurora.org> > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > > > Changes in v12: > > - None. > > > > --- > > drivers/base/power/domain.c | 12 ++++++++++-- > > include/linux/pm_domain.h | 4 +++- > > 2 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 2c334c01fc43..03885c003c6a 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -1685,6 +1685,12 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, > > } > > EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain); > > > > +static void genpd_free_default_power_state(struct genpd_power_state *states, > > + unsigned int state_count) > > +{ > > + kfree(states); > > +} > > + > > static int genpd_set_default_power_state(struct generic_pm_domain *genpd) > > { > > struct genpd_power_state *state; > > @@ -1695,7 +1701,7 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd) > > > > genpd->states = state; > > genpd->state_count = 1; > > - genpd->free = state; > > + genpd->free_states = genpd_free_default_power_state; > > > > return 0; > > } > > @@ -1811,7 +1817,9 @@ static int genpd_remove(struct generic_pm_domain *genpd) > > list_del(&genpd->gpd_list_node); > > genpd_unlock(genpd); > > cancel_work_sync(&genpd->power_off_work); > > - kfree(genpd->free); > > + if (genpd->free_states) > > Is this test necessary as the free_states function is initialized with > the genpd_set_default_power_state() in any case? That's the case when the genpd provider did not allocate states, so then we know genpd deals with this properly for us. In the other case, when genpd provider has allocates states, then we need to check that the provider has assigned the ->free_states() callback before we invokes it, as there is no guarantees that it had. I was initially tempted to do this check already at pm_genpd_init(), as it would allow us to check for the error condition and return an error code if it's not been assigned. However, that requires me to change all providers that currently "allocates" their states, so that isn't really a smooth way forward. Perhaps, we should simply print a message to the log about this condition in pm_genpd_init(), as to start with!? I can add a patch on top doing that. > > > + genpd->free_states(genpd->states, genpd->state_count); > > + > > pr_debug("%s: removed %s\n", __func__, genpd->name); > > [...] Kind regards Uffe
On 21/03/2019 10:36, Ulf Hansson wrote: > On Thu, 21 Mar 2019 at 08:47, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: [ ... ] >>> cancel_work_sync(&genpd->power_off_work); >>> - kfree(genpd->free); >>> + if (genpd->free_states) >> >> Is this test necessary as the free_states function is initialized with >> the genpd_set_default_power_state() in any case? > > That's the case when the genpd provider did not allocate states, so > then we know genpd deals with this properly for us. > > In the other case, when genpd provider has allocates states, then we > need to check that the provider has assigned the ->free_states() > callback before we invokes it, as there is no guarantees that it had. > > I was initially tempted to do this check already at pm_genpd_init(), > as it would allow us to check for the error condition and return an > error code if it's not been assigned. However, that requires me to > change all providers that currently "allocates" their states, so that > isn't really a smooth way forward. Perhaps, we should simply print a > message to the log about this condition in pm_genpd_init(), as to > start with!? I can add a patch on top doing that. Yes, that would make sense to track those providers which do not initialize the free field. Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 2c334c01fc43..03885c003c6a 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1685,6 +1685,12 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, } EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain); +static void genpd_free_default_power_state(struct genpd_power_state *states, + unsigned int state_count) +{ + kfree(states); +} + static int genpd_set_default_power_state(struct generic_pm_domain *genpd) { struct genpd_power_state *state; @@ -1695,7 +1701,7 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd) genpd->states = state; genpd->state_count = 1; - genpd->free = state; + genpd->free_states = genpd_free_default_power_state; return 0; } @@ -1811,7 +1817,9 @@ static int genpd_remove(struct generic_pm_domain *genpd) list_del(&genpd->gpd_list_node); genpd_unlock(genpd); cancel_work_sync(&genpd->power_off_work); - kfree(genpd->free); + if (genpd->free_states) + genpd->free_states(genpd->states, genpd->state_count); + pr_debug("%s: removed %s\n", __func__, genpd->name); return 0; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 1ed5874bcee0..8e1399231753 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -69,6 +69,7 @@ struct genpd_power_state { s64 residency_ns; struct fwnode_handle *fwnode; ktime_t idle_time; + void *data; }; struct genpd_lock_ops; @@ -110,9 +111,10 @@ struct generic_pm_domain { struct device *dev); unsigned int flags; /* Bit field of configs for genpd */ struct genpd_power_state *states; + void (*free_states)(struct genpd_power_state *states, + unsigned int state_count); unsigned int state_count; /* number of states */ unsigned int state_idx; /* state that genpd will go to when off */ - void *free; /* Free the state that was allocated for default */ ktime_t on_time; ktime_t accounting_time; const struct genpd_lock_ops *lock_ops;
Let's add a data pointer to the genpd_power_state struct, to allow a genpd backend driver to store per state specific data. To introduce the pointer, we need to change the way genpd deals with freeing of the corresponding allocated data. More precisely, let's clarify the responsibility of whom that shall free the data, by adding a ->free_states() callback to the struct generic_pm_domain. The one allocating the data shall assign the callback, to allow genpd to invoke it from genpd_remove(). Cc: Lina Iyer <ilina@codeaurora.org> Co-developed-by: Lina Iyer <lina.iyer@linaro.org> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Changes in v12: - None. --- drivers/base/power/domain.c | 12 ++++++++++-- include/linux/pm_domain.h | 4 +++- 2 files changed, 13 insertions(+), 3 deletions(-) -- 2.17.1