Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1608947 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] regulator: core: Limit propagation of parent voltage count and list Matthias Kaehlcke <mka@chromium.org> - 2017-03-24 21:20 +0100
    Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Brian Norris <briannorris@chromium.org> - 2017-03-24 21:40 +0100
      Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Javier Martinez Canillas <javier@osg.samsung.com> - 2017-03-25 06:10 +0100
        Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Mark Brown <broonie@kernel.org> - 2017-03-27 12:30 +0200
        Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Matthias Kaehlcke <mka@chromium.org> - 2017-03-27 19:50 +0200
          Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Javier Martinez Canillas <javier@osg.samsung.com> - 2017-03-27 20:00 +0200
            Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Mark Brown <broonie@kernel.org> - 2017-03-27 20:20 +0200
            Re: [PATCH] regulator: core: Limit propagation of parent voltage  count and list Matthias Kaehlcke <mka@chromium.org> - 2017-03-27 20:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1609090 — 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@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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1610026 — 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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web