Re: [PATCH v3 1/9] pinctrl: mvebu: pinctrl driver core

From: Thomas Petazzoni
Date: Tue Sep 11 2012 - 10:44:22 EST


Hello Sebastian,

Sorry for getting back to you so late about this patch set. I have been
very busy with other things.

Le Mon, 10 Sep 2012 10:39:38 +0200,
Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx> a Ãcrit :

> v3:
> - list of functions is now built out of pin groups passed to core driver
> instead of parsing DT node.

Even though I have gone through your discussion with Stephen Warren on
this, I don't get what you have done exactly, and I'm even more puzzled
by the following piece of code:

> +static int __devinit _add_function(const char **funcs, const char *name)
> +{
> + int n = 0;
> +
> + while (funcs[n]) {
> + /* function already there */
> + if (strcmp(funcs[n], name) == 0)
> + return -EEXIST;
> + n++;
> + }
> + funcs[n] = name;
> + return 0;
> +}
> +
> +static int __devinit mvebu_pinctrl_build_functions(struct platform_device *pdev,
> + struct mvebu_pinctrl *pctl)
> +{
> + const char **prefunc = kzalloc(sizeof(char *), GFP_KERNEL);
> + int num = 0;
> + int n, s;
> +
> + for (n = 0; n < pctl->num_groups; n++) {
> + struct mvebu_pinctrl_group *grp = &pctl->groups[n];
> + for (s = 0; s < grp->num_settings; s++) {
> + /* skip unsupported settings on this variant */
> + if (pctl->variant &&
> + !(pctl->variant & grp->settings[s].variant))
> + continue;
> +
> + /* check for unique functions */
> + if (_add_function(prefunc, grp->settings[s].name))
> + continue;
> +
> + num++;
> + }
> + }
> + return 0;
> +}

What is this supposed to do? It allocates an array prefunc, whose
reference is only stored in a local variable, and anywhere else, so
basically it does nothing except leaking memory unless I got it wrong.

Moreover, this array has only one entry, while the loop accesses
several entries, which probably explains the crash I'm seeing at boot
time:

Unable to handle kernel paging request at virtual address ff74edca
pgd = ef290000
[ff74edca] *pgd=00000000
Internal error: Oops: 15 [#1] ARM
Modules linked in:
CPU: 0 Not tainted (3.6.0-rc1-00021-g1dd75c4-dirty #336)
PC is at strcmp+0x0/0x30
LR is at mvebu_pinctrl_build_functions.isra.1+0x78/0xd0
pc : [<c011b600>] lr : [<c021fbf8>] psr: a0000013
sp : ef02de80 ip : 00000000 fp : c02a7e28
r10: 000002a0 r9 : ef0e4924 r8 : 00000018
r7 : ef2742d0 r6 : ef0cb2b0 r5 : 00000020 r4 : 00000002
r3 : ff74edca r2 : ef0e4920 r1 : c02a7e28 r0 : ff74edca
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c53c7d Table: 2f290059 DAC: 00000015
Process swapper (pid: 1, stack limit = 0xef02c268)
Stack: (0xef02de80 to 0xef02e000)
de80: ef0879c8 ef0e4900 00000043 00000043 ef2742d0 c054dd68 00000002 c057486c
dea0: 00000000 ef0879c8 ef0cb748 c02200f0 ef086348 c054c948 c055b550 c0576230
dec0: c054c6e0 ef0879c8 00000000 c054c6e0 c03008a0 c02f5c88 00000000 c014ed28
dee0: c014ed14 c014da3c ef0879c8 c054c6e0 ef0879fc 00000000 c02e91e0 c014dd30
df00: c054c6e0 c014dca4 00000000 c014c5e0 ef00554c ef0266f0 c054c6e0 c0551708
df20: ef274340 c014d380 c02a7da0 c055bf40 c054c6e0 ef02c000 c055bf40 00000000
df40: c02e91e0 c014e1e0 00000000 c02fdb4c ef02c000 c055bf40 00000000 c02e91e0
df60: c02f5c88 c00085c0 c02d1768 00000006 00000044 c02f5c88 00000044 00000006
df80: 00000006 c02e91e0 c1857e15 00000000 00000000 c02fdb4c 00000006 c02fdb2c
dfa0: c055bf40 c02e91e0 c03008a0 00000044 00000000 c02e9838 00000006 00000006
dfc0: c02e91e0 c030066c c030066c c030066c c000a04c 00000013 00000000 00000000
dfe0: 00000000 c02e98cc 00000000 00000000 c02e986c c000a04c be8ff7c8 e8dfff5e
[<c011b600>] (strcmp+0x0/0x30) from [<c021fbf8>] (mvebu_pinctrl_build_functions.isra.1+0x78/0xd0)
[<c021fbf8>] (mvebu_pinctrl_build_functions.isra.1+0x78/0xd0) from [<c02200f0>] (mvebu_pinctrl_probe+0x4a0/0x564)
[<c02200f0>] (mvebu_pinctrl_probe+0x4a0/0x564) from [<c014ed28>] (platform_drv_probe+0x14/0x18)
[<c014ed28>] (platform_drv_probe+0x14/0x18) from [<c014da3c>] (really_probe+0x60/0x1e4)
[<c014da3c>] (really_probe+0x60/0x1e4) from [<c014dd30>] (__driver_attach+0x8c/0x90)
[<c014dd30>] (__driver_attach+0x8c/0x90) from [<c014c5e0>] (bus_for_each_dev+0x50/0x7c)
[<c014c5e0>] (bus_for_each_dev+0x50/0x7c) from [<c014d380>] (bus_add_driver+0x168/0x22c)
[<c014d380>] (bus_add_driver+0x168/0x22c) from [<c014e1e0>] (driver_register+0x78/0x144)
[<c014e1e0>] (driver_register+0x78/0x144) from [<c00085c0>] (do_one_initcall+0x34/0x174)
[<c00085c0>] (do_one_initcall+0x34/0x174) from [<c02e9838>] (do_basic_setup+0x90/0xc4)
[<c02e9838>] (do_basic_setup+0x90/0xc4) from [<c02e98cc>] (kernel_init+0x60/0xf4)
[<c02e98cc>] (kernel_init+0x60/0xf4) from [<c000a04c>] (kernel_thread_exit+0x0/0x8)
Code: e3530000 e4c23001 1afffffb e12fff1e (e4d03001)
---[ end trace a80e01be2a5f5b89 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

I'd like to fix that myself, but I really don't understand what the
"functions" word mean now in this v3, as compared to v1/v2 of this
patch set. Could you enlighten me on this?

Thanks!

Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/