Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

From: Jon Hunter <jonathanh@nvidia.com>
To: Claudiu Beznea <claudiu.beznea@microchip.com>,
	<lgirdwood@gmail.com>, <broonie@kernel.org>
Cc: <s.hauer@pengutronix.de>, <ttynkkynen@nvidia.com>,
	<linus.walleij@linaro.org>, <axel.lin@ingics.com>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	linux-tegra <linux-tegra@vger.kernel.org>
Subject: Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel
Date: Tue, 24 Nov 2020 09:36:52 +0000	[thread overview]
Message-ID: <cb096dc5-3757-d72f-41a9-c99007c84b40@nvidia.com> (raw)
In-Reply-To: <1605280870-32432-2-git-send-email-claudiu.beznea@microchip.com>

On 13/11/2020 15:21, Claudiu Beznea wrote:
> There are regulators who's min selector is not zero. Selectors loops
> (looping b/w zero and regulator::desc::n_voltages) might throw errors
> because invalid selectors are used (lower than
> regulator::desc::linear_min_sel). For this situations validate selectors
> against regulator::desc::linear_min_sel.

After this commit was merged, I noticed a regression in the DFLL (CPU
clock source) on Tegra124. The DFLL driver
(drivers/clk/tegra/clk-dfll.c) calls regulator_list_voltage() in a loop
to determine the selector for a given voltage (see function
find_vdd_map_entry_exact()).

Currently, the DFLL driver queries the number of voltages provided by
the regulator by calling regulator_count_voltages() and then starting
from 0, iterates through the number of voltages to find the selector
value for the voltage it is looking for by calling
regulator_list_voltage(). It assumes that any negative value returned by
calling regulator_list_voltage() is an error and this will cause the
loop up to terminate.

In this case the regulator in question is the as3722 and the
linear_min_sel for this regulator is 1 and so when the DFLL driver calls
regulator_list_voltage() with a selector value of 0 it now returns a
negative error code, as expected by this change, and this terminates the
loop up in the DFLL driver. So I can clearly see why this is happening
and I could fix up the DFLL driver to avoid this.

Before doing so, I wanted to ask if that is the correct fix here,
because it seems a bit odd that regulator_count_voltages() returns N
voltages, but if the min selector value is greater than 0, then actually
there are less than N. However, changing the number of voltages
supported by the regulator to be N - linear_min_sel does not make sense
either because then we need to know the linear_min_sel in order to
determine the first valid voltage.

In case of the as3722, the value 0 means that the regulator is powered
down. So it is a valid setting and equates to 0 volts at the output AFAICT.

Please let me know your thoughts are the correct way to fix this up.

Thanks
Jon

-- 
nvpublic