linux-kernel - Re: [PATCH v4 4/4] regulator: Prevent falling too fast

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]

Date:   Mon, 12 Sep 2016 19:56:33 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     lgirdwood@...il.com, Douglas Anderson <dianders@...omium.org>,
        briannorris@...omium.org, javier@...hile0.org, robh+dt@...nel.org,
        mark.rutland@....com, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v4 4/4] regulator: Prevent falling too fast

On Tue, Sep 06, 2016 at 12:05:24PM -0700, Matthias Kaehlcke wrote:

> On some boards it's possible that transitioning the PWM regulator
> downwards too fast will trigger the over voltage protection (OVP) on the
> regulator.  This is because until the voltage actually falls there is a
> time when the requested voltage is much lower than the actual voltage.

So your PWM regulators are not very good and overshooting?  Do you have
any numbers on this - what values do you end up setting for both the
delay and safe fall percentages?

> We'll fix this OVP problem by allowing users to specify the maximum
> voltage that we can safely fall.  Apparently this maximum safe voltage
> should be specified as a percentage of the current voltage.  The
> regulator will then break things into separate steps with a delay in
> between.

I'd like to see some more thorough analysis than just an "apparently"
here.  It's making the code more fiddly for something very handwavy.

> @@ -2753,8 +2753,6 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>  	int old_selector = -1;
>  	int old_uV = _regulator_get_voltage(rdev);
>  
> -	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> -
>  	min_uV += rdev->constraints->uV_offset;
>  	max_uV += rdev->constraints->uV_offset;
>  
> @@ -2842,11 +2840,53 @@ no_delay:
>  				     (void *)data);
>  	}
>  
> -	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
> -
>  	return ret;

Let's remove some trace points too because...?  These *are* the actual
voltage changes in the device, we're just splitting things up into a
series of transitions.

> +static int _regulator_set_voltage(struct regulator_dev *rdev,
> +				     int min_uV, int max_uV)
> +{
> +	int safe_fall_percent = rdev->constraints->safe_fall_percent;
> +	int slowest_decay_rate = rdev->constraints->slowest_decay_rate;
> +	int orig_uV = _regulator_get_voltage(rdev);
> +	int uV = orig_uV;
> +	int ret;
> +
> +	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> +
> +	 /* If we're rising or we're falling but don't need to slow; easy */
> +	if (min_uV >= uV || !safe_fall_percent)

Indentation is broken, the two lines above don't agree with each other.

> +	ret = of_property_read_u32(np, "regulator-slowest-decay-rate", &pval);
> +	if (!ret)
> +		constraints->slowest_decay_rate = pval;
> +	else
> +		constraints->slowest_decay_rate = INT_MAX;

The documentation said this is mandatory if we have a safe fall rate
specified but the code says it's optional and we'll silently default to
an absurdly high rate instead (it's odd that a large number in a field
marked slowest is fast BTW).  Complaining loudly seems better than
ignoring the error.

> +	/* We use the value as int and as divider; sanity check */
> +	if (constraints->slowest_decay_rate == 0) {
> +		pr_err("%s: regulator-slowest-decay-rate must not be 0\n",
> +			np->name);
> +		constraints->slowest_decay_rate = INT_MAX;
> +	} else if (constraints->slowest_decay_rate > INT_MAX) {
> +		pr_err("%s: regulator-slowest-decay-rate (%u) too big\n",
> +			np->name, constraints->slowest_decay_rate);
> +		constraints->slowest_decay_rate = INT_MAX;
> +	}

This should be with the parsing of slowest-decay-rate and checked only
if we have a safe-fall-percent, there's no error if the value is omitted.

> +	if (constraints->safe_fall_percent > 100) {
> +		pr_err("%s: regulator-safe-fall-percent (%u) > 100\n",
> +			np->name, constraints->safe_fall_percent);
> +		constraints->safe_fall_percent = 0;
> +       }

Again indentation is borked here, I think you have tab/space issues.

> +	if (constraints->safe_fall_percent &&
> +		!constraints->slowest_decay_rate) {
> +		pr_err("%s: regulator-slowest-decay-rate requires "
> +			"regulator-safe-fall-percent\n", np->name);

Don't align the second line of the condition with the body of the if,
that just makes things hard to read - do what the rest of the code does
and align with the (.

Don't split text messages over multiple lines, it makes it impossible to
grep for the error as printed.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)