| Message ID | b669f8e2aebcfe7a0937175058364daa5862d862.1698766265.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH/RFC] mmc: tmio: Cancel delayed work before freeing host | expand |
On Tue, 31 Oct 2023 at 16:48, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > > On RZ/Five SMARC EVK, where probing of SDHI fails due to missing pin > control: > > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 0 at kernel/time/timer.c:1739 __run_timers.part.0+0x1e8/0x22a > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-10953-ga37a67d260e6-dirty #86 > Hardware name: Renesas SMARC EVK based on r9a07g043f01 (DT) > epc : __run_timers.part.0+0x1e8/0x22a > ra : __run_timers.part.0+0x130/0x22a > epc : ffffffff8007540c ra : ffffffff80075354 sp : ffffffc800003e60 > gp : ffffffff814f5f08 tp : ffffffff8140d5c0 t0 : 0000000000046600 > t1 : 0000000000000001 t2 : ffffffff81200c10 s0 : ffffffc800003f20 > s1 : ffffffd8023bc4a0 a0 : 00000000fffee790 a1 : 0000000000000200 > a2 : 0000000000000200 a3 : ffffffff81489640 a4 : ffffffc800003e60 > a5 : 0000000000000000 a6 : 0000000000000000 a7 : ffffffc800003e68 > s2 : 0000000000200000 s3 : 0000000000000122 s4 : 0000000000000000 > s5 : ffffffffffffffff s6 : ffffffff814896c0 s7 : ffffffff814f58a0 > s8 : ffffffff80f8bec8 s9 : 0000000000000000 s10: ffffffc800003e60 > s11: ffffffff81489640 t3 : ffffffff81489678 t4 : 0000000000000240 > t5 : ffffffd8024ac018 t6 : ffffffd8024ac038 > status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003 > [<ffffffff8007540c>] __run_timers.part.0+0x1e8/0x22a > [<ffffffff80075472>] run_timer_softirq+0x24/0x4a > [<ffffffff80804ec6>] __do_softirq+0xc6/0x212 > [<ffffffff80027434>] irq_exit_rcu+0x7c/0x9a > [<ffffffff807fcd8a>] handle_riscv_irq+0x40/0x4e > [<ffffffff807fd7f8>] do_irq+0x40/0x68 > ---[ end trace 0000000000000000 ]--- > > What happens? > > renesas_sdhi_probe() > { > tmio_mmc_host_alloc() > mmc_alloc_host() > INIT_DELAYED_WORK(&host->detect, mmc_rescan); > > devm_request_irq(tmio_mmc_irq); > > /* > * After this, the interrupt handler may be invoked at any time > * > * tmio_mmc_irq() > * { > * __tmio_mmc_card_detect_irq() > * mmc_detect_change() > * _mmc_detect_change() > * mmc_schedule_delayed_work(&host->detect, delay); > * } > */ > > tmio_mmc_host_probe() > tmio_mmc_init_ocr() > -EPROBE_DEFER > > tmio_mmc_host_free() > mmc_free_host() > } > > When expire_timers() runs later, it warns because the scheduled work was > freed, and now contains a NULL handler function pointer. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Calling __mmc_stop_host() instead works too, but __mmc_stop_host() is an > internal core function, and is not exported to modules yet. > > Perhaps this should be handled by mmc_free_host() instead? That sounds reasonable to me. Or actually not "instead of" __mmc_stop_host(), but rather from mmc_free_host() too. mmc_stop_host() also needs to make sure that mmc_rescan() isn't currently being executed, so that when setting the "host->rescan_disable = 1;" it really takes effect. Do you want to send a patch? > --- > drivers/mmc/host/tmio_mmc_core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index be7f18fd4836ab29..1e56e78a020d94b9 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1132,6 +1132,7 @@ EXPORT_SYMBOL_GPL(tmio_mmc_host_alloc); > > void tmio_mmc_host_free(struct tmio_mmc_host *host) > { > + cancel_delayed_work_sync(&host->mmc->detect); > mmc_free_host(host->mmc); > } > EXPORT_SYMBOL_GPL(tmio_mmc_host_free); Kind regards Uffe
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index be7f18fd4836ab29..1e56e78a020d94b9 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1132,6 +1132,7 @@ EXPORT_SYMBOL_GPL(tmio_mmc_host_alloc); void tmio_mmc_host_free(struct tmio_mmc_host *host) { + cancel_delayed_work_sync(&host->mmc->detect); mmc_free_host(host->mmc); } EXPORT_SYMBOL_GPL(tmio_mmc_host_free);
On RZ/Five SMARC EVK, where probing of SDHI fails due to missing pin control: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 0 at kernel/time/timer.c:1739 __run_timers.part.0+0x1e8/0x22a Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-10953-ga37a67d260e6-dirty #86 Hardware name: Renesas SMARC EVK based on r9a07g043f01 (DT) epc : __run_timers.part.0+0x1e8/0x22a ra : __run_timers.part.0+0x130/0x22a epc : ffffffff8007540c ra : ffffffff80075354 sp : ffffffc800003e60 gp : ffffffff814f5f08 tp : ffffffff8140d5c0 t0 : 0000000000046600 t1 : 0000000000000001 t2 : ffffffff81200c10 s0 : ffffffc800003f20 s1 : ffffffd8023bc4a0 a0 : 00000000fffee790 a1 : 0000000000000200 a2 : 0000000000000200 a3 : ffffffff81489640 a4 : ffffffc800003e60 a5 : 0000000000000000 a6 : 0000000000000000 a7 : ffffffc800003e68 s2 : 0000000000200000 s3 : 0000000000000122 s4 : 0000000000000000 s5 : ffffffffffffffff s6 : ffffffff814896c0 s7 : ffffffff814f58a0 s8 : ffffffff80f8bec8 s9 : 0000000000000000 s10: ffffffc800003e60 s11: ffffffff81489640 t3 : ffffffff81489678 t4 : 0000000000000240 t5 : ffffffd8024ac018 t6 : ffffffd8024ac038 status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003 [<ffffffff8007540c>] __run_timers.part.0+0x1e8/0x22a [<ffffffff80075472>] run_timer_softirq+0x24/0x4a [<ffffffff80804ec6>] __do_softirq+0xc6/0x212 [<ffffffff80027434>] irq_exit_rcu+0x7c/0x9a [<ffffffff807fcd8a>] handle_riscv_irq+0x40/0x4e [<ffffffff807fd7f8>] do_irq+0x40/0x68 ---[ end trace 0000000000000000 ]--- What happens? renesas_sdhi_probe() { tmio_mmc_host_alloc() mmc_alloc_host() INIT_DELAYED_WORK(&host->detect, mmc_rescan); devm_request_irq(tmio_mmc_irq); /* * After this, the interrupt handler may be invoked at any time * * tmio_mmc_irq() * { * __tmio_mmc_card_detect_irq() * mmc_detect_change() * _mmc_detect_change() * mmc_schedule_delayed_work(&host->detect, delay); * } */ tmio_mmc_host_probe() tmio_mmc_init_ocr() -EPROBE_DEFER tmio_mmc_host_free() mmc_free_host() } When expire_timers() runs later, it warns because the scheduled work was freed, and now contains a NULL handler function pointer. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Calling __mmc_stop_host() instead works too, but __mmc_stop_host() is an internal core function, and is not exported to modules yet. Perhaps this should be handled by mmc_free_host() instead? --- drivers/mmc/host/tmio_mmc_core.c | 1 + 1 file changed, 1 insertion(+)