| Message ID | cover.1656935522.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | OPP: Add new configuration interface: dev_pm_opp_set_config() | expand |
On 04/07/2022 13:07, Viresh Kumar wrote: > Make dev_pm_opp_set_regulators() accept a NULL terminated list of names > instead of making the callers keep the two parameters in sync, which > creates an opportunity for bugs to get in. > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq-dt.c | 9 ++++----- > drivers/cpufreq/ti-cpufreq.c | 7 +++---- > drivers/devfreq/exynos-bus.c | 4 ++-- > drivers/gpu/drm/lima/lima_devfreq.c | 3 ++- > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 ++-- > drivers/opp/core.c | 18 ++++++++++++------ > drivers/soc/tegra/pmc.c | 4 ++-- > include/linux/pm_opp.h | 9 ++++----- > 8 files changed, 31 insertions(+), 27 deletions(-) > [...] > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > index 194af7f607a6..12784f349550 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c > @@ -91,6 +91,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) > struct devfreq *devfreq; > struct thermal_cooling_device *cooling; > struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq; > + const char *supplies[] = { pfdev->comp->supply_names[0], NULL }; > > if (pfdev->comp->num_supplies > 1) { > /* > @@ -101,8 +102,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) > return 0; > } > > - ret = devm_pm_opp_set_regulators(dev, pfdev->comp->supply_names, > - pfdev->comp->num_supplies); > + ret = devm_pm_opp_set_regulators(dev, supplies); > if (ret) { > /* Continue if the optional regulator is missing */ > if (ret != -ENODEV) { I have to say the 'new improved' list ending with NULL approach doesn't work out so well for Panfrost. We already have to have a separate 'num_supplies' variable for devm_regulator_bulk_get() / regulator_bulk_{en,dis}able(), so the keeping everything in sync argument is lost here. I would suggest added the NULL on the end of the lists in panfrost_drv.c but then it would break the use of ARRAY_SIZE() to automagically keep the length correct... For now the approach isn't too bad because Panfrost doesn't yet support enabling devfreq with more than one supply. But that array isn't going to work so nicely when that restriction is removed. The only sane way I can see of handling this in Panfrost would be replicating the loop to count the supplies in the Panfrost code which would allow dropping num_supplies from struct panfrost_compatible and then supply_names in the same struct could be NULL terminated ready for devm_pm_opp_set_regulators(). Steve > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e166bfe5fc90..4e4593957ec5 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2105,13 +2105,20 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name); > * This must be called before any OPPs are initialized for the device. > */ > struct opp_table *dev_pm_opp_set_regulators(struct device *dev, > - const char * const names[], > - unsigned int count) > + const char * const names[]) > { > struct dev_pm_opp_supply *supplies; > + const char * const *temp = names; > struct opp_table *opp_table; > struct regulator *reg; > - int ret, i; > + int count = 0, ret, i; > + > + /* Count number of regulators */ > + while (*temp++) > + count++; > + > + if (!count) > + return ERR_PTR(-EINVAL); > > opp_table = _add_opp_table(dev, false); > if (IS_ERR(opp_table)) > @@ -2236,12 +2243,11 @@ static void devm_pm_opp_regulators_release(void *data) > * Return: 0 on success and errorno otherwise. > */ > int devm_pm_opp_set_regulators(struct device *dev, > - const char * const names[], > - unsigned int count) > + const char * const names[]) > { > struct opp_table *opp_table; > > - opp_table = dev_pm_opp_set_regulators(dev, names, count); > + opp_table = dev_pm_opp_set_regulators(dev, names); > if (IS_ERR(opp_table)) > return PTR_ERR(opp_table); > > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c > index 5611d14d3ba2..6a4b8f7e7948 100644 > --- a/drivers/soc/tegra/pmc.c > +++ b/drivers/soc/tegra/pmc.c > @@ -1384,7 +1384,7 @@ tegra_pmc_core_pd_opp_to_performance_state(struct generic_pm_domain *genpd, > static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np) > { > struct generic_pm_domain *genpd; > - const char *rname = "core"; > + const char *rname[] = { "core", NULL}; > int err; > > genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL); > @@ -1395,7 +1395,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np) > genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state; > genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state; > > - err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1); > + err = devm_pm_opp_set_regulators(pmc->dev, rname); > if (err) > return dev_err_probe(pmc->dev, err, > "failed to set core OPP regulator\n"); > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index 6708b4ec244d..4c490865d574 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -159,9 +159,9 @@ void dev_pm_opp_put_supported_hw(struct opp_table *opp_table); > int devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count); > struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name); > void dev_pm_opp_put_prop_name(struct opp_table *opp_table); > -struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); > +struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[]); > void dev_pm_opp_put_regulators(struct opp_table *opp_table); > -int devm_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); > +int devm_pm_opp_set_regulators(struct device *dev, const char * const names[]); > struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name); > void dev_pm_opp_put_clkname(struct opp_table *opp_table); > int devm_pm_opp_set_clkname(struct device *dev, const char *name); > @@ -379,7 +379,7 @@ static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, con > > static inline void dev_pm_opp_put_prop_name(struct opp_table *opp_table) {} > > -static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) > +static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[]) > { > return ERR_PTR(-EOPNOTSUPP); > } > @@ -387,8 +387,7 @@ static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, co > static inline void dev_pm_opp_put_regulators(struct opp_table *opp_table) {} > > static inline int devm_pm_opp_set_regulators(struct device *dev, > - const char * const names[], > - unsigned int count) > + const char * const names[]) > { > return -EOPNOTSUPP; > }
On 04-07-22, 15:35, Steven Price wrote: > I have to say the 'new improved' list ending with NULL approach doesn't > work out so well for Panfrost. We already have to have a separate > 'num_supplies' variable for devm_regulator_bulk_get() / > regulator_bulk_{en,dis}able(), so the keeping everything in sync > argument is lost here. > > I would suggest added the NULL on the end of the lists in panfrost_drv.c > but then it would break the use of ARRAY_SIZE() to automagically keep > the length correct... Actually we can still make it work. > For now the approach isn't too bad because Panfrost doesn't yet support > enabling devfreq with more than one supply. But that array isn't going > to work so nicely when that restriction is removed. > > The only sane way I can see of handling this in Panfrost would be > replicating the loop to count the supplies in the Panfrost code which > would allow dropping num_supplies from struct panfrost_compatible and > then supply_names in the same struct could be NULL terminated ready for > devm_pm_opp_set_regulators(). Or doing this, which will simplify both the cases. diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 7fcbc2a5b6cd..b3b55565b8ef 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -625,24 +625,29 @@ static int panfrost_remove(struct platform_device *pdev) return 0; } -static const char * const default_supplies[] = { "mali" }; +/* + * The OPP core wants the supply names to be NULL terminated, but we need the + * correct num_supplies value for regulator core. Hence, we NULL terminate here + * and then initialize num_supplies with ARRAY_SIZE - 1. + */ +static const char * const default_supplies[] = { "mali", NULL }; static const struct panfrost_compatible default_data = { - .num_supplies = ARRAY_SIZE(default_supplies), + .num_supplies = ARRAY_SIZE(default_supplies) - 1, .supply_names = default_supplies, .num_pm_domains = 1, /* optional */ .pm_domain_names = NULL, }; static const struct panfrost_compatible amlogic_data = { - .num_supplies = ARRAY_SIZE(default_supplies), + .num_supplies = ARRAY_SIZE(default_supplies) - 1, .supply_names = default_supplies, .vendor_quirk = panfrost_gpu_amlogic_quirk, }; -static const char * const mediatek_mt8183_supplies[] = { "mali", "sram" }; +static const char * const mediatek_mt8183_supplies[] = { "mali", "sram", NULL }; static const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "core2" }; static const struct panfrost_compatible mediatek_mt8183_data = { - .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies), + .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies) - 1, .supply_names = mediatek_mt8183_supplies, .num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains), .pm_domain_names = mediatek_mt8183_pm_domains,
On 05/07/2022 05:34, Viresh Kumar wrote: > On 04-07-22, 15:35, Steven Price wrote: >> I have to say the 'new improved' list ending with NULL approach doesn't >> work out so well for Panfrost. We already have to have a separate >> 'num_supplies' variable for devm_regulator_bulk_get() / >> regulator_bulk_{en,dis}able(), so the keeping everything in sync >> argument is lost here. >> >> I would suggest added the NULL on the end of the lists in panfrost_drv.c >> but then it would break the use of ARRAY_SIZE() to automagically keep >> the length correct... > > Actually we can still make it work. > >> For now the approach isn't too bad because Panfrost doesn't yet support >> enabling devfreq with more than one supply. But that array isn't going >> to work so nicely when that restriction is removed. >> >> The only sane way I can see of handling this in Panfrost would be >> replicating the loop to count the supplies in the Panfrost code which >> would allow dropping num_supplies from struct panfrost_compatible and >> then supply_names in the same struct could be NULL terminated ready for >> devm_pm_opp_set_regulators(). > > Or doing this, which will simplify both the cases. Yes the below works, it's just a bit ugly having the "- 1", and potentially easy to forgot when adding another. However I don't suppose it would get far in that case so I think it would be spotted quickly when adding a new compatible. It's probably the best solution at the moment. Thanks, Steve > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 7fcbc2a5b6cd..b3b55565b8ef 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -625,24 +625,29 @@ static int panfrost_remove(struct platform_device *pdev) > return 0; > } > > -static const char * const default_supplies[] = { "mali" }; > +/* > + * The OPP core wants the supply names to be NULL terminated, but we need the > + * correct num_supplies value for regulator core. Hence, we NULL terminate here > + * and then initialize num_supplies with ARRAY_SIZE - 1. > + */ > +static const char * const default_supplies[] = { "mali", NULL }; > static const struct panfrost_compatible default_data = { > - .num_supplies = ARRAY_SIZE(default_supplies), > + .num_supplies = ARRAY_SIZE(default_supplies) - 1, > .supply_names = default_supplies, > .num_pm_domains = 1, /* optional */ > .pm_domain_names = NULL, > }; > > static const struct panfrost_compatible amlogic_data = { > - .num_supplies = ARRAY_SIZE(default_supplies), > + .num_supplies = ARRAY_SIZE(default_supplies) - 1, > .supply_names = default_supplies, > .vendor_quirk = panfrost_gpu_amlogic_quirk, > }; > > -static const char * const mediatek_mt8183_supplies[] = { "mali", "sram" }; > +static const char * const mediatek_mt8183_supplies[] = { "mali", "sram", NULL }; > static const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "core2" }; > static const struct panfrost_compatible mediatek_mt8183_data = { > - .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies), > + .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies) - 1, > .supply_names = mediatek_mt8183_supplies, > .num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains), > .pm_domain_names = mediatek_mt8183_pm_domains, >