[PATCH] regulator: core: Limit propagation of parent voltage count and list

[PATCH] regulator: core: Limit propagation of parent voltage count and list

[Matthias Kaehlcke]

Commit 26988efe11b1 ("regulator: core: Allow to get voltage count and
list from parent") introduces the propagation of the parent voltage
count and list for regulators that don't provide this information
themselves. The goal is to support simple switch regulators, however as
a side effect normal continuous regulators can leak details of their
supplies and provide consumers with inconsistent information.

Limit the propagation of the voltage count and list to regulators which
don't have get_voltage(_sel) and list_voltage ops.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/regulator/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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
+		if (ops->get_voltage || ops->get_voltage_sel)
+			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
+	if (ops->get_voltage || ops->get_voltage_sel || ops->list_voltage)
+		return -EINVAL;
+
 	return regulator_count_voltages(rdev->supply);
 }
 EXPORT_SYMBOL_GPL(regulator_count_voltages);
-- 
2.12.1.578.ge9c3154ca4-goog

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

[Brian Norris]

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
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)
> +			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;
> +
>  	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@chromium.org>

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

Brian

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

[Javier Martinez Canillas]

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@chromium.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@osg.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@osg.samsung.com>

> Brian
> 

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

On Sat, Mar 25, 2017 at 02:05:47AM -0300, Javier Martinez Canillas wrote:
> On 03/24/2017 05:38 PM, Brian Norris wrote:

> > 
> >> +		if (ops->get_voltage || ops->get_voltage_sel)

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

No.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matthias Kaehlcke]

Thanks for the reviews and testing!

El Sat, Mar 25, 2017 at 02:05:47AM -0300 Javier Martinez Canillas ha dit:

 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

Will fix

> > 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) {
> ...
> }

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 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'll send out a new version soon.

Matthias

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

[Javier Martinez Canillas]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

On Mon, Mar 27, 2017 at 01:54:50PM -0400, Javier Martinez Canillas wrote:
> On 03/27/2017 01:39 PM, Matthias Kaehlcke wrote:

> > 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.

Yes.

> 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.

Yes.

> >> 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?

We may already be substituting in the dummy supply, I'd need to go check
but if we're not that's a fairly straightforward fix for the immediate
problem.  You do then have to worry about the fact that the parent
supply might not have voltage operations either, dummy supplies
obviously won't and there are use cases where an actual supply might not
either (things like unregulated wall supplies).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matthias Kaehlcke]

El Mon, Mar 27, 2017 at 01:54:50PM -0400 Javier Martinez Canillas ha dit:

> On 03/27/2017 01:39 PM, Matthias Kaehlcke wrote:
>
> >>>> +		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 see, thanks for the clarification.

> >> 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?

My idea was to only set rdev->switch after having resolved the
parent supply, though I concede this is not semantically. Maybe we
still want this logic but give the flag a different name?

Matthias