regulator: gpio-regulator: fix crash when no states property in dt

PATCH New Frank Li 4 participants 7 messages Nov 9, 2012 – Nov 14, 2012 Voltage and current regulator framework

Share & tools

Frank Li

author

2012-11-09 08:34 UTC #0 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

Unable to handle kernel NULL pointer dereference at virtual address 00000004
pgd = 80004000
[00000004] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0    Not tainted  (3.5.7+ #11)
PC is at of_get_gpio_regulator_config+0x1b0/0x2e0
LR is at of_find_property+0x4c/0x9c
pc : [<8022498c>]    lr : [<80322d44>]    psr: 60000013

sp : bf859de8 ip : bf859dc8 fp : bf859e14
r10: 805d1180 r9 : 8053e208 r8 : 00000001
r7 : 811056ec r6 : 00000000 r5 : bf8e0208 r4 : bf8f2b50 r3 : 805a7e00 r2 : 000000d0 r1 : 00000000 r0 : 00000000
Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c53c7d Table: 1000404a DAC: 00000017 Process swapper/0 (pid: 1, stack limit = 0xbf8582f0)

Stack: (0xbf859de8 to 0xbf85a000)

9de0: bf859e34 bf859df8 8010a040 00000000 bf8e0200 811056ec
9e00: 00000000 805b4d9c bf859e54 bf859e18 80404a14 802247e8 00000000 00000000 9e20: 00000000 00000000 00000000 00000000 00000000 00000000 8061f05c 805b4d9c 9e40: bf8e0208 00000000 bf859e64 bf859e58 8024bc0c 804049e0 bf859e8c bf859e68 9e60: 8024a7d8 8024bbf8 00000000 bf8e0208 805b4d9c bf8e023c 00000000 0000008d 9e80: bf859eac bf859e90 8024a9dc 8024a76c 8024a948 805b4d9c 8024a948 00000000 9ea0: bf859ed4 bf859eb0 80248f10 8024a954 bf83b458 bf8dd934 801f13e0 805b4d9c 9ec0: 805b8af8 bf8e9f00 bf859ee4 bf859ed8 8024a358 80248ec4 bf859f14 bf859ee8 9ee0: 80249f94 8024a344 804eb244 805d1180 805b4d9c 00000004 00000000 805d1180 9f00: 0000008d 805d1180 bf859f3c bf859f18 8024aefc 80249e1c 00000000 bf858000 9f20: 00000004 00000000 805d1180 0000008d bf859f4c bf859f40 8024bedc 8024ae88 9f40: bf859f5c bf859f50 805600b4 8024be9c bf859fb4 bf859f60 800086e4 805600ac 9f60: bf859fb4 bf859f70 805600a0 00000000 00000000 00000004 00000004 8053cb20 9f80: 00000000 804c9788 bf859fb4 805770a0 00000004 80577080 805d1180 0000008d 9fa0: 8053e208 805827a4 bf859ff4 bf859fb8 8053e974 800086b0 00000004 00000004 9fc0: 8053e208 8053e870 80026750 00000000 8053e870 80026750 00000013 00000000 9fe0: 00000000 00000000 00000000 bf859ff8 80026750 8053e87c 9773f7a7 ed28dffe

Backtrace:
[<802247dc>] (of_get_gpio_regulator_config+0x0/0x2e0) from [<80404a14>] (gpio_regulator_probe+0x40/0x2f0)
 r8:805b4d9c r7:00000000 r6:811056ec r5:bf8e0200 r4:00000000
[<804049d4>] (gpio_regulator_probe+0x0/0x2f0) from [<8024bc0c>] (platform_drv_probe+0x20/0x24)
 r7:00000000 r6:bf8e0208 r5:805b4d9c r4:8061f05c
[<8024bbec>] (platform_drv_probe+0x0/0x24) from [<8024a7d8>] (driver_probe_device+0x78/0x1e8)
[<8024a760>] (driver_probe_device+0x0/0x1e8) from [<8024a9dc>] (__driver_attach+0x94/0x98)
 r8:0000008d r7:00000000 r6:bf8e023c r5:805b4d9c r4:bf8e0208
 r3:00000000
[<8024a948>] (__driver_attach+0x0/0x98) from [<80248f10>] (bus_for_each_dev+0x58/0x84)
 r6:00000000 r5:8024a948 r4:805b4d9c r3:8024a948
[<80248eb8>] (bus_for_each_dev+0x0/0x84) from [<8024a358>] (driver_attach+0x20/0x28)
 r6:bf8e9f00 r5:805b8af8 r4:805b4d9c
[<8024a338>] (driver_attach+0x0/0x28) from [<80249f94>] (bus_add_driver+0x184/0x250)
[<80249e10>] (bus_add_driver+0x0/0x250) from [<8024aefc>] (driver_register+0x80/0x134)
[<8024ae7c>] (driver_register+0x0/0x134) from [<8024bedc>] (platform_driver_register+0x4c/0x60)
 r8:0000008d r7:805d1180 r6:00000000 r5:00000004 r4:bf858000
 r3:00000000
[<8024be90>] (platform_driver_register+0x0/0x60) from [<805600b4>] (gpio_regulator_init+0x14/0x1c)
[<805600a0>] (gpio_regulator_init+0x0/0x1c) from [<800086e4>] (do_one_initcall+0x40/0x184)
[<800086a4>] (do_one_initcall+0x0/0x184) from [<8053e974>] (kernel_init+0x104/0x1c8)
[<8053e870>] (kernel_init+0x0/0x1c8) from [<80026750>] (do_exit+0x0/0x7fc)
Code: e3a02000 e1a00007 eb03f8db e3a020d0 (e5903004)

Signed-off-by Frank Li <[email protected]>

---
 drivers/regulator/gpio-regulator.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/regulator/gpio-regulator.c +1 -1

index e467d0a..b51e757 100644
@@ -183,7 +183,7 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
 
 	/* Fetch states. */
 	prop = of_find_property(np, "states", NULL);
-	proplen = prop->length / sizeof(int);
+	proplen = prop ? prop->length / sizeof(int) : 0;
 
 	config->states = devm_kzalloc(dev,
 				sizeof(struct gpio_regulator_state)

Signature · 1 line

1.7.1

Mark Brown

2012-11-09 16:56 UTC #1 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

drivers/regulator/gpio-regulator.c of_get_gpio_regulator_config(struct device *dev, struct device_node *np) · line 186

Quoted 4 lines · Frank Li

drivers/regulator/gpio-regulator.c +1 -1

 	/* Fetch states. */
 	prop = of_find_property(np, "states", NULL);
-	proplen = prop->length / sizeof(int);
+	proplen = prop ? prop->length / sizeof(int) : 0;

Aren't states mandatory for this driver, in which case shouldn't the
probe fail here?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121109/32576d25/attachment.sig>

Frank Li

2012-11-12 01:56 UTC #2 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

2012/11/10 Mark Brown <[email protected]>:

Quoted 9 lines · Frank Li

drivers/regulator/gpio-regulator.c of_get_gpio_regulator_config(struct device *dev, struct device_node *np) · line 186
Quoted 5 lines
      /* Fetch states. */
      prop = of_find_property(np, "states", NULL);
drivers/regulator/gpio-regulator.c +1 -1
-     proplen = prop->length / sizeof(int);
+     proplen = prop ? prop->length / sizeof(int) : 0;
Aren't states mandatory for this driver, in which case shouldn't the probe fail here?

I think No. GPIO-Regulator can be used as just turn on/off power domain and not adjust voltage or current. So "states" is option. "gpios" was already options.

Mark Brown

2012-11-13 07:57 UTC #3 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

Quoted 8 lines · Frank Li

2012/11/10 Mark Brown <[email protected]>:
Quoted 6 lines · Frank Li
Quoted 5 lines
      /* Fetch states. */
      prop = of_find_property(np, "states", NULL);
drivers/regulator/gpio-regulator.c +1 -1
-     proplen = prop->length / sizeof(int);
+     proplen = prop ? prop->length / sizeof(int) : 0;

Quoted 6 lines

Quoted 2 lines
Aren't states mandatory for this driver, in which case shouldn't the probe fail here?
I think No. GPIO-Regulator can be used as just turn on/off power domain and not adjust voltage or current. So "states" is option. "gpios" was already options.

Are you sure this actually works? I glanced at the code and wasn't
convinced it was taking sufficient care to cope with this possibility.
Normally people use a fixed regulator for a regulator with a simple
on/off switch (which allows the voltage to be specified which this
won't...) so I'd be somewhat surprised if it were tested.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121113/d211d11f/attachment.sig>

Frank Li

2012-11-13 08:12 UTC #4 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

2012/11/13 Mark Brown <[email protected]>:

Quoted 16 lines · Frank Li

Quoted 8 lines
2012/11/10 Mark Brown <[email protected]>:
Quoted 6 lines · Frank Li
Quoted 5 lines
      /* Fetch states. */
      prop = of_find_property(np, "states", NULL);
drivers/regulator/gpio-regulator.c +1 -1
-     proplen = prop->length / sizeof(int);
+     proplen = prop ? prop->length / sizeof(int) : 0;
Quoted 6 lines
Quoted 2 lines
Aren't states mandatory for this driver, in which case shouldn't the probe fail here?
I think No. GPIO-Regulator can be used as just turn on/off power domain and not adjust voltage or current. So "states" is option. "gpios" was already options.

Quoted 5 lines

Are you sure this actually works? I glanced at the code and wasn't convinced it was taking sufficient care to cope with this possibility. Normally people use a fixed regulator for a regulator with a simple on/off switch (which allows the voltage to be specified which this won't...) so I'd be somewhat surprised if it were tested.

Yes, I tested it.
I use gpio-enable to turn on\off a sensor power switcher. the below is a part of my dt file.

+ mma8451 at 1c {

+                                       compatible = "fsl,mma8451";
+                                       reg = <0x1c>;
+                                       vdd-supply = <&sensor>;
+                                       vddio-supply = <&sensor>;
+                               };
+                       };
                };
        };
+       sensor: gpio-regulator {
+               compatible = "regulator-gpio";
+               regulator-name = "sensor-supply";
+               enable-gpio = <&gpio2 31 1>;
+               enable-active-high;
+               regulator-type = "voltage";

+ };

best regards
Frank Li

Mark Brown

2012-11-13 08:17 UTC #5 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

Quoted 3 lines · Frank Li

Yes, I tested it.
I use gpio-enable to turn on\off a sensor power switcher. the below is a part of my dt file.

You should really use a fixed voltage regulator here, or fix the code so
you can specify a voltage for the gpio regulator when no GPIO is
specified (ideally the two drivers would be unified but that's another
story and mostly orthogonal to the DT).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121113/5c5a887c/attachment.sig>

Lee Jones

2012-11-14 09:36 UTC #6 lore raw

PATCH 1/1 regulator: gpio-regulator: fix crash when no states property in dt

In reply to Frank Li

Quoted 68 lines · Frank Li

Unable to handle kernel NULL pointer dereference at virtual address 00000004
pgd = 80004000
[00000004] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0    Not tainted  (3.5.7+ #11)
PC is at of_get_gpio_regulator_config+0x1b0/0x2e0
LR is at of_find_property+0x4c/0x9c
pc : [<8022498c>]    lr : [<80322d44>]    psr: 60000013
sp : bf859de8 ip : bf859dc8 fp : bf859e14
r10: 805d1180 r9 : 8053e208 r8 : 00000001
r7 : 811056ec r6 : 00000000 r5 : bf8e0208 r4 : bf8f2b50 r3 : 805a7e00 r2 : 000000d0 r1 : 00000000 r0 : 00000000
Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c53c7d Table: 1000404a DAC: 00000017 Process swapper/0 (pid: 1, stack limit = 0xbf8582f0)
Stack: (0xbf859de8 to 0xbf85a000)
9de0: bf859e34 bf859df8 8010a040 00000000 bf8e0200 811056ec
9e00: 00000000 805b4d9c bf859e54 bf859e18 80404a14 802247e8 00000000 00000000 9e20: 00000000 00000000 00000000 00000000 00000000 00000000 8061f05c 805b4d9c 9e40: bf8e0208 00000000 bf859e64 bf859e58 8024bc0c 804049e0 bf859e8c bf859e68 9e60: 8024a7d8 8024bbf8 00000000 bf8e0208 805b4d9c bf8e023c 00000000 0000008d 9e80: bf859eac bf859e90 8024a9dc 8024a76c 8024a948 805b4d9c 8024a948 00000000 9ea0: bf859ed4 bf859eb0 80248f10 8024a954 bf83b458 bf8dd934 801f13e0 805b4d9c 9ec0: 805b8af8 bf8e9f00 bf859ee4 bf859ed8 8024a358 80248ec4 bf859f14 bf859ee8 9ee0: 80249f94 8024a344 804eb244 805d1180 805b4d9c 00000004 00000000 805d1180 9f00: 0000008d 805d1180 bf859f3c bf859f18 8024aefc 80249e1c 00000000 bf858000 9f20: 00000004 00000000 805d1180 0000008d bf859f4c bf859f40 8024bedc 8024ae88 9f40: bf859f5c bf859f50 805600b4 8024be9c bf859fb4 bf859f60 800086e4 805600ac 9f60: bf859fb4 bf859f70 805600a0 00000000 00000000 00000004 00000004 8053cb20 9f80: 00000000 804c9788 bf859fb4 805770a0 00000004 80577080 805d1180 0000008d 9fa0: 8053e208 805827a4 bf859ff4 bf859fb8 8053e974 800086b0 00000004 00000004 9fc0: 8053e208 8053e870 80026750 00000000 8053e870 80026750 00000013 00000000 9fe0: 00000000 00000000 00000000 bf859ff8 80026750 8053e87c 9773f7a7 ed28dffe
Backtrace:
[<802247dc>] (of_get_gpio_regulator_config+0x0/0x2e0) from [<80404a14>] (gpio_regulator_probe+0x40/0x2f0)
 r8:805b4d9c r7:00000000 r6:811056ec r5:bf8e0200 r4:00000000
[<804049d4>] (gpio_regulator_probe+0x0/0x2f0) from [<8024bc0c>] (platform_drv_probe+0x20/0x24)
 r7:00000000 r6:bf8e0208 r5:805b4d9c r4:8061f05c
[<8024bbec>] (platform_drv_probe+0x0/0x24) from [<8024a7d8>] (driver_probe_device+0x78/0x1e8)
[<8024a760>] (driver_probe_device+0x0/0x1e8) from [<8024a9dc>] (__driver_attach+0x94/0x98)
 r8:0000008d r7:00000000 r6:bf8e023c r5:805b4d9c r4:bf8e0208
 r3:00000000
[<8024a948>] (__driver_attach+0x0/0x98) from [<80248f10>] (bus_for_each_dev+0x58/0x84)
 r6:00000000 r5:8024a948 r4:805b4d9c r3:8024a948
[<80248eb8>] (bus_for_each_dev+0x0/0x84) from [<8024a358>] (driver_attach+0x20/0x28)
 r6:bf8e9f00 r5:805b8af8 r4:805b4d9c
[<8024a338>] (driver_attach+0x0/0x28) from [<80249f94>] (bus_add_driver+0x184/0x250)
[<80249e10>] (bus_add_driver+0x0/0x250) from [<8024aefc>] (driver_register+0x80/0x134)
[<8024ae7c>] (driver_register+0x0/0x134) from [<8024bedc>] (platform_driver_register+0x4c/0x60)
 r8:0000008d r7:805d1180 r6:00000000 r5:00000004 r4:bf858000
 r3:00000000
[<8024be90>] (platform_driver_register+0x0/0x60) from [<805600b4>] (gpio_regulator_init+0x14/0x1c)
[<805600a0>] (gpio_regulator_init+0x0/0x1c) from [<800086e4>] (do_one_initcall+0x40/0x184)
[<800086a4>] (do_one_initcall+0x0/0x184) from [<8053e974>] (kernel_init+0x104/0x1c8)
[<8053e870>] (kernel_init+0x0/0x1c8) from [<80026750>] (do_exit+0x0/0x7fc)
Code: e3a02000 e1a00007 eb03f8db e3a020d0 (e5903004)
Signed-off-by Frank Li <[email protected]>
---
 drivers/regulator/gpio-regulator.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/regulator/gpio-regulator.c +0 -0
index e467d0a..b51e757 100644

drivers/regulator/gpio-regulator.c of_get_gpio_regulator_config(struct device *dev, struct device_node *np) · line 189

Quoted 8 lines

drivers/regulator/gpio-regulator.c +1 -1

@@ -183,7 +183,7 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
 
 	/* Fetch states. */
 	prop = of_find_property(np, "states", NULL);
-	proplen = prop->length / sizeof(int);
+	proplen = prop ? prop->length / sizeof(int) : 0;
 
 	config->states = devm_kzalloc(dev,
 				sizeof(struct gpio_regulator_state)

I agree that this should fail more gracfully, but a state should be compulsary. If you can't change the value of either state you should be using a fixed regulator.

I'll submit a patch.

Signature · 4 lines

Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog