Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: do not inline MEMREAD/MEMWRITE ioctls #12472

Closed
wants to merge 1 commit into from

Conversation

kempniu
Copy link
Contributor

@kempniu kempniu commented Apr 24, 2023

A Linksys E8450 (mt7622) device running current master has recently started crashing [1]:

[    0.562900] mtk-ecc 1100e000.ecc: probed
[    0.570254] spi-nand spi2.0: Fidelix SPI NAND was found.
[    0.575576] spi-nand spi2.0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
[    0.583780] mtk-snand 1100d000.spi: ECC strength: 4 bits per 512 bytes
[    0.682930] Insufficient stack space to handle exception!
[    0.682939] ESR: 0x0000000096000047 -- DABT (current EL)
[    0.682946] FAR: 0xffffffc008c47fe0
[    0.682948] Task stack:     [0xffffffc008c48000..0xffffffc008c4c000]
[    0.682951] IRQ stack:      [0xffffffc008008000..0xffffffc00800c000]
[    0.682954] Overflow stack: [0xffffff801feb00a0..0xffffff801feb10a0]
[    0.682959] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G S                5.15.107 #0
[    0.682966] Hardware name: Linksys E8450 (DT)
[    0.682969] pstate: 800000c5 (Nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.682975] pc : dequeue_entity+0x0/0x250
[    0.682988] lr : dequeue_task_fair+0x98/0x290
[    0.682992] sp : ffffffc008c48030
[    0.682994] x29: ffffffc008c48030 x28: 0000000000000001 x27: ffffff801feb6380
[    0.683004] x26: 0000000000000001 x25: ffffff801feb6300 x24: ffffff8000068000
[    0.683011] x23: 0000000000000001 x22: 0000000000000009 x21: 0000000000000000
[    0.683017] x20: ffffff801feb6380 x19: ffffff8000068080 x18: 0000000017a740a6
[    0.683024] x17: ffffffc008bae748 x16: ffffffc008bae6d8 x15: ffffffffffffffff
[    0.683031] x14: ffffffffffffffff x13: 0000000000000000 x12: 0000000f00000101
[    0.683038] x11: 0000000000000449 x10: 0000000000000127 x9 : 0000000000000000
[    0.683044] x8 : 0000000000000125 x7 : 0000000000116da1 x6 : 0000000000116da1
[    0.683051] x5 : 00000000001165a1 x4 : ffffff801feb6e00 x3 : 0000000000000000
[    0.683058] x2 : 0000000000000009 x1 : ffffff8000068080 x0 : ffffff801feb6380
[    0.683066] Kernel panic - not syncing: kernel stack overflow
[    0.683069] SMP: stopping secondary CPUs
[    1.648361] SMP: failed to stop secondary CPUs 0-1
[    1.648366] Kernel Offset: disabled
[    1.648368] CPU features: 0x00003000,00000802
[    1.648372] Memory Limit: none

The last working revision was reportedly commit e11d00d ("ath79: create Aruba AP-105 APBoot compatible image") while the first tested revision that failed with the above message was commit 1416b9b ("tools/dwarves: update to 1.25").

The exact reason for why these kernel panics started happening has not been thoroughly investigated. However, since the crash happens right after snand driver initialization, commit fa4dc86 ("kernel: backport MEMREAD ioctl") is the most likely culprit.

The panic message quoted above includes mentions of a stack overflow. A pending upstream kernel patch [2] exists that prevents inlining mtdchar_read_ioctl() and mtdchar_write_ioctl() into mtdchar_ioctl() as the addition of the former triggered compiler warnings related to stack size on some platforms. Add a backport of that patch in hope of fixing the crashes described above.

[1] https://lists.openwrt.org/pipermail/openwrt-devel/2023-April/040872.html
[2] https://lists.infradead.org/pipermail/linux-mtd/2023-April/097912.html

See also #12225

A Linksys E8450 (mt7622) device running current master has recently
started crashing [1]:

    [    0.562900] mtk-ecc 1100e000.ecc: probed
    [    0.570254] spi-nand spi2.0: Fidelix SPI NAND was found.
    [    0.575576] spi-nand spi2.0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
    [    0.583780] mtk-snand 1100d000.spi: ECC strength: 4 bits per 512 bytes
    [    0.682930] Insufficient stack space to handle exception!
    [    0.682939] ESR: 0x0000000096000047 -- DABT (current EL)
    [    0.682946] FAR: 0xffffffc008c47fe0
    [    0.682948] Task stack:     [0xffffffc008c48000..0xffffffc008c4c000]
    [    0.682951] IRQ stack:      [0xffffffc008008000..0xffffffc00800c000]
    [    0.682954] Overflow stack: [0xffffff801feb00a0..0xffffff801feb10a0]
    [    0.682959] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G S                5.15.107 #0
    [    0.682966] Hardware name: Linksys E8450 (DT)
    [    0.682969] pstate: 800000c5 (Nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
    [    0.682975] pc : dequeue_entity+0x0/0x250
    [    0.682988] lr : dequeue_task_fair+0x98/0x290
    [    0.682992] sp : ffffffc008c48030
    [    0.682994] x29: ffffffc008c48030 x28: 0000000000000001 x27: ffffff801feb6380
    [    0.683004] x26: 0000000000000001 x25: ffffff801feb6300 x24: ffffff8000068000
    [    0.683011] x23: 0000000000000001 x22: 0000000000000009 x21: 0000000000000000
    [    0.683017] x20: ffffff801feb6380 x19: ffffff8000068080 x18: 0000000017a740a6
    [    0.683024] x17: ffffffc008bae748 x16: ffffffc008bae6d8 x15: ffffffffffffffff
    [    0.683031] x14: ffffffffffffffff x13: 0000000000000000 x12: 0000000f00000101
    [    0.683038] x11: 0000000000000449 x10: 0000000000000127 x9 : 0000000000000000
    [    0.683044] x8 : 0000000000000125 x7 : 0000000000116da1 x6 : 0000000000116da1
    [    0.683051] x5 : 00000000001165a1 x4 : ffffff801feb6e00 x3 : 0000000000000000
    [    0.683058] x2 : 0000000000000009 x1 : ffffff8000068080 x0 : ffffff801feb6380
    [    0.683066] Kernel panic - not syncing: kernel stack overflow
    [    0.683069] SMP: stopping secondary CPUs
    [    1.648361] SMP: failed to stop secondary CPUs 0-1
    [    1.648366] Kernel Offset: disabled
    [    1.648368] CPU features: 0x00003000,00000802
    [    1.648372] Memory Limit: none

The last working revision was reportedly commit e11d00d ("ath79:
create Aruba AP-105 APBoot compatible image") while the first tested
revision that failed with the above message was commit 1416b9b
("tools/dwarves: update to 1.25").

The exact reason for why these kernel panics started happening has not
been thoroughly investigated.  However, since the crash happens right
after snand driver initialization, commit fa4dc86 ("kernel: backport
MEMREAD ioctl") is the most likely culprit.

The panic message quoted above includes mentions of a stack overflow.  A
pending upstream kernel patch [2] exists that prevents inlining
mtdchar_read_ioctl() and mtdchar_write_ioctl() into mtdchar_ioctl() as
the addition of the former triggered compiler warnings related to stack
size on some platforms.  Add a backport of that patch in hope of fixing
the crashes described above.

[1] https://lists.openwrt.org/pipermail/openwrt-devel/2023-April/040872.html
[2] https://lists.infradead.org/pipermail/linux-mtd/2023-April/097912.html

Signed-off-by: Michał Kępień <openwrt@kempniu.pl>
@github-actions github-actions bot added the kernel pull request/issue with Linux kernel related changes label Apr 24, 2023
@ynezz
Copy link
Member

ynezz commented Apr 24, 2023

commit fa4dc86 ("kernel: backport MEMREAD ioctl") is the most likely culprit.

I'm now 99.9% sure, as reverting the commit fixes the problem.

Add a backport of that patch in hope of fixing the crashes described above.

Thanks a lot for the fix attempt, but this doesn't help.

@f00b4r0
Copy link
Contributor

f00b4r0 commented Apr 24, 2023

It's been pointed out that the crash happens shortly after mtk_snand_ecc_init_ctx() returns, judging by the last output line before the crash.

Looking at target/linux/mediatek/patches-5.15/120-12-v5.19-spi-add-driver-for-MTK-SPI-NAND-Flash-Interface.patch where this function is defined this is a backport from 5.19 that is specific to the mediatek target: could it simply be a matter of missing backported bits for this particular piece of code to play ball with MEMREAD/MEMWRITE?

@kempniu
Copy link
Contributor Author

kempniu commented Apr 25, 2023

Looking at target/linux/mediatek/patches-5.15/120-12-v5.19-spi-add-driver-for-MTK-SPI-NAND-Flash-Interface.patch where this function is defined this is a backport from 5.19 that is specific to the mediatek target: could it simply be a matter of missing backported bits for this particular piece of code to play ball with MEMREAD/MEMWRITE?

@f00b4r0 I don't think so. MEMREAD/MEMWRITE are high-level abstractions that use internal interfaces provided by the Linux kernel for talking to MTDs. mtk-snand is a low-level flash memory driver. If every flash driver needed to be adjusted to handle MEMREAD/MEMWRITE, I would expect tons of other breakage by now.

commit fa4dc86 ("kernel: backport MEMREAD ioctl") is the most likely culprit.

I'm now 99.9% sure, as reverting the commit fixes the problem.

Add a backport of that patch in hope of fixing the crashes described above.

Thanks a lot for the fix attempt, but this doesn't help.

@ynezz Thanks for checking. Is there any chance you could retest master with a kernel configuration that has CONFIG_DEBUG_BUGVERBOSE=y set? The panic message in the log file I looked at is notably missing a stack trace, which could be very useful for pinpointing the code that triggers these crashes.

@f00b4r0
Copy link
Contributor

f00b4r0 commented Apr 25, 2023

Looking at target/linux/mediatek/patches-5.15/120-12-v5.19-spi-add-driver-for-MTK-SPI-NAND-Flash-Interface.patch where this function is defined this is a backport from 5.19 that is specific to the mediatek target: could it simply be a matter of missing backported bits for this particular piece of code to play ball with MEMREAD/MEMWRITE?

@f00b4r0 I don't think so. MEMREAD/MEMWRITE are high-level abstractions that use internal interfaces provided by the Linux kernel for talking to MTDs. mtk-snand is a low-level flash memory driver. If every flash driver needed to be adjusted to handle MEMREAD/MEMWRITE, I would expect tons of other breakage by now.

That makes sense, however AIUI the driver above is not a flash driver: I've only given a quick glance at it but it appears to be a spi-mem glue that does clever things to interface a NAND device. If you look at the comment at the very beginning of this file, it seems in particular to handle the peculiarities of this platform wrt OOB. This might have some bearing here?

In fact your argument cuts both ways: I think it's a clue that this bug seemingly only happens on this platform where this driver is used. Otherwise we would have seen tons of other breakages on OpenWrt devices by now :)

@ynezz Thanks for checking. Is there any chance you could retest master with a kernel configuration that has CONFIG_DEBUG_BUGVERBOSE=y set? The panic message in the log file I looked at is notably missing a stack trace, which could be very useful for pinpointing the code that triggers these crashes.

Indeed, the stack trace would confirm/infirm whether the above driver is really involved in this bug.

@ynezz
Copy link
Member

ynezz commented Apr 25, 2023

[    0.549847] mtk-ecc 1100e000.ecc: probed
[    0.557419] spi-nand spi2.0: Fidelix SPI NAND was found.
[    0.562742] spi-nand spi2.0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
[    0.570945] mtk-snand 1100d000.spi: ECC strength: 4 bits per 512 bytes
[    0.669300] Insufficient stack space to handle exception!
[    0.669311] ESR: 0x0000000096000047 -- DABT (current EL)
[    0.669317] FAR: 0xffffffc008bd7fe0
[    0.669319] Task stack:     [0xffffffc008bd8000..0xffffffc008bdc000]
[    0.669322] IRQ stack:      [0xffffffc008000000..0xffffffc008004000]
[    0.669325] Overflow stack: [0xffffff801fea00a0..0xffffff801fea10a0]
[    0.669330] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S                5.15.108 #0
[    0.669337] Hardware name: Linksys E8450 (DT)
[    0.669340] pstate: 800000c5 (Nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.669346] pc : dequeue_entity+0x0/0x250
[    0.669359] lr : dequeue_task_fair+0x98/0x290
[    0.669363] sp : ffffffc008bd8030
[    0.669365] x29: ffffffc008bd8030 x28: 0000000000000001 x27: ffffff801fea61c0
[    0.669376] x26: 0000000000000001 x25: ffffff801fea6140 x24: ffffff8000068000
[    0.669382] x23: 0000000000000001 x22: 0000000000000009 x21: 0000000000000000
[    0.669389] x20: ffffff801fea61c0 x19: ffffff8000068080 x18: 0000000000000000
[    0.669396] x17: ffffffc008b4e608 x16: ffffffc008b4e598 x15: ffffffffffffffff
[    0.669403] x14: ffffffffffffffff x13: 0000000000000000 x12: 0000000f00000101
[    0.669410] x11: 0000000000000a9e x10: 000000000000012b x9 : 0000000000000000
[    0.669416] x8 : 0000000000000129 x7 : 00000000002ac497 x6 : 00000000002ac497
[    0.669423] x5 : 00000000002abc97 x4 : ffffff801fea6c40 x3 : 0000000000000000
[    0.669429] x2 : 0000000000000009 x1 : ffffff8000068080 x0 : ffffff801fea61c0
[    0.669438] Kernel panic - not syncing: kernel stack overflow
[    0.669441] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S                5.15.108 #0
[    0.669446] Hardware name: Linksys E8450 (DT)
[    0.669449] Call trace:
[    0.669450]  dump_backtrace+0x0/0x170
[    0.669459]  show_stack+0x14/0x30
[    0.669464]  dump_stack_lvl+0x60/0x78
[    0.669473]  dump_stack+0x14/0x2c
[    0.669478]  panic+0x140/0x2e0
[    0.669483]  nmi_panic+0x6c/0x70
[    0.669491]  panic_bad_stack+0xe0/0xf0
[    0.669497]  handle_bad_stack+0x30/0x60
[    0.669502]  __bad_stack+0x88/0x8c
[    0.669506]  dequeue_entity+0x0/0x250
[    0.669511]  __schedule+0x2a4/0x4b0
[    0.669516]  schedule+0x58/0xc0
[    0.669521]  schedule_timeout+0x6c/0xc4
[    0.669528]  __wait_for_common+0xc4/0x1d0
[    0.669533]  wait_for_completion_timeout+0x1c/0x2c
[    0.669538]  mtk_snand_exec_op+0xb4c/0x11a0
[    0.669544]  spi_mem_exec_op+0x394/0x3e4
[    0.669553]  spi_mem_no_dirmap_read+0x6c/0x90
[    0.669559]  spi_mem_dirmap_read+0xcc/0x130
[    0.669565]  spinand_read_page+0xf8/0x1e0
[    0.669573]  spinand_mtd_read+0x1b0/0x2fc
[    0.669579]  scan_bmt+0x90/0x1b0
[    0.669587]  scan_bmt+0xb8/0x1b0
[    0.669592]  scan_bmt+0xb8/0x1b0
[    0.669597]  scan_bmt+0xb8/0x1b0
[    0.669602]  scan_bmt+0xb8/0x1b0
[    0.669607]  scan_bmt+0xb8/0x1b0
[    0.669612]  scan_bmt+0xb8/0x1b0
[    0.669617]  scan_bmt+0xb8/0x1b0
[    0.669622]  scan_bmt+0xb8/0x1b0
[    0.669628]  scan_bmt+0xb8/0x1b0
[    0.669633]  scan_bmt+0xb8/0x1b0
[    0.669638]  scan_bmt+0xb8/0x1b0
[    0.669643]  scan_bmt+0xb8/0x1b0
[    0.669648]  scan_bmt+0xb8/0x1b0
[    0.669653]  scan_bmt+0xb8/0x1b0
[    0.669658]  scan_bmt+0xb8/0x1b0
[    0.669663]  scan_bmt+0xb8/0x1b0
[    0.669669]  scan_bmt+0xb8/0x1b0
[    0.669674]  scan_bmt+0xb8/0x1b0
[    0.669679]  scan_bmt+0xb8/0x1b0
[    0.669684]  scan_bmt+0xb8/0x1b0
[    0.669689]  scan_bmt+0xb8/0x1b0
[    0.669694]  scan_bmt+0xb8/0x1b0
[    0.669699]  scan_bmt+0xb8/0x1b0
[    0.669704]  scan_bmt+0xb8/0x1b0
[    0.669710]  scan_bmt+0xb8/0x1b0
[    0.669715]  scan_bmt+0xb8/0x1b0
[    0.669720]  scan_bmt+0xb8/0x1b0
[    0.669725]  scan_bmt+0xb8/0x1b0
[    0.669730]  scan_bmt+0xb8/0x1b0
[    0.669735]  scan_bmt+0xb8/0x1b0
[    0.669740]  scan_bmt+0xb8/0x1b0
[    0.669745]  scan_bmt+0xb8/0x1b0
[    0.669751]  scan_bmt+0xb8/0x1b0
[    0.669756]  scan_bmt+0xb8/0x1b0
[    0.669761]  scan_bmt+0xb8/0x1b0
[    0.669766]  scan_bmt+0xb8/0x1b0
[    0.669771]  scan_bmt+0xb8/0x1b0
[    0.669776]  scan_bmt+0xb8/0x1b0
[    0.669781]  scan_bmt+0xb8/0x1b0
[    0.669786]  scan_bmt+0xb8/0x1b0
[    0.669791]  scan_bmt+0xb8/0x1b0
[    0.669796]  scan_bmt+0xb8/0x1b0
[    0.669802]  scan_bmt+0xb8/0x1b0
[    0.669807]  scan_bmt+0xb8/0x1b0
[    0.669812]  scan_bmt+0xb8/0x1b0
[    0.669817]  scan_bmt+0xb8/0x1b0
[    0.669822]  scan_bmt+0xb8/0x1b0
[    0.669827]  scan_bmt+0xb8/0x1b0
[    0.669832]  scan_bmt+0xb8/0x1b0
[    0.669838]  scan_bmt+0xb8/0x1b0
[    0.669843]  scan_bmt+0xb8/0x1b0
[    0.669848]  scan_bmt+0xb8/0x1b0
[    0.669853]  scan_bmt+0xb8/0x1b0
[    0.669858]  scan_bmt+0xb8/0x1b0
[    0.669863]  scan_bmt+0xb8/0x1b0
[    0.669868]  scan_bmt+0xb8/0x1b0
[    0.669874]  scan_bmt+0xb8/0x1b0
[    0.669879]  scan_bmt+0xb8/0x1b0
[    0.669884]  scan_bmt+0xb8/0x1b0
[    0.669889]  scan_bmt+0xb8/0x1b0
[    0.669894]  scan_bmt+0xb8/0x1b0
[    0.669899]  scan_bmt+0xb8/0x1b0
[    0.669904]  scan_bmt+0xb8/0x1b0
[    0.669910]  scan_bmt+0xb8/0x1b0
[    0.669915]  scan_bmt+0xb8/0x1b0
[    0.669920]  scan_bmt+0xb8/0x1b0
[    0.669925]  scan_bmt+0xb8/0x1b0
[    0.669930]  scan_bmt+0xb8/0x1b0
[    0.669935]  scan_bmt+0xb8/0x1b0
[    0.669940]  scan_bmt+0xb8/0x1b0
[    0.669945]  scan_bmt+0xb8/0x1b0
[    0.669951]  scan_bmt+0xb8/0x1b0
[    0.669956]  scan_bmt+0xb8/0x1b0
[    0.669961]  scan_bmt+0xb8/0x1b0
[    0.669966]  scan_bmt+0xb8/0x1b0
[    0.669971]  scan_bmt+0xb8/0x1b0
[    0.669976]  mtk_bmt_init_v2+0x12c/0x360
[    0.669981]  mtk_bmt_attach+0x17c/0x2c0
[    0.669987]  spinand_probe+0x50c/0x640
[    0.669994]  spi_mem_probe+0x68/0xb0
[    0.670000]  spi_probe+0x80/0xdc
[    0.670005]  really_probe.part.0+0x98/0x2f4
[    0.670015]  __driver_probe_device+0x94/0x13c
[    0.670021]  driver_probe_device+0x40/0x114
[    0.670026]  __device_attach_driver+0xac/0x124
[    0.670033]  bus_for_each_drv+0x64/0xa0
[    0.670038]  __device_attach+0x98/0x18c
[    0.670044]  device_initial_probe+0x10/0x1c
[    0.670050]  bus_probe_device+0x94/0x9c
[    0.670055]  device_add+0x358/0x860
[    0.670060]  __spi_add_device+0x70/0x130
[    0.670066]  spi_add_device+0x5c/0x90
[    0.670071]  of_register_spi_device+0x2dc/0x524
[    0.670076]  spi_register_controller+0x578/0x78c
[    0.670082]  mtk_snand_probe+0x284/0x394
[    0.670086]  platform_probe+0x64/0xbc
[    0.670092]  really_probe.part.0+0x98/0x2f4
[    0.670097]  __driver_probe_device+0x94/0x13c
[    0.670103]  driver_probe_device+0x40/0x114
[    0.670109]  __driver_attach+0x8c/0x180
[    0.670114]  bus_for_each_dev+0x5c/0x90
[    0.670119]  driver_attach+0x20/0x2c
[    0.670125]  bus_add_driver+0x100/0x1f0
[    0.670130]  driver_register+0x74/0x120
[    0.670136]  __platform_driver_register+0x24/0x30
[    0.670140]  mtk_snand_driver_init+0x18/0x20
[    0.670146]  do_one_initcall+0x4c/0x1b0
[    0.670150]  kernel_init_freeable+0x230/0x294
[    0.670156]  kernel_init+0x20/0x120
[    0.670161]  ret_from_fork+0x10/0x20
[    0.670166] SMP: stopping secondary CPUs
[    0.670173] Kernel Offset: disabled
[    0.670175] CPU features: 0x00003000,00000802
[    0.670179] Memory Limit: none

@kempniu
Copy link
Contributor Author

kempniu commented Apr 25, 2023

Thanks, @ynezz, that looks very useful.

Note the recursion depth in that stack trace. That comes from target/linux/generic/files/drivers/mtd/nand/ - scan_bmt() calls itself recursively. It also calls bbt_nand_read(), which contains:

  • a stack-allocated struct mtd_oob_ops structure, which the MEMREAD backport grows by 8 bytes (sizeof(struct mtd_req_stats *)),
  • invokes spinand_mtd_read(), whose stack size the MEMREAD backport grows by 16 bytes (sizeof(struct mtd_ecc_stats)).

Not to mention that bbt_nand_read() is marked with static inline and its caller, read_bmt(), may itself be inlined into scan_bmt().

It is hard for me to confirm this with 100% certainty without getting my hands on the device in question, but it seems to me that the MEMREAD backport may have pushed this call stack over the limit. Unfortunately I also do not see any way of preventing scan_bmt() from getting called, which could also confirm that is indeed the root cause.

@f00b4r0
Copy link
Contributor

f00b4r0 commented Apr 25, 2023

@kempniu this is sound analysis. What does bother me is that scan_bmt() has obviously been written to be tail-recursive, so it shouldn't trash the stack so badly. Something else is off here?

@kempniu
Copy link
Contributor Author

kempniu commented Apr 27, 2023

Nothing obvious, at least not to me. Since this is the only lead I have for now and I do not have direct access to the affected device, I looked at the code I called out in my previous message and it seems to me that there is no real need for recursion there as the logic just scans flash blocks backwards. IMHO the current code layout only muddles what this code actually does.

@ynezz, could you perhaps give kempniu/openwrt@2f248ab (the mtk_bmt-refactor-to-avoid-deep-recursion branch in my fork) a shot? It replaces the noinline_for_stack patch originally proposed in this PR with a refactoring of scan_bmt() & read_bmt() that intends to prevent deep recursion (by replacing it with iteration). The revised code was not tested beyond compilation.

(Please make sure that CONFIG_DEBUG_BUGVERBOSE=y is still set, in case this approach does not help, either. Thanks!)

@f00b4r0
Copy link
Contributor

f00b4r0 commented Apr 27, 2023

Nothing obvious, at least not to me. Since this is the only lead I have for now and I do not have direct access to the affected device, I looked at the code I called out in my previous message and it seems to me that there is no real need for recursion there as the logic just scans flash blocks backwards. IMHO the current code layout only muddles what this code actually does.

Agreed, the recursion is unnecessary.

@ynezz, could you perhaps give kempniu/openwrt@2f248ab (the mtk_bmt-refactor-to-avoid-deep-recursion branch in my fork) a shot? It replaces the noinline_for_stack patch originally proposed in this PR with a refactoring of scan_bmt() & read_bmt() that intends to prevent deep recursion (by replacing it with iteration). The revised code was not tested beyond compilation.

In case this helps, the new code looks correct. Thanks for proposing this.

@ynezz
Copy link
Member

ynezz commented Apr 27, 2023

@ynezz, could you perhaps give kempniu/openwrt@2f248ab (the mtk_bmt-refactor-to-avoid-deep-recursion branch in my fork) a shot? It replaces the noinline_for_stack patch originally proposed in this PR with a refactoring of scan_bmt() & read_bmt() that intends to prevent deep recursion (by replacing it with iteration). The revised code was not tested beyond compilation.

Nice work, seems to be fixed, thanks!

[    0.555281] mtk-ecc 1100e000.ecc: probed
[    0.562611] spi-nand spi2.0: Fidelix SPI NAND was found.
[    0.568003] spi-nand spi2.0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
[    0.576166] mtk-snand 1100d000.spi: ECC strength: 4 bits per 512 bytes
[    0.682324] bbt_nand_read: 4 bitflips
[    0.686190] bbt_nand_read: 4 bitflips
[    0.690228] bbt_nand_read: 4 bitflips
[    0.694083] bbt_nand_read: 4 bitflips
[    0.697925] bbt_nand_read: 4 bitflips
[    0.701751] bbt_nand_read: 4 bitflips
[    0.771390] bbt_nand_read: 1 bitflips
[    0.844943] [BBT] 80 available blocks in BMT pool
[    0.854292] [BBT] BMT.v2 is written into PBA:0x3ff
[    1.351647] Freeing initrd memory: 3668K
[    1.363307] 12 fixed-partitions partitions found on MTD device spi2.0
[    1.369818] OF: Bad cell count for /spi@1100d000/flash@0/partitions
[    1.376102] OF: Bad cell count for /spi@1100d000/flash@0/partitions
[    1.382662] OF: Bad cell count for /spi@1100d000/flash@0/partitions
[    1.388962] OF: Bad cell count for /spi@1100d000/flash@0/partitions
[    1.395408] OF: Bad cell count for /spi@1100d000/flash@0/partitions
[    1.401700] OF: Bad cell count for /spi@1100d000/flash@0/partitions
[    1.408126] Creating 12 MTD partitions on "spi2.0":
[    1.413001] 0x000000000000-0x000000080000 : "Preloader"
[    1.419212] 0x000000080000-0x0000000c0000 : "ATF"
[    1.424632] 0x0000000c0000-0x000000140000 : "u-boot"
[    1.430727] 0x000000140000-0x0000001c0000 : "u-boot-env"
[    1.437020] 0x0000001c0000-0x0000002c0000 : "factory"
[    1.443741] 0x000000300000-0x000000320000 : "devinfo"
[    1.449384] 0x000000320000-0x000000340000 : "senv"
[    1.454783] 0x000000360000-0x000000380000 : "bootseq"
[    1.460460] 0x000000500000-0x000002300000 : "firmware1"
[    1.496911] 0x000002300000-0x000004100000 : "firmware2"
[    1.533445] 0x000004100000-0x000005a00000 : "data"
[    1.564833] 0x000005a00000-0x000006e00000 : "mfg"

@f00b4r0
Copy link
Contributor

f00b4r0 commented Apr 27, 2023

@ynezz, could you perhaps give kempniu/openwrt@2f248ab (the mtk_bmt-refactor-to-avoid-deep-recursion branch in my fork) a shot? It replaces the noinline_for_stack patch originally proposed in this PR with a refactoring of scan_bmt() & read_bmt() that intends to prevent deep recursion (by replacing it with iteration). The revised code was not tested beyond compilation.

Nice work, seems to be fixed, thanks!

Nice. However that seems to suggest that tail-call optimization is disabled here (although it's also possible the compiler failed to optimize). This is something to bear in mind if stack exhaustion happens again elsewhere.

@nbd168
Copy link
Member

nbd168 commented Apr 27, 2023

The rework also looks good to me, thanks.
Regarding tail-call optimization, the kernel Makefile has this:

ifdef CONFIG_FRAME_POINTER
KBUILD_CFLAGS   += -fno-omit-frame-pointer -fno-optimize-sibling-calls
else
...
endif

-fno-optimize-sibling-calls prevents tail-call optimization

@f00b4r0
Copy link
Contributor

f00b4r0 commented Apr 27, 2023

Indeed, then this explains that!

@kempniu
Copy link
Contributor Author

kempniu commented Apr 27, 2023

Cool, thanks everyone for taking a look. I will soon polish the commit message for the revised fix and prepare a proper (separate) merge request out of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel pull request/issue with Linux kernel related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants