diff mbox

[v5,30/46] regulator: pwm: retrieve correct voltage

Message ID 1459368249-13241-31-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON March 30, 2016, 8:03 p.m. UTC
The continuous PWM voltage regulator is caching the voltage value in
the ->volt_uV field. While most of the time this value should reflect the
real voltage, sometime it can be sightly different if the PWM device
rounded the set_duty_cycle request.
Moreover, this value is not valid until someone has modified the regulator
output.

Remove the ->volt_uV field and always rely on the PWM state to calculate
the regulator output.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/regulator/pwm-regulator.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Comments

Mark Brown March 30, 2016, 9:24 p.m. UTC | #1
On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote:
> The continuous PWM voltage regulator is caching the voltage value in
> the ->volt_uV field. While most of the time this value should reflect the
> real voltage, sometime it can be sightly different if the PWM device
> rounded the set_duty_cycle request.
> Moreover, this value is not valid until someone has modified the regulator
> output.

Acked-by: Mark Brown <broonie@kernel.org>
Boris BREZILLON April 7, 2016, 9:54 p.m. UTC | #2
Hi Mark,

On Wed, 30 Mar 2016 14:24:10 -0700
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Mar 30, 2016 at 10:03:53PM +0200, Boris Brezillon wrote:
> > The continuous PWM voltage regulator is caching the voltage value in
> > the ->volt_uV field. While most of the time this value should reflect the
> > real voltage, sometime it can be sightly different if the PWM device
> > rounded the set_duty_cycle request.
> > Moreover, this value is not valid until someone has modified the regulator
> > output.
> 
> Acked-by: Mark Brown <broonie@kernel.org>

Actually this patch introduces a bug (reported by Stephen):

"
I applied your patch series [PATCH v5 00/46] pwm: add support for
atomic update and found a null pointer dereference when probing a
pwm-regulator at boot. See the below stack trace:

[ 4.282374] [<ffffffc000399088>] pwm_regulator_get_voltage+0x78/0xa0
[ 4.289344] [<ffffffc000390740>] regulator_attr_is_visible+0x7c/0x264
[ 4.296408] [<ffffffc0001f75a0>] internal_create_group+0x14c/0x280
[ 4.303184] [<ffffffc0001f76e8>] sysfs_create_group+0x14/0x1c
[ 4.309483] [<ffffffc0001f77c8>] sysfs_create_groups+0x30/0x78
[ 4.315881] [<ffffffc00043631c>] device_add+0x224/0x4d8
[ 4.321609] [<ffffffc0004365ec>] device_register+0x1c/0x28
[ 4.327623] [<ffffffc0003952a4>] regulator_register+0x2e4/0xc14
[ 4.334112] [<ffffffc000396844>] devm_regulator_register+0x54/0x94
[ 4.340887] [<ffffffc000399328>] pwm_regulator_probe+0x278/0x2b8
[ 4.347473] [<ffffffc000439fe4>] platform_drv_probe+0x58/0xa4
[ 4.353772] [<ffffffc0004387a8>] driver_probe_device+0x114/0x2ac
[ 4.360358] [<ffffffc0004389a4>] __driver_attach+0x64/0x90
[ 4.366371] [<ffffffc000436f50>] bus_for_each_dev+0x74/0x90
[ 4.372478] [<ffffffc000438bd4>] driver_attach+0x20/0x28
[ 4.378299] [<ffffffc00043778c>] bus_add_driver+0xe8/0x1e0
[ 4.384312] [<ffffffc00043959c>] driver_register+0x98/0xe4
[ 4.390326] [<ffffffc00043aa04>] __platform_driver_register+0x48/0x50
[ 4.397388] [<ffffffc000cb2710>] pwm_regulator_driver_init+0x18/0x20
[ 4.404356] [<ffffffc000c8ca7c>] do_one_initcall+0xf8/0x180
[ 4.410466] [<ffffffc000c8cc58>] kernel_init_freeable+0x154/0x1f4
[ 4.417148] [<ffffffc000929cf4>] kernel_init+0x10/0xf8
[ 4.422782] [<ffffffc000084450>] ret_from_fork+0x10/0x40

It looks like the root cause is that regulator_attr_is_visible will
try to get the voltage, but at this point in regulator_register,
rdev->constraints is still null. So
pwm_duty_cycle_percentage_to_voltage will dereference a null
rdev->constraints pointer.
"

The problem is that we need to know the min and max voltage constraints
to calculate the current voltage. ->get_voltage() is called when the
sysfs attributes are created (part of device registration), and
set_machine_constraints() is called after device_register(), thus
leading to the NULL pointer dereference.

Is there any reason for calling set_machine_constraints() after
device_register() in regulator_register()?

Best Regards,

Boris
Mark Brown April 12, 2016, 4:42 a.m. UTC | #3
On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:

> Is there any reason for calling set_machine_constraints() after
> device_register() in regulator_register()?

I'm not sure there's a strong one, we don't really use the class device
for anything, but without doing a full audit I couldn't guarantee that.
Boris BREZILLON April 12, 2016, 8:37 a.m. UTC | #4
Hi Mark,

On Tue, 12 Apr 2016 05:42:03 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:
> 
> > Is there any reason for calling set_machine_constraints() after
> > device_register() in regulator_register()?
> 
> I'm not sure there's a strong one, we don't really use the class device
> for anything, but without doing a full audit I couldn't guarantee that.

At first glance I don't see any problem (even the rdev_err/info/...()
functions do not use dev_err/info/...()). The patch will be part of v6
(unless you want me to send it independently).

Thanks,

Boris
Mark Brown April 12, 2016, 10:09 a.m. UTC | #5
On Tue, Apr 12, 2016 at 10:37:22AM +0200, Boris Brezillon wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:

> > I'm not sure there's a strong one, we don't really use the class device
> > for anything, but without doing a full audit I couldn't guarantee that.

> At first glance I don't see any problem (even the rdev_err/info/...()
> functions do not use dev_err/info/...()). The patch will be part of v6
> (unless you want me to send it independently).

I'd rather it didn't get sucked into this series since it seems that
it's getting delayed indefinitely - I can apply it to the regulator tree
and create a tag that can be pulled into other trees as needed if things
do get applied.
Boris BREZILLON April 12, 2016, 10:31 a.m. UTC | #6
On Tue, 12 Apr 2016 11:09:38 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Tue, Apr 12, 2016 at 10:37:22AM +0200, Boris Brezillon wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Apr 07, 2016 at 11:54:31PM +0200, Boris Brezillon wrote:
> 
> > > I'm not sure there's a strong one, we don't really use the class device
> > > for anything, but without doing a full audit I couldn't guarantee that.
> 
> > At first glance I don't see any problem (even the rdev_err/info/...()
> > functions do not use dev_err/info/...()). The patch will be part of v6
> > (unless you want me to send it independently).
> 
> I'd rather it didn't get sucked into this series since it seems that
> it's getting delayed indefinitely - I can apply it to the regulator tree
> and create a tag that can be pulled into other trees as needed if things
> do get applied.

Done.
diff mbox

Patch

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 9374796..77f42d8 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -35,9 +35,6 @@  struct pwm_regulator_data {
 	struct regulator_ops ops;
 
 	int state;
-
-	/* Continuous voltage */
-	int volt_uV;
 };
 
 struct pwm_voltages {
@@ -167,11 +164,27 @@  static int pwm_voltage_to_duty_cycle_percentage(struct regulator_dev *rdev, int
 	return ((req_uV * 100) - (min_uV * 100)) / diff;
 }
 
+static int pwm_duty_cycle_percentage_to_voltage(struct regulator_dev *rdev,
+						int dutycycle)
+{
+	int min_uV = rdev->constraints->min_uV;
+	int max_uV = rdev->constraints->max_uV;
+	int diff = max_uV - min_uV;
+
+	return min_uV + ((diff * dutycycle) / 100);
+}
+
 static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	struct pwm_state pstate;
+	u64 dutycycle;
 
-	return drvdata->volt_uV;
+	pwm_get_state(drvdata->pwm, &pstate);
+	dutycycle = pstate.duty_cycle * 100;
+	do_div(dutycycle, pstate.period);
+
+	return pwm_duty_cycle_percentage_to_voltage(rdev, dutycycle);
 }
 
 static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
@@ -196,8 +209,6 @@  static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 		return ret;
 	}
 
-	drvdata->volt_uV = min_uV;
-
 	/* Delay required by PWM regulator to settle to the new voltage */
 	usleep_range(ramp_delay, ramp_delay + 1000);