linux-kernel - Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210901071655.gf6qg7piisddg2a3@vireshk-i7>
Date:   Wed, 1 Sep 2021 12:46:55 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH v10 5/8] soc/tegra: pmc: Implement
 dev_get_performance_state() callback

On 01-09-21, 09:57, Dmitry Osipenko wrote:
> 01.09.2021 09:10, Viresh Kumar пишет:
> > So you create the OPP table and just after that you remove it ? Just
> > to get the current OPP and pstate ? This doesn't look okay.
> > 
> > Moreover, this routine must be implemented with the expectation that
> > the genpd core can call it anytime it wants, not just at the
> > beginning. And so if the table is already setup by someone else then,
> > then this all will just fail.
> 
> This is not doable using the current OPP API, it doesn't allow to
> re-use initialized OPP table.

That isn't correct, you can call dev_pm_opp_of_add_table() as many
times as you want. It will just increase the refcount and return the
next time.

Yes, you can call the APIs like set-clk-name or supported-hw, since
they are supposed to be set only once.

> The current limitation is okay because genpd core doesn't call
> routine anytime.

Yeah, but is broken by design. People can make changes to genpd core
later on to call it as many times and they aren't required to have a
look at all the users to see who abused the API and who didn't.

> > Can you give the sequence in which the whole thing works out with
> > respect to the OPP core? who calls
> > devm_tegra_core_dev_init_opp_table() and when, and when exactly will
> > this routine get called, etc ?
> > 
> 
> gr3d_probe(struct platform_device *pdev)

Thanks for this.

> {
> 	gr3d_init_power(dev)
> 	{
> 		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
> 
> 		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
> 		{
> 			dev_pm_opp_attach_genpd(dev, names, virt_devs)
> 			{
> 				// takes and holds table reference
> 				opp_table = _add_opp_table(dev, false);
> 
> 				while (*name) {
> 					// first attachment succeeds, 
> 					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
> 					dev_pm_domain_attach_by_name(dev, *name)
> 					{
> 						tegra_pmc_pd_dev_get_performance_state(dev)
> 						{
> 							dev_pm_opp_set_clkname(dev, NULL);
> 							dev_pm_opp_of_add_table(dev);

What you end up doing here is pretty much like
devm_tegra_core_dev_init_opp_table_simple(), right ?

> 							opp = dev_pm_opp_get_current(dev);
> 							dev_pm_opp_of_remove_table(dev);
> 							dev_pm_opp_put_clkname(opp_table);

You shouldn't be required to do this at all.

> 							...
> 						}
> 						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
> 					}
> 				}
> 			}
> 		}
> 	}
> 
> 	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);

Can we make the call at the beginning ? before calling
gr3d_init_power() ? I mean you should only be required to initialize
the OPP table just once.

If not, then what about calling
devm_tegra_core_dev_init_opp_table_simple() from here and from
tegra_pmc_pd_dev_get_performance_state() as well ?

And update devm_tegra_core_dev_init_opp_table_simple() to call
dev_pm_opp_get_opp_table() first and return early if the OPP table is
already initialized ?

-- 
viresh