linux-kernel - Re: [PATCH] regulator: core: Limit propagation of parent voltage count and list

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <f38f79fb-71df-7ce0-86b9-ff6d2d0d4470@osg.samsung.com>
Date:   Sat, 25 Mar 2017 02:05:47 -0300
From:   Javier Martinez Canillas <javier@....samsung.com>
To:     Brian Norris <briannorris@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>
Subject: Re: [PATCH] regulator: core: Limit propagation of parent voltage
 count and list

Hello Matthias,

On 03/24/2017 05:38 PM, Brian Norris wrote:
> On Fri, Mar 24, 2017 at 01:09:52PM -0700, Matthias Kaehlcke wrote:
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index 53d4fc70dbd0..121838e0125b 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -2487,6 +2487,10 @@ static int _regulator_list_voltage(struct regulator *regulator,
>>  		if (lock)
>>  			mutex_unlock(&rdev->mutex);
>>  	} else if (rdev->supply) {
>> +		// Limit propagation of parent values to switch regulators
> 
> The kernel doesn't use C99 comments. Oddly enough, this isn't actually

+1

> in the coding style doc (Documentation/process/coding-style.rst), nor is
> it caught by scripts/checkpatch.pl (even though it clearly has a 'C99
> comment' rule).
> 
>> +		if (ops->get_voltage || ops->get_voltage_sel)

It's valid to have a .get_voltage_sel callback without a .list_voltage?

At least it seems that _regulator_get_voltage() assumes that having a
.get_voltage_sel implies that a .list_voltage will also be available.

static int _regulator_get_voltage(struct regulator_dev *rdev)
{
...
	if (rdev->desc->ops->get_voltage_sel) {
		sel = rdev->desc->ops->get_voltage_sel(rdev);
		if (sel < 0)
			return sel;
		ret = rdev->desc->ops->list_voltage(rdev, sel);
	} else if (rdev->desc->ops->get_voltage) {
...
}

So I would only check for if (ops->get_voltage).

>> +			return -EINVAL;
>> +
>>  		ret = _regulator_list_voltage(rdev->supply, selector, lock);
>>  	} else {
>>  		return -EINVAL;
>> @@ -2540,6 +2544,7 @@ EXPORT_SYMBOL_GPL(regulator_is_enabled);
>>  int regulator_count_voltages(struct regulator *regulator)
>>  {
>>  	struct regulator_dev	*rdev = regulator->rdev;
>> +	const struct regulator_ops *ops = rdev->desc->ops;
>>  
>>  	if (rdev->desc->n_voltages)
>>  		return rdev->desc->n_voltages;
>> @@ -2547,6 +2552,10 @@ int regulator_count_voltages(struct regulator *regulator)
>>  	if (!rdev->supply)
>>  		return -EINVAL;
>>  
>> +	// Limit propagation of parent value to switch regulators
> 
> Same here.
> 
>> +	if (ops->get_voltage || ops->get_voltage_sel || ops->list_voltage)
>> +		return -EINVAL;
>> +

I wonder if instead of always checking if the regulator lacks operations,
it wouldn't be better to do it just once and store that the regulator is
a switch so that state can be used as explicit check for switch instead.

Something like if (!rdev->supply || !rdev->switch) looks more clear to me.

>>  	return regulator_count_voltages(rdev->supply);
>>  }
>>  EXPORT_SYMBOL_GPL(regulator_count_voltages);
> 
> I'm not very familiar with this code, but judging by your problem
> description in previous threads and by comparing with the logic in
> _regulator_get_voltage() (for when to reference the ->supply), this
> seems resonable. So:
> 
> Reviewed-by: Brian Norris <briannorris@...omium.org>
>

Agreed, the logic sounds reasonable indeed and I didn't think of this
case when writing the mentioned commit, so feel free to add:

Reviewed-by: Javier Martinez Canillas <javier@....samsung.com>

> It's probably worth verifying that this doesn't break whatever Javier
> was supporting in the first place, as a sanity check.
>

I've tested in the system that led to the mentioned commit and I did
not find any issue with $SUBJECT.

Tested-by: Javier Martinez Canillas <javier@....samsung.com>

> Brian
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America