Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Viresh Kumar <viresh.kumar@linaro.org>]

On 05-11-20, 17:18, Dmitry Osipenko wrote:
> 05.11.2020 12:58, Viresh Kumar пишет:
> >> +static void sdhci_tegra_deinit_opp_table(void *data)
> >> +{
> >> +       struct device *dev = data;
> >> +       struct opp_table *opp_table;
> >> +
> >> +       opp_table = dev_pm_opp_get_opp_table(dev);
> > So you need to get an OPP table to put one :)
> > You need to save the pointer returned by dev_pm_opp_set_regulators() instead.
> 
> This is intentional because why do we need to save the pointer if we're
> not using it and we know that we could get this pointer using OPP API?

Because it is highly inefficient and it doesn't follow the rules set
by the OPP core. Hypothetically speaking, the OPP core is free to
allocate the OPP table structure as much as it wants, and if you don't
use the value returned back to you earlier (think of it as a cookie
assigned to your driver), then it will eventually lead to memory leak.

> This is exactly the same what I did for the CPUFreq driver [1] :)

I will strongly suggest you to save the pointer here and do the same
in the cpufreq driver as well.

> >> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
> >> +{
> >> +       struct opp_table *opp_table;
> >> +       const char *rname = "core";
> >> +       int err;
> >> +
> >> +       /* voltage scaling is optional */
> >> +       if (device_property_present(dev, "core-supply"))
> >> +               opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> >> +       else
> > 
> >> +               opp_table = dev_pm_opp_get_opp_table(dev);

To make it further clear, this will end up allocating an OPP table for
you, which it shouldn't have.

> > Nice. I didn't think that someone will end up abusing this API and so made it
> > available for all, but someone just did that. I will fix that in the OPP core.

To be fair, I allowed the cpufreq-dt driver to abuse it too, which I
am going to fix shortly.

> The dev_pm_opp_put_regulators() handles the case where regulator is
> missing by acting as dev_pm_opp_get_opp_table(), but the
> dev_pm_opp_set_regulators() doesn't do it. Hence I don't think this is
> an abuse, but the OPP API drawback.

I am not sure what you meant here. Normally you are required to call
dev_pm_opp_put_regulators() only if you have called
dev_pm_opp_set_regulators() earlier. And the refcount stays in
balance.

> > Any idea why you are doing what you are doing here ?
> 
> Two reasons:
> 
> 1. Voltage regulator is optional, but dev_pm_opp_set_regulators()
> doesn't support optional regulators.
> 
> 2. We need to balance the opp_table refcount in order to use OPP API
> without polluting code with if(have_regulator), hence the
> dev_pm_opp_get_opp_table() is needed for taking the opp_table reference
> to have the same refcount as in the case of the dev_pm_opp_set_regulators().

I am going to send a patchset shortly after which this call to
dev_pm_opp_get_opp_table() will fail, if it is called before adding
the OPP table.

> I guess we could make dev_pm_opp_set_regulators(dev, count) to accept
> regulators count=0 and then act as dev_pm_opp_get_opp_table(dev), will
> it be acceptable?

Setting regulators for count as 0 doesn't sound good to me.

But, I understand that you don't want to have that if (have_regulator)
check, and it is a fair request. What I will instead do is, allow all
dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
table and fail silently. And so you won't be required to have this
unwanted check. But you will be required to save the pointer returned
back by dev_pm_opp_set_regulators(), which is the right thing to do
anyways.

-- 
viresh