Date:   Mon, 18 Sep 2017 09:50:21 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Martin Kaiser <martin@...ser.cx>
Cc:     kernel@...gutronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal

On Thu, 14 Sep 2017, Martin Kaiser wrote:

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

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

Powered by blists - more mailing lists