| Message ID | 1459368249-13241-31-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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
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.
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
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.
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 --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);
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(-)