power_supply_show_property Kernel Oops

power_supply_show_property Kernel Oops

[Alistair]

Hey,

I am working on mainlining support for the reMarkable2 eInk tablet.

I haven an out of tree max77818 power controller [1], [2] and [3] that I
haven't upstreamed yet, but am using on top of the mainline kernel.

My driver worked with the 6.2 kernel [4], but since rebasing to the 6.3-rc4
kernel [5] I see this kernel oops:

```
[    2.107861] 8<--- cut here ---
[    2.110964] Unable to handle kernel paging request at virtual address 002080a2 when read
[    2.119064] [002080a2] *pgd=00000000
[    2.122675] Internal error: Oops: 5 [#1] SMP ARM
[    2.127302] Modules linked in:
[    2.130369] CPU: 1 PID: 49 Comm: kworker/1:2 Not tainted 6.3.0-rc4-00043-ga28b4b2f86cb-dirty #6
[    2.139079] Hardware name: Freescale i.MX7 Dual (Device Tree)
[    2.144832] Workqueue: events power_supply_changed_work
[    2.150081] PC is at string+0x80/0x158
[    2.153847] LR is at 0xc2826fff
[    2.156996] pc : [<c0da9314>]    lr : [<c2826fff>]    psr: a0000013
[    2.163270] sp : f0dadd98  ip : c2827000  fp : f0dadddc
[    2.168501] r10: c2827000  r9 : c2828000  r8 : c10ec1da
[    2.173733] r7 : f0dade34  r6 : 00000002  r5 : f0dadd98  r4 : f0daddb0
[    2.180268] r3 : 002080a2  r2 : c2828000  r1 : 00000000  r0 : c2827000
[    2.186803] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    2.193949] Control: 10c5387d  Table: 8000406a  DAC: 00000051
[    2.199699] Register r0 information: non-slab/vmalloc memory
[    2.205371] Register r1 information: NULL pointer
[    2.210084] Register r2 information: non-slab/vmalloc memory
[    2.215753] Register r3 information: non-paged memory
[    2.220814] Register r4 information: 2-page vmalloc region starting at 0xf0dac000 allocated at kernel_
clone+0xa4/0x360
[    2.231542] Register r5 information: 2-page vmalloc region starting at 0xf0dac000 allocated at kernel_
clone+0xa4/0x360
[    2.242265] Register r6 information: non-paged memory
[    2.247326] Register r7 information: 2-page vmalloc region starting at 0xf0dac000 allocated at kernel_
clone+0xa4/0x360
[    2.258048] Register r8 information: non-slab/vmalloc memory
[    2.263718] Register r9 information: non-slab/vmalloc memory
[    2.269387] Register r10 information: non-slab/vmalloc memory
[    2.275144] Register r11 information: 2-page vmalloc region starting at 0xf0dac000 allocated at kernel_clone+0xa4/0x360
[    2.285953] Register r12 information: non-slab/vmalloc memory
[    2.291709] Process kworker/1:2 (pid: 49, stack limit = 0xf6b02a82)
[    2.297990] Stack: (0xf0dadd98 to 0xf0dae000)
[    2.302356] dd80:                                                       ffffff04 ffff0a00
[    2.310546] dda0: f0daddf0 c07b982c ffffff04 ffff0a00 f0dade34 c10ec1d8 c2827000 c0dac10c
[    2.318735] ddc0: c254b420 ffffff04 ffff0a00 000000b7 f0daddf0 00001000 0000120c ffffff04
[    2.326924] dde0: ffff0a00 dbe877b2 f0dade38 00001000 00000000 c254b420 c15262e0 0000120c
[    2.335113] de00: c2827000 c254b400 c11145cc c0dac3e8 0000004d c03363ec 00000001 dbe877b2
[    2.343302] de20: f0dade30 dbe877b2 c09ca444 c10ec1d8 002080a2 00000000 00000000 dbe877b2
[    2.351491] de40: ffffff04 0000004d c2827000 c2181000 000004d0 c15250b0 c2827000 c0ea7d94
[    2.359680] de60: c11145cc c09ca5f8 00000000 00000006 c254b420 c2181000 c254b400 c2827000
[    2.367869] de80: c0ea7d94 c09ca7d8 c254b420 c2181000 00000000 c10f3358 00000002 00000000
[    2.376058] dea0: c0ea7d94 c07986e0 00000000 c0ea7d94 c11145cc c0d8f7e8 f0daded0 dbe877b2
[    2.384247] dec0: c254b420 dbe877b2 c0d8fdf0 c254b420 c2181000 c0d8fe5c c09c88d4 dbe877b2
[    2.392435] dee0: c254b420 c2822f00 c254b400 00000000 c254b634 c254b5f8 00000000 c254b634
[    2.400624] df00: c254b400 c254b420 c15a2fa4 00000040 ef7d4880 c09c8af0 c254b5f8 c27f1e00
[    2.408813] df20: ef7d4880 ef7d7c00 00000000 ef7d7c05 00000040 c013ddb8 c1403d40 c2321100
[    2.417002] df40: c27f1e00 ef7d4880 ef7d489c c1403d40 c2321100 c27f1e18 00000008 c013ea40
[    2.425191] df60: c27f1e00 c2620340 f08f1eb8 c27f3880 c2321100 c013ea14 c27f1e00 c2620340
[    2.433380] df80: f08f1eb8 00000000 00000000 c0146218 c27f3880 c014614c 00000000 00000000
[    2.441568] dfa0: 00000000 00000000 00000000 c0100170 00000000 00000000 00000000 00000000
[    2.449756] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.457943] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    2.466136]  string from vsnprintf+0x158/0x424
[    2.470603]  vsnprintf from vscnprintf+0x10/0x24
[    2.475241]  vscnprintf from sysfs_emit+0x50/0xac
[    2.479975]  sysfs_emit from power_supply_show_property+0x1d0/0x26c
[    2.486269]  power_supply_show_property from add_prop_uevent+0x30/0x8c
[    2.492815]  add_prop_uevent from power_supply_uevent+0xb4/0xe4
[    2.498753]  power_supply_uevent from dev_uevent+0xc4/0x21c
[    2.504352]  dev_uevent from kobject_uevent_env+0x1cc/0x510
[    2.509953]  kobject_uevent_env from power_supply_changed_work+0x7c/0xb4
[    2.516675]  power_supply_changed_work from process_one_work+0x1e8/0x3e8
[    2.523396]  process_one_work from worker_thread+0x2c/0x504
[    2.528986]  worker_thread from kthread+0xcc/0xec
[    2.533716]  kthread from ret_from_fork+0x14/0x24
[    2.538443] Exception stack(0xf0dadfb0 to 0xf0dadff8)
[    2.543505] dfa0:                                     00000000 00000000 00000000 00000000
[    2.551692] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.559879] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    2.566507] Code: e2800001 e150000e e2811001 0a000002 (e4d3c001)
[    2.572682] ---[ end trace 0000000000000000 ]---
```

As it's on a consumer device I don't have a way to connect a debugger. So I'm
a little stuck on what the problem is. The only related change I see between
6.2 and 6.3-rc4 is commit a441f3b90a340e5c94df36c33fb7000193ee0aa7
"power: supply: use sysfs_emit() instead of sprintf() for sysfs show()", but
that doesn't look like it would cause this oops.

I would really appreciate any help or ideas on what the issue might be or how
I could fix it

1: https://github.com/alistair23/linux/commit/5f1579e12c35d75529328e84d78b36ac11da8cd4
2: https://github.com/alistair23/linux/commit/c6383382278c5e594bb889f62dab853da8b73532
3: https://github.com/alistair23/linux/commit/5d8f2d3a08b8604a0ca5282f2b0aea9ebe7ee5fa
4: https://github.com/alistair23/linux/tree/rM2-6.2
5: https://github.com/alistair23/linux/tree/rM2-mainline

Alistair

Re: power_supply_show_property Kernel Oops

[Linus Walleij]

On Wed, Mar 29, 2023 at 1:16 PM Alistair <alistair@alistair23.me> wrote:

> [    2.466136]  string from vsnprintf+0x158/0x424
> [    2.470603]  vsnprintf from vscnprintf+0x10/0x24
> [    2.475241]  vscnprintf from sysfs_emit+0x50/0xac
> [    2.479975]  sysfs_emit from power_supply_show_property+0x1d0/0x26c
> [    2.486269]  power_supply_show_property from add_prop_uevent+0x30/0x8c
> [    2.492815]  add_prop_uevent from power_supply_uevent+0xb4/0xe4
> [    2.498753]  power_supply_uevent from dev_uevent+0xc4/0x21c
> [    2.504352]  dev_uevent from kobject_uevent_env+0x1cc/0x510
> [    2.509953]  kobject_uevent_env from power_supply_changed_work+0x7c/0xb4
> [    2.516675]  power_supply_changed_work from process_one_work+0x1e8/0x3e8
> [    2.523396]  process_one_work from worker_thread+0x2c/0x504
> [    2.528986]  worker_thread from kthread+0xcc/0xec
> [    2.533716]  kthread from ret_from_fork+0x14/0x24
> [    2.538443] Exception stack(0xf0dadfb0 to 0xf0dadff8)

This looks like running a worker before something this worker is accessing
has been set up.

> As it's on a consumer device I don't have a way to connect a debugger. So I'm
> a little stuck on what the problem is. The only related change I see between
> 6.2 and 6.3-rc4 is commit a441f3b90a340e5c94df36c33fb7000193ee0aa7
> "power: supply: use sysfs_emit() instead of sprintf() for sysfs show()", but
> that doesn't look like it would cause this oops.

Did you try reverting it?

Yours,
Linus Walleij

Re: power_supply_show_property Kernel Oops

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

Hi,

On Wed, Mar 29, 2023 at 04:43:19PM +0200, Linus Walleij wrote:
> On Wed, Mar 29, 2023 at 1:16 PM Alistair <alistair@alistair23.me> wrote:
> 
> > [    2.466136]  string from vsnprintf+0x158/0x424
> > [    2.470603]  vsnprintf from vscnprintf+0x10/0x24
> > [    2.475241]  vscnprintf from sysfs_emit+0x50/0xac
> > [    2.479975]  sysfs_emit from power_supply_show_property+0x1d0/0x26c
> > [    2.486269]  power_supply_show_property from add_prop_uevent+0x30/0x8c
> > [    2.492815]  add_prop_uevent from power_supply_uevent+0xb4/0xe4
> > [    2.498753]  power_supply_uevent from dev_uevent+0xc4/0x21c
> > [    2.504352]  dev_uevent from kobject_uevent_env+0x1cc/0x510
> > [    2.509953]  kobject_uevent_env from power_supply_changed_work+0x7c/0xb4
> > [    2.516675]  power_supply_changed_work from process_one_work+0x1e8/0x3e8
> > [    2.523396]  process_one_work from worker_thread+0x2c/0x504
> > [    2.528986]  worker_thread from kthread+0xcc/0xec
> > [    2.533716]  kthread from ret_from_fork+0x14/0x24
> > [    2.538443] Exception stack(0xf0dadfb0 to 0xf0dadff8)
> 
> This looks like running a worker before something this worker is
> accessing has been set up.
> 
> > As it's on a consumer device I don't have a way to connect a debugger. So I'm
> > a little stuck on what the problem is. The only related change I see between
> > 6.2 and 6.3-rc4 is commit a441f3b90a340e5c94df36c33fb7000193ee0aa7
> > "power: supply: use sysfs_emit() instead of sprintf() for sysfs show()", but
> > that doesn't look like it would cause this oops.
> 
> Did you try reverting it?

Does not look like a race condition with a worker to me. The patch
adds a couple of properties to the power-supply in an incorrect way.
I did not look deeply, but it's at least missing an update to
power_supply_attrs. I guess you were 'lucky' that it did not crash
with v6.2.

None of the extra properties are acceptable upstream btw.:

POWER_SUPPLY_PROP_CURRENT_MAX2:
    The driver seems to use CURRENT_MAX2 for input current;
    POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT should be used for that

POWER_SUPPLY_PROP_CHARGER_MODE:
    OTG should be handled via a regulator

POWER_SUPPLY_PROP_STATUS_EX:
    Use extcon for connectors

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: power_supply_show_property Kernel Oops

[Alistair]

On Thu, 30 Mar 2023, at 8:47 AM, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Mar 29, 2023 at 04:43:19PM +0200, Linus Walleij wrote:
> > On Wed, Mar 29, 2023 at 1:16 PM Alistair <alistair@alistair23.me> wrote:
> > 
> > > [    2.466136]  string from vsnprintf+0x158/0x424
> > > [    2.470603]  vsnprintf from vscnprintf+0x10/0x24
> > > [    2.475241]  vscnprintf from sysfs_emit+0x50/0xac
> > > [    2.479975]  sysfs_emit from power_supply_show_property+0x1d0/0x26c
> > > [    2.486269]  power_supply_show_property from add_prop_uevent+0x30/0x8c
> > > [    2.492815]  add_prop_uevent from power_supply_uevent+0xb4/0xe4
> > > [    2.498753]  power_supply_uevent from dev_uevent+0xc4/0x21c
> > > [    2.504352]  dev_uevent from kobject_uevent_env+0x1cc/0x510
> > > [    2.509953]  kobject_uevent_env from power_supply_changed_work+0x7c/0xb4
> > > [    2.516675]  power_supply_changed_work from process_one_work+0x1e8/0x3e8
> > > [    2.523396]  process_one_work from worker_thread+0x2c/0x504
> > > [    2.528986]  worker_thread from kthread+0xcc/0xec
> > > [    2.533716]  kthread from ret_from_fork+0x14/0x24
> > > [    2.538443] Exception stack(0xf0dadfb0 to 0xf0dadff8)
> > 
> > This looks like running a worker before something this worker is
> > accessing has been set up.
> > 
> > > As it's on a consumer device I don't have a way to connect a debugger. So I'm
> > > a little stuck on what the problem is. The only related change I see between
> > > 6.2 and 6.3-rc4 is commit a441f3b90a340e5c94df36c33fb7000193ee0aa7
> > > "power: supply: use sysfs_emit() instead of sprintf() for sysfs show()", but
> > > that doesn't look like it would cause this oops.
> > 
> > Did you try reverting it?
> 
> Does not look like a race condition with a worker to me. The patch
> adds a couple of properties to the power-supply in an incorrect way.

Thanks for that.

It appears that these two drivers cause the problem:

https://github.com/alistair23/linux/blob/rM2-mainline/drivers/power/supply/max77818-charger.c
https://github.com/alistair23/linux/blob/rM2-mainline/drivers/regulator/max77818-regulator.c 

as removing those two fixes the oops.

> I did not look deeply, but it's at least missing an update to
> power_supply_attrs. I guess you were 'lucky' that it did not crash
> with v6.2.

Do you mind pointing to an example that does this correctly? I don't see
anything obviously wrong compared to the in-tree max* drivers.

Anything to help point me in the right direction would be greatly
appreciated :)

> 
> None of the extra properties are acceptable upstream btw.:
> 
> POWER_SUPPLY_PROP_CURRENT_MAX2:
>     The driver seems to use CURRENT_MAX2 for input current;
>     POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT should be used for that
> 
> POWER_SUPPLY_PROP_CHARGER_MODE:
>     OTG should be handled via a regulator
> 
> POWER_SUPPLY_PROP_STATUS_EX:
>     Use extcon for connectors

Thanks! I figured as much. These are just taken from the vendor tree, I need to
clean everything up for upstreaming. I just wanted to have a working base point
to start.

> 
> Greetings,
> 
> -- Sebastian
> 
> 
> *Attachments:*
>  • signature.asc

Re: power_supply_show_property Kernel Oops

[Alistair]

On Thu, 30 Mar 2023, at 12:43 AM, Linus Walleij wrote:
> On Wed, Mar 29, 2023 at 1:16 PM Alistair <alistair@alistair23.me> wrote:
> 
> > [    2.466136]  string from vsnprintf+0x158/0x424
> > [    2.470603]  vsnprintf from vscnprintf+0x10/0x24
> > [    2.475241]  vscnprintf from sysfs_emit+0x50/0xac
> > [    2.479975]  sysfs_emit from power_supply_show_property+0x1d0/0x26c
> > [    2.486269]  power_supply_show_property from add_prop_uevent+0x30/0x8c
> > [    2.492815]  add_prop_uevent from power_supply_uevent+0xb4/0xe4
> > [    2.498753]  power_supply_uevent from dev_uevent+0xc4/0x21c
> > [    2.504352]  dev_uevent from kobject_uevent_env+0x1cc/0x510
> > [    2.509953]  kobject_uevent_env from power_supply_changed_work+0x7c/0xb4
> > [    2.516675]  power_supply_changed_work from process_one_work+0x1e8/0x3e8
> > [    2.523396]  process_one_work from worker_thread+0x2c/0x504
> > [    2.528986]  worker_thread from kthread+0xcc/0xec
> > [    2.533716]  kthread from ret_from_fork+0x14/0x24
> > [    2.538443] Exception stack(0xf0dadfb0 to 0xf0dadff8)
> 
> This looks like running a worker before something this worker is accessing
> has been set up.
> 
> > As it's on a consumer device I don't have a way to connect a debugger. So I'm
> > a little stuck on what the problem is. The only related change I see between
> > 6.2 and 6.3-rc4 is commit a441f3b90a340e5c94df36c33fb7000193ee0aa7
> > "power: supply: use sysfs_emit() instead of sprintf() for sysfs show()", but
> > that doesn't look like it would cause this oops.
> 
> Did you try reverting it?

I did after I sent this. Reverting the commit doesn't fix the issue.

Alistair

> 
> Yours,
> Linus Walleij
> 

Re: power_supply_show_property Kernel Oops

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

Hi,

On Thu, Mar 30, 2023 at 06:00:41PM +1000, Alistair wrote:
> > I did not look deeply, but it's at least missing an update to
> > power_supply_attrs. I guess you were 'lucky' that it did not crash
> > with v6.2.
> 
> Do you mind pointing to an example that does this correctly? I don't see
> anything obviously wrong compared to the in-tree max* drivers.
> 
> Anything to help point me in the right direction would be greatly
> appreciated :)

I am talking about the extra additions to the core, that have been
done incorrectly. You cannot see that in other max* drivers. The
additions to the core basically break the core and thus the drivers
are broken.

> > None of the extra properties are acceptable upstream btw.:
> > 
> > POWER_SUPPLY_PROP_CURRENT_MAX2:
> >     The driver seems to use CURRENT_MAX2 for input current;
> >     POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT should be used for that
> > 
> > POWER_SUPPLY_PROP_CHARGER_MODE:
> >     OTG should be handled via a regulator
> > 
> > POWER_SUPPLY_PROP_STATUS_EX:
> >     Use extcon for connectors
> 
> Thanks! I figured as much. These are just taken from the vendor
> tree, I need to clean everything up for upstreaming. I just wanted
> to have a working base point to start.

That did not turn out that well so far... :)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]