From: Markus Pargmann <hidden> Date: 2014-12-01 06:51:01
Hi,
On Mon, Dec 01, 2014 at 11:50:51AM +0900, Jiada Wang wrote:
irq_dispose_mapping() in turns calls unregister_irq_proc(),
which will remove irq proc entry, if IRQ is not freed
before calling of irq_dispose_mapping(), then it will cause
kernel warning.
By free IRQ before irq_dispose_mapping(), this patch fix
the following kernel warning found when remove of fsl_ssi driver:
[ 31.515336] ------------[ cut here ]------------
[ 31.520091] WARNING: CPU: 2 PID: 434 at fs/proc/generic.c:521 remove_proc_entry+0x14c/0x16c()
[ 31.528708] remove_proc_entry: removing non-empty directory 'irq/79', leaking at least '202c000.ss'
[ 31.537911] Modules linked in: snd_soc_wm8962 snd_soc_imx_wm8962 snd_soc_fsl_ssi(-) evbug
[ 31.546249] CPU: 2 PID: 434 Comm: rmmod Not tainted 3.18.0-rc6-00028-g3314bf6-dirty #1
[ 31.554235] Backtrace:
[ 31.556816] [<80011ea8>] (dump_backtrace) from [<80012044>] (show_stack+0x18/0x1c)
[ 31.564416] r6:80142c88 r5:00000000 r4:00000000 r3:00000000
[ 31.570267] [<8001202c>] (show_stack) from [<806980ec>] (dump_stack+0x88/0xa4)
[ 31.577588] [<80698064>] (dump_stack) from [<80029d78>] (warn_slowpath_common+0x70/0x94)
[ 31.585711] r5:00000009 r4:bb61fd90
[ 31.589423] [<80029d08>] (warn_slowpath_common) from [<80029e40>] (warn_slowpath_fmt+0x38/0x40)
[ 31.598187] r8:bb61fdfe r7:be05d76d r6:be05d9a8 r5:00000002 r4:be05d700
[ 31.605054] [<80029e0c>] (warn_slowpath_fmt) from [<80142c88>] (remove_proc_entry+0x14c/0x16c)
[ 31.613709] r3:806a79c0 r2:808229a0
[ 31.617371] [<80142b3c>] (remove_proc_entry) from [<80070380>] (unregister_irq_proc+0x94/0xb8)
[ 31.625989] r10:00000000 r8:8000ede4 r7:80955f2c r6:0000004f r5:8118e738 r4:be00af00
[ 31.633952] [<800702ec>] (unregister_irq_proc) from [<80069dac>] (free_desc+0x2c/0x64)
[ 31.641898] r6:0000004f r5:80955f38 r4:be00af00
[ 31.646604] [<80069d80>] (free_desc) from [<80069e68>] (irq_free_descs+0x4c/0x8c)
[ 31.654092] r7:00000081 r6:00000001 r5:0000004f r4:00000001
[ 31.659863] [<80069e1c>] (irq_free_descs) from [<8006fc3c>] (irq_dispose_mapping+0x40/0x5c)
[ 31.668247] r6:be17b844 r5:be17b800 r4:0000004f r3:802c5ec0
[ 31.673998] [<8006fbfc>] (irq_dispose_mapping) from [<7f004ea4>] (fsl_ssi_remove+0x58/0x70 [snd_so)
[ 31.683948] r4:bb5bba10 r3:00000001
[ 31.687618] [<7f004e4c>] (fsl_ssi_remove [snd_soc_fsl_ssi]) from [<803720a0>] (platform_drv_remove)
[ 31.697564] r5:7f0064f8 r4:be17b810
[ 31.701195] [<80372080>] (platform_drv_remove) from [<80370494>] (__device_release_driver+0x78/0xc)
[ 31.710361] r5:7f0064f8 r4:be17b810
[ 31.713987] [<8037041c>] (__device_release_driver) from [<80370d20>] (driver_detach+0xbc/0xc0)
[ 31.722631] r5:7f0064f8 r4:be17b810
[ 31.726259] [<80370c64>] (driver_detach) from [<80370304>] (bus_remove_driver+0x54/0x98)
[ 31.734382] r6:00000800 r5:00000000 r4:7f0064f8 r3:bb67f500
[ 31.740149] [<803702b0>] (bus_remove_driver) from [<80371398>] (driver_unregister+0x30/0x50)
[ 31.748617] r4:7f0064f8 r3:bd9f7080
[ 31.752245] [<80371368>] (driver_unregister) from [<80371f3c>] (platform_driver_unregister+0x14/0x)
[ 31.761498] r4:7f00655c r3:7f005a70
[ 31.765130] [<80371f28>] (platform_driver_unregister) from [<7f005a84>] (fsl_ssi_driver_exit+0x14/)
[ 31.776147] [<7f005a70>] (fsl_ssi_driver_exit [snd_soc_fsl_ssi]) from [<8008ed80>] (SyS_delete_mod)
[ 31.786553] [<8008ec64>] (SyS_delete_module) from [<8000ec20>] (ret_fast_syscall+0x0/0x48)
[ 31.794824] r6:00c46d18 r5:00000800 r4:00c46d18
[ 31.799530] ---[ end trace 954e8a3a15379e52 ]---
Moreover replace devm_request_irq() with request_irq() since there is
no need to use it as now driver always frees IRQ manually.
devm_request_irq() is used by other drivers too, this should not be a
problem. Looking at the code it seems that irq_dispose_mapping may not
be necessary with devm_request_irq(). So I think it would be better to
remove irq_dispose_mapping() instead.
Best Regards,
Markus Pargmann
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 12/01/2014 07:50 AM, Markus Pargmann wrote:
[...]
devm_request_irq() is used by other drivers too, this should not be a
problem. Looking at the code it seems that irq_dispose_mapping may not
be necessary with devm_request_irq(). So I think it would be better to
remove irq_dispose_mapping() instead.
The driver creates the mapping by calling irq_of_parse_and_map(), so it also
has to dispose the mapping. But the easy way out is to simply use
platform_get_irq() instead of irq_of_parse_map(). In this case the mapping
is not managed by the device but by the of core, so the device has not to
dispose the mapping.
- Lars
From: Timur Tabi <hidden> Date: 2014-12-01 16:52:03
On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:
The driver creates the mapping by calling irq_of_parse_and_map(), so it
also has to dispose the mapping. But the easy way out is to simply use
platform_get_irq() instead of irq_of_parse_map(). In this case the
mapping is not managed by the device but by the of core, so the device
has not to dispose the mapping.
Is this a problem unique to the SSI driver? Maybe devm_free_irq()
should also dispose of the mapping?
The driver creates the mapping by calling irq_of_parse_and_map(), so it
also has to dispose the mapping. But the easy way out is to simply use
platform_get_irq() instead of irq_of_parse_map(). In this case the
mapping is not managed by the device but by the of core, so the device
has not to dispose the mapping.
Is this a problem unique to the SSI driver? Maybe devm_free_irq() should
also dispose of the mapping?
If the mapping was not created by the device, the device shouldn't dispose
it. Mapping and requesting the interrupt are two independent operations.
From: Timur Tabi <hidden> Date: 2014-12-01 18:48:58
On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:
The driver creates the mapping by calling irq_of_parse_and_map(), so it
also has to dispose the mapping.
I agree with Markus, this does seem weird. It sounds like you're saying
that irq_of_parse_and_map() and devm_request_irq() are incompatible. A
quick grep shows the following drivers that call both functions:
ata/pata_mpc52xx.c
built-in.o
cpufreq/exynos5440-cpufreq.c
crypto/omap-sham.c
dma/moxart-dma.c
edac/mpc85xx_edac.c
hsi/clients/nokia-modem.c
i2c/busses/i2c-wmt.c
input/serio/apbps2.c
mmc/host/omap_hsmmc.c
mmc/host/moxart-mmc.c
mtd/nand/mpc5121_nfc.c
net/ethernet/arc/emac_main.c
net/ethernet/moxa/moxart_ether.c
pci/host/pcie-rcar.c
pinctrl/samsung/pinctrl-exynos5440.c
pinctrl/samsung/pinctrl-exynos.c
pinctrl/pinctrl-bcm2835.c
spi/spi-bcm2835.c
spi/spi-mpc512x-psc.c
staging/xillybus/xillybus_of.c
thermal/samsung/exynos_tmu.c
From: Mark Brown <broonie@kernel.org> Date: 2014-12-01 19:24:48
On Mon, Dec 01, 2014 at 05:49:56PM +0100, Lars-Peter Clausen wrote:
On 12/01/2014 07:50 AM, Markus Pargmann wrote:
quoted
devm_request_irq() is used by other drivers too, this should not be a
problem. Looking at the code it seems that irq_dispose_mapping may not
be necessary with devm_request_irq(). So I think it would be better to
remove irq_dispose_mapping() instead.
The driver creates the mapping by calling irq_of_parse_and_map(), so it also
has to dispose the mapping. But the easy way out is to simply use
platform_get_irq() instead of irq_of_parse_map(). In this case the mapping
is not managed by the device but by the of core, so the device has not to
dispose the mapping.
It also has the advantage of not being DT specific so providing some
chance that future firmware interfaces can be supported without driver
modification.
The driver creates the mapping by calling irq_of_parse_and_map(), so it
also has to dispose the mapping.
I agree with Markus, this does seem weird. It sounds like you're saying
that irq_of_parse_and_map() and devm_request_irq() are incompatible.
They probably are. You have to create the mapping before you request the IRQ
and if devm_request_irq() is used the IRQ is only freed again after the
remove function of the driver been called. Yet if a driver creates a mapping
in its probe function it should also dispose it in its remove function. So
you are stuck with either freeing the mapping before freeing the IRQ or
leaking the mapping.
My opinion on this is that devices should not create mappings and should
leave that to the core. This quite easily solves the dilemma.
A quick grep shows the following drivers that call both functions:
Most of these drivers will probably work fine without irq_of_parse_and_map().
On Monday 01 December 2014 19:41:47 Mark Brown wrote:
On Mon, Dec 01, 2014 at 08:39:51PM +0100, Lars-Peter Clausen wrote:
quoted
On 12/01/2014 07:48 PM, Timur Tabi wrote:
quoted
quoted
A quick grep shows the following drivers that call both functions:
quoted
Most of these drivers will probably work fine without irq_of_parse_and_map().
I'd also note that quite a few of these drivers look pretty legacy - a
very large proportion are for old PowerPC hardware, though by no means
all.
Right, from the times before we were using platform_device for probing
device tree based devices and they had to map the interrupt themselves.
Some of them like arch/powerpc/sysdev/fsl_pci.c seem fine, this one
is does not expect to ever destroy a device, and it only unmaps the
interrupt if request_irq fails. drivers/ata/pata_mpc52xx.c on the
other hand seems wrong in the same was as drivers/edac/mpc85xx_edac.c
and sound/soc/fsl/fsl_ssi.c.
All other drivers that call irq_of_parse_and_map and pass that into
devm_request_irq just never unmap, and their interrupts are already
mapped by the platform code, so I think it's not even a leak.
Arnd
From: Timur Tabi <hidden> Date: 2014-12-01 19:59:31
On 12/01/2014 01:56 PM, Arnd Bergmann wrote:
All other drivers that call irq_of_parse_and_map and pass that into
devm_request_irq just never unmap, and their interrupts are already
mapped by the platform code, so I think it's not even a leak.
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
On Monday 01 December 2014 13:59:27 Timur Tabi wrote:
On 12/01/2014 01:56 PM, Arnd Bergmann wrote:
quoted
All other drivers that call irq_of_parse_and_map and pass that into
devm_request_irq just never unmap, and their interrupts are already
mapped by the platform code, so I think it's not even a leak.
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
From: Timur Tabi <hidden> Date: 2014-12-01 20:11:43
On 12/01/2014 02:01 PM, Arnd Bergmann wrote:
quoted
quoted
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
platform_get_irq()
Ok, but that function also calls irq_create_of_mapping(). So it still
appears that the only way to get the IRQ is to map it, but then we can't
use devm_request_irq().
From: Mark Brown <broonie@kernel.org> Date: 2014-12-01 20:17:01
On Mon, Dec 01, 2014 at 09:01:43PM +0100, Arnd Bergmann wrote:
On Monday 01 December 2014 13:59:27 Timur Tabi wrote:
quoted
On 12/01/2014 01:56 PM, Arnd Bergmann wrote:
quoted
quoted
All other drivers that call irq_of_parse_and_map and pass that into
devm_request_irq just never unmap, and their interrupts are already
mapped by the platform code, so I think it's not even a leak.
quoted
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
platform_get_irq()
Right, and just to emphasize what we were saying earlier the code was
fine when originally written - both mapping inside platform_get_irq()
and devm_ came along quite a while after the driver was originally
written.
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
platform_get_irq()
Ok, but that function also calls irq_create_of_mapping(). So it still
appears that the only way to get the IRQ is to map it, but then we can't use
devm_request_irq().
Hm... that's new. But it's not really a driver issue anymore if it is done
in the core. So I guess for now just use platform_get_irq() and ignore the
other issue.
On Mon, Dec 1, 2014 at 6:30 PM, Lars-Peter Clausen [off-list ref] wrote:
On 12/01/2014 09:11 PM, Timur Tabi wrote:
quoted
On 12/01/2014 02:01 PM, Arnd Bergmann wrote:
quoted
quoted
quoted
Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map? How else should it get the IRQ?
platform_get_irq()
Ok, but that function also calls irq_create_of_mapping(). So it still
appears that the only way to get the IRQ is to map it, but then we can't
use
devm_request_irq().
Hm... that's new. But it's not really a driver issue anymore if it is done
in the core. So I guess for now just use platform_get_irq() and ignore the
other issue.
With the suggested changes below, the removal of the driver works fine on a mx6:
root@freescale /$ modprobe snd-soc-fsl-ssi
root@freescale /$ modprobe snd-soc-imx-wm8962
[ 319.517679] input: WM8962 Beep Generator as
/devices/soc0/soc/2100000.aips-bus/21a0000.i2c/i2c-0/0-001a/input/input7
[ 319.543225] imx-wm8962 sound: wm8962 <-> 202c000.ssi mapping ok
root@freescale /$ rmmod snd-soc-imx-wm8962
root@freescale /$ rmmod snd-soc-fsl-ssi
sound/soc/fsl/fsl_ssi.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
From: Timur Tabi <hidden> Date: 2014-12-01 20:42:51
On 12/01/2014 02:40 PM, Fabio Estevam wrote:
quoted
quoted
Hm... that's new. But it's not really a driver issue anymore if it is done
in the core. So I guess for now just use platform_get_irq() and ignore the
other issue.
With the suggested changes below, the removal of the driver works fine on a mx6:
Would the mapping continue to exist after the driver is unloaded? Can
you try multiple loads/unloads and see if interrupts still work?
On Mon, Dec 1, 2014 at 6:42 PM, Timur Tabi [off-list ref] wrote:
Would the mapping continue to exist after the driver is unloaded? Can you
try multiple loads/unloads and see if interrupts still work?
I tried multiple loads/unloads and audio works fine with those changes.
About the ssi irq we have:
- With the ssi driver loaded:
root@freescale /home$ cat /proc/interrupts | grep ssi
79: 0 0 0 0 GIC 79 202c000.ssi
- After removing the ssi driver:
root@freescale /home$ rmmod snd-soc-fsl-ssi
root@freescale /home$ cat /proc/interrupts | grep ssi
root@freescale /home$
,so it seems to behave properly.