Re: [PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal

[Martin Kaiser <martin@kaiser.cx>]

Thus wrote Lee Jones (lee.jones@linaro.org):

> On Tue, 12 Sep 2017, Martin Kaiser wrote:

> > When fsl-imx25-tsadc is compiled as a module, unloading and reloading
> > the module will lead to a crash.

> > Add a removal function which clears the irq handler and removes the irq
> > domain. With this cleanup in place, it's possible to unload and reload
> > the module.

> More information please.

> What is it specifically that causes the crash?

I tested using a touchscreen and compiled both
drivers/input/touchscreen/fsl-imx25-tcq.c and
drivers/mfd/fsl-imx25-tsadc.c
as modules.

I got the crash after running
insmod fsl-imx25-tcq.ko
insmod fsl-imx25-tsadc.ko
rmmod fsl-imx25-tsadc.ko
insmod fsl-imx25-tsadc.ko

[  133.594246] Unable to handle kernel paging request at virtual address bf005430
[  133.601685] pgd = d3b90000
[  133.604435] [bf005430] *pgd=93b61811, *pte=00000000, *ppte=00000000
[  133.610902] Internal error: Oops: 7 [#1] ARM
[  133.615208] Modules linked in: fsl_imx25_tsadc(+) fsl_imx25_tcq [last unloaded: fsl_imx25_tsadc]
[  133.624078] CPU: 0 PID: 173 Comm: insmod Not tainted 4.13.0-next-20170911+ #132
[  133.631413] Hardware name: Freescale i.MX25 (Device Tree Support)
[  133.637537] task: d3b64000 task.stack: d3a64000
[  133.642131] PC is at irq_find_matching_fwspec+0x40/0x108
[  133.647484] LR is at irq_find_matching_fwspec+0x30/0x108
[  133.652826] pc : [<c004dfac>]    lr : [<c004df9c>]    psr: 20000013
[  133.659116] sp : d3a65bb0  ip : d3a65bb0  fp : d3a65bd4
[  133.664362] r10: 00000000  r9 : c03cac4c  r8 : d3a65c20
[  133.669612] r7 : 00000000  r6 : c0ab5a58  r5 : d3d700dc  r4 : d3b80000
[  133.676167] r3 : bf00542c  r2 : 40000013  r1 : d3b64000  r0 : c0ab5a4c
[  133.682721] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  133.689883] Control: 0005317f  Table: 93b90000  DAC: 00000051
[  133.695655] Process insmod (pid: 173, stack limit = 0xd3a64190)
...
[  133.993772] Backtrace:
[  133.996307] [<c004df6c>] (irq_find_matching_fwspec) from [<c028d5ec>] (of_irq_get+0x58/0x74)
[  134.004798]  r9:d3d737ec r8:d3948000 r7:d3948010 r6:d3b6cb10 r5:00000000 r4:d3d700dc
[  134.012607] [<c028d594>] (of_irq_get) from [<c01ff970>] (platform_get_irq+0x48/0xc8)
[  134.020375]  r4:d3948000
[  134.022990] [<c01ff928>] (platform_get_irq) from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc])
[  134.033019]  r5:00000012 r4:00000000
[  134.036663] [<bf00e11c>] (mx25_tsadc_probe [fsl_imx25_tsadc]) from [<c01ff658>] (platform_drv_probe+0x60/0xb0)
[  134.046711]  r9:00000008 r8:00000000 r7:c0b15680 r6:bf00e7b0 r5:d3948010 r4:bf00e11c
[  134.054504] [<c01ff5f8>] (platform_drv_probe) from [<c01fd5fc>] (driver_probe_device+0x204/0x470)
[  134.063414]  r6:00000000 r5:bf00e7b0 r4:d3948010 r3:c01ff5f8
[  134.069119] [<c01fd3f8>] (driver_probe_device) from [<c01fd940>] (__driver_attach+0xd8/0x118)
[  134.077691]  r9:bf00e84c r8:c0af6078 r7:c0ad8ee0 r6:bf00e7b0 r5:d3948044 r4:d3948010
[  134.085483] [<c01fd868>] (__driver_attach) from [<c01fb734>] (bus_for_each_dev+0x7c/0xa0)


irq_find_matching_fwspec() loops over all registered irq domains, I guess the
crash happens inside this loop. The irq domain is still registered from last
time the module was loaded but the pointer to its operations is invalid after
the module was unloaded.

My hope was that removing the irq domain along with module removal would fix
the crash in a proper way.

> Why is this code not required?  What's stopping this causing a leak?

Not sure I got your point here.

My assumption is that (at least for platform drivers), the removal function is
called only when the previous probe was successful. platform_get_irq() should
then get us the same irq we got in the probe function and we can remove the
handler and the irq domain.

Needless to say, I'm happy to update and re-test the patch if required.

Best regards,

   Martin