Messages in this thread | | | Date | Thu, 9 May 2024 14:35:05 +0200 | Subject | Re: [PATCH V2] OPP: Fix required_opp_tables for multiple genpds using same table | From | Thorsten Leemhuis <> |
| |
On 12.04.24 08:41, Viresh Kumar wrote: > The required_opp_tables parsing is not perfect, as the OPP core does the > parsing solely based on the DT node pointers. > > The core sets the required_opp_tables entry to the first OPP table in > the "opp_tables" list, that matches with the node pointer. > > If the target DT OPP table is used by multiple devices and they all > create separate instances of 'struct opp_table' from it, then it is > possible that the required_opp_tables entry may be set to the incorrect > sibling device. > > Unfortunately, there is no clear way to initialize the right values > during the initial parsing and we need to do this at a later point of > time. > > Cross check the OPP table again while the genpds are attached and fix > them if required. > > Also add a new API for the genpd core to fetch the device pointer for > the genpd. > > Cc: Thorsten Leemhuis <regressions@leemhuis.info> > Reported-by: Vladimir Lypak <vladimir.lypak@gmail.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218682
Did this fall through the cracks? Just wondering, as from here it looks like for about four weeks now nothing happened to fix the regression linked above. But I might have missed something. Or is everybody waiting for a test from the reporter?
Ciao, Thorsten
> Co-developed-by: Vladimir Lypak <vladimir.lypak@gmail.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V2: > - Fix an `if` condition. > - s/Bugzilla/Closes/ and change ordering. > > drivers/opp/core.c | 31 ++++++++++++++++++++++++++++++- > drivers/pmdomain/core.c | 10 ++++++++++ > include/linux/pm_domain.h | 6 ++++++ > 3 files changed, 46 insertions(+), 1 deletion(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e233734b7220..cb4611fe1b5b 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2394,7 +2394,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > const char * const *names, struct device ***virt_devs) > { > - struct device *virt_dev; > + struct device *virt_dev, *gdev; > + struct opp_table *genpd_table; > int index = 0, ret = -EINVAL; > const char * const *name = names; > > @@ -2427,6 +2428,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > goto err; > } > > + /* > + * The required_opp_tables parsing is not perfect, as the OPP > + * core does the parsing solely based on the DT node pointers. > + * The core sets the required_opp_tables entry to the first OPP > + * table in the "opp_tables" list, that matches with the node > + * pointer. > + * > + * If the target DT OPP table is used by multiple devices and > + * they all create separate instances of 'struct opp_table' from > + * it, then it is possible that the required_opp_tables entry > + * may be set to the incorrect sibling device. > + * > + * Cross check it again and fix if required. > + */ > + gdev = dev_to_genpd_dev(virt_dev); > + if (IS_ERR(gdev)) > + return PTR_ERR(gdev); > + > + genpd_table = _find_opp_table(gdev); > + if (!IS_ERR(genpd_table)) { > + if (genpd_table != opp_table->required_opp_tables[index]) { > + dev_pm_opp_put_opp_table(opp_table->required_opp_tables[index]); > + opp_table->required_opp_tables[index] = genpd_table; > + } else { > + dev_pm_opp_put_opp_table(genpd_table); > + } > + } > + > /* > * Add the virtual genpd device as a user of the OPP table, so > * we can call dev_pm_opp_set_opp() on it directly. > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > index 4215ffd9b11c..c40eda92a85a 100644 > --- a/drivers/pmdomain/core.c > +++ b/drivers/pmdomain/core.c > @@ -184,6 +184,16 @@ static struct generic_pm_domain *dev_to_genpd(struct device *dev) > return pd_to_genpd(dev->pm_domain); > } > > +struct device *dev_to_genpd_dev(struct device *dev) > +{ > + struct generic_pm_domain *genpd = dev_to_genpd(dev); > + > + if (IS_ERR(genpd)) > + return ERR_CAST(genpd); > + > + return &genpd->dev; > +} > + > static int genpd_stop_dev(const struct generic_pm_domain *genpd, > struct device *dev) > { > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 772d3280d35f..f24546a3d3db 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -260,6 +260,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, > int pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off); > int pm_genpd_remove(struct generic_pm_domain *genpd); > +struct device *dev_to_genpd_dev(struct device *dev); > int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); > int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); > int dev_pm_genpd_remove_notifier(struct device *dev); > @@ -307,6 +308,11 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd) > return -EOPNOTSUPP; > } > > +static inline struct device *dev_to_genpd_dev(struct device *dev) > +{ > + return ERR_PTR(-EOPNOTSUPP); > +} > + > static inline int dev_pm_genpd_set_performance_state(struct device *dev, > unsigned int state) > {
| |