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

From: Lee Jones
Date: Mon Sep 18 2017 - 04:50:31 EST


On Thu, 14 Sep 2017, Martin Kaiser wrote:

> Thus wrote Lee Jones (lee.jones@xxxxxxxxxx):
>
> > 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.

Great. Please condense this information as much as possible and place
it in the commit log.

> 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.

Never mind. This was due to an incorrect assumption I made.

You can ignore this.

> 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

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog