Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

From: Dmitry Osipenko <digetx@gmail.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Peter Geis <pgwipeout@gmail.com>,
	Nicolas Chauvet <kwizart@gmail.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Kevin Hilman <khilman@kernel.org>,
	Peter De Schrijver <pdeschrijver@nvidia.com>,
	Viresh Kumar <vireshk@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>, Nishanth Menon <nm@ti.com>,
	Yangtao Li <tiny.windzz@gmail.com>,
	Matt Merhar <mattmerhar@protonmail.com>,
	linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-pm@vger.kernel.org
Subject: Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
Date: Wed, 20 Jan 2021 01:42:21 +0300	[thread overview]
Message-ID: <436f873a-2f79-51f0-31f3-b1f38b406004@gmail.com> (raw)
In-Reply-To: <20210119045827.2645gk6vabubehuh@vireshk-i7>

19.01.2021 07:58, Viresh Kumar пишет:
> On 18-01-21, 21:35, Dmitry Osipenko wrote:
>> 18.01.2021 11:20, Viresh Kumar пишет:
>>>> +int dev_pm_opp_sync_regulators(struct device *dev)
>>>> +{
>>>> +	struct opp_table *opp_table;
>>>> +	struct regulator *reg;
>>>> +	int i, ret = 0;
>>>> +
>>>> +	/* Device may not have OPP table */
>>>> +	opp_table = _find_opp_table(dev);
>>>> +	if (IS_ERR(opp_table))
>>>> +		return 0;
>>>> +
>>>> +	/* Regulator may not be required for the device */
>>>> +	if (!opp_table->regulators)
>>>> +		goto put_table;
>>>> +
>>>> +	mutex_lock(&opp_table->lock);
>>> What exactly do you need this lock for ?
>>
>> It is needed for protecting simultaneous invocations of
>> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().
>>
>> The sync_regulators() should be invoked only after completion of the
>> set_voltage() in a case of Tegra power domain driver since potentially
>> both could be running in parallel. For example device driver may be
>> changing performance state in a work thread, while PM domain state is
>> syncing.
> 
> I think just checking the 'enabled' flag should be enough here (you may need a
> lock for it though, but the lock should cover only the area it is supposed to
> cover and nothing else.

I'll remove the locks from these OPP patches and move them to the PD
driver. It should be the best option right now since OPP API isn't
entirely thread-safe, making it thread-safe should be a separate topic.