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

Groups > linux.kernel > #1373803 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-08 00:00 +0200
    Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Mark Brown <broonie@kernel.org> - 2016-04-12 06:50 +0200
      Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-12 10:40 +0200
        Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Mark Brown <broonie@kernel.org> - 2016-04-12 12:20 +0200
          Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-12 12:40 +0200

#1373803 — Re: [PATCH v5 30/46] regulator: pwm: retrieve correct voltage

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

[toc] | [next] | [standalone]

#1376444

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

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.

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

#1376562

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

#1376621

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

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.

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

#1376671

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.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web