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: <b7929693-fbb0-f5ce-da44-6c378cc25e0d@osg.samsung.com>
Date:   Mon, 27 Mar 2017 13:54:50 -0400
From:   Javier Martinez Canillas <javier@....samsung.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Brian Norris <briannorris@...omium.org>,
        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/27/2017 01:39 PM, Matthias Kaehlcke wrote:
> Thanks for the reviews and testing!
>

You are welcome.

[snip]

>>>> +		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) {
>> ...
>> }
> 
> The same function (from which I derived the conditions) suggests that
> a regulator could have a .list_voltage op even if it doesn't have
> .get_voltage_sel:
> 
>> ...
>> if (rdev->desc->ops->get_voltage_sel) {
>>   ...
>> } else if (rdev->desc->ops->get_voltage) {
>>   ...
>> } else if (rdev->desc->ops->list_voltage) {
> 
> I don't know for sure if this condition is superfluous or if there are
> cases where it makes sense to have a .list_voltage but not
> .get_voltage_sel.
>

I don't think is the same condition. Unless I'm misreading the code
what it's checking is if there's a .list_voltage even when there is
no .get_voltage_sel.

IOW, it's valid to have a .list_voltage even when there's no callback
for .get_voltage_sel, but the opposite isn't true.

>> 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.
> 
> I agree and we can even reduce it to if (!rdev_switch) since a switch
> implicitly has a supply.
>

I wonder if that's always true. What happens if you have a switch but
its <name>-supply parent isn't defined in the Device Tree?

> I'll send out a new version soon.
> 
> Matthias
> 

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