Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 31-05-22, 09:45, Viresh Kumar wrote:
> On 31-05-22, 13:12, Chanwoo Choi wrote:
> > I try to find the cause of this warning.
> > I think that dev_pm_opp_clear_config needs to check
> > whether 'opp_table' is NULL or not as following:
> > 
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index fba6e2b20b8f..cbf8f10b9ff0 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
> >   */
> >  void dev_pm_opp_clear_config(struct opp_table *opp_table)
> >  {
> > +       if (unlikely(!opp_table))
> > +               return;
> > +
> >         if (opp_table->genpd_virt_devs)
> >                 dev_pm_opp_detach_genpd(opp_table);
> 
> Does this fixes it for you ?
> 
> It isn't allowed to call this routine with opp_table as NULL, I should
> rather have a WARN() for the same instead.
> 
> Can you check why exynos is passing NULL here as I don't see an
> obvious reason currently.

Looking at the exynos devfreq driver again, it feels like the design
of the driver itself is causing all these issues.

Ideally, whatever resources are acquired by probe() must be freed only
and only by remove()/shutdown(). But you are trying to do it from
exynos_bus_exit() as well. Calling dev_pm_opp_of_remove_table() as
well from this function is wrong as you may very well end up
corrupting the OPP refcount and OPP may never get freed or something
else may come up.

For now I am adding following to the patch, please see if it fixes it
or not (I have pushed the changes to my branch as well).

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 780e525eb92a..8fca24565e7d 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -161,8 +161,11 @@ static void exynos_bus_exit(struct device *dev)

        dev_pm_opp_of_remove_table(dev);
        clk_disable_unprepare(bus->clk);
-       dev_pm_opp_clear_config(bus->opp_table);
-       bus->opp_table = NULL;
+
+       if (bus->opp_table) {
+               dev_pm_opp_clear_config(bus->opp_table);
+               bus->opp_table = NULL;
+       }
 }

 static void exynos_bus_passive_exit(struct device *dev)
@@ -463,8 +466,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
        dev_pm_opp_of_remove_table(dev);
        clk_disable_unprepare(bus->clk);
 err_reg:
-       dev_pm_opp_clear_config(bus->opp_table);
-       bus->opp_table = NULL;
+       if (bus->opp_table) {
+               dev_pm_opp_clear_config(bus->opp_table);
+               bus->opp_table = NULL;
+       }

        return ret;
 }

-- 
viresh