[BUG] irq_dispose_mapping after irq request failure

[BUG] irq_dispose_mapping after irq request failure

[Baruch Siach]

Hi lkml,

The drivers/edac/mpc85xx_edac.c driver contains the following (abbreviated)
code snippet it its .probe:

		res = devm_request_irq(&op->dev, pdata->irq,
				       mpc85xx_pci_isr, IRQF_DISABLED,
				       "[EDAC] PCI err", pci);
		if (res < 0) {
			irq_dispose_mapping(pdata->irq);
			goto err2;
		}

Now, since the requested irq is already in use, and IRQF_SHARED is not set,
devm_request_irq errors() out, which is OK. Less OK is the
irq_dispose_mapping() call, which gives me this:

EDAC PCI1: Giving out device to module 'MPC85xx_edac' controller 'mpc85xx_pci_err': DEV 'ffe0a000.pcie' (INTERRUPT)
genirq: Flags mismatch irq 16. 00000020 ([EDAC] PCI err) vs. 00000020 ([EDAC] PCI err)
mpc85xx_pci_err_probe: Unable to requiest irq 16 for MPC85xx PCI err
remove_proc_entry: removing non-empty directory 'irq/16', leaking at least '[EDAC] PCI err'
------------[ cut here ]------------
WARNING: at fs/proc/generic.c:842
NIP: c00cd00c LR: c00cd00c CTR: c000c5e4
REGS: cf039b80 TRAP: 0700   Not tainted  (3.8.0-rc7-00002-g37ddebf)
MSR: 00029000 <CE,EE,ME>  CR: 42042422  XER: 00000000
TASK = cf034000[1] 'swapper' THREAD: cf038000
GPR00: c00cd00c cf039c30 cf034000 0000005b 0000005c 0000005c c04b7dde 435d2050 
GPR08: 43492065 c04a9a44 00000000 cf039bf0 22042424 00000000 c00025d0 00000000 
GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 c042fe78 
GPR24: 00000000 00000000 c04c3f90 cf05294c 00100100 00200200 cf039c78 cf052900 
NIP [c00cd00c] remove_proc_entry+0x190/0x1bc
LR [c00cd00c] remove_proc_entry+0x190/0x1bc
Call Trace:
[cf039c30] [c00cd00c] remove_proc_entry+0x190/0x1bc (unreliable)
[cf039c70] [c0058c64] unregister_irq_proc+0x6c/0x74
[cf039c90] [c0054530] free_desc+0x34/0x68
[cf039cb0] [c00545f0] irq_free_descs+0x44/0x88
[cf039cd0] [c00585c8] irq_dispose_mapping+0x68/0x70
[cf039ce0] [c0222650] mpc85xx_pci_err_probe+0x2a8/0x308
[cf039d20] [c0014f8c] fsl_pci_probe+0x74/0x80
[cf039d30] [c01a9c48] platform_drv_probe+0x20/0x30
[cf039d40] [c01a88c4] driver_probe_device+0xcc/0x1f4
[cf039d60] [c01a7288] bus_for_each_drv+0x60/0x9c
[cf039d90] [c01a85ac] device_attach+0x78/0x90
[cf039db0] [c01a7430] bus_probe_device+0x34/0x9c
[cf039dd0] [c01a55c4] device_add+0x410/0x580
[cf039e10] [c022eef4] of_device_add+0x40/0x50
[cf039e20] [c022f550] of_platform_device_create_pdata+0x6c/0x8c
[cf039e40] [c022f658] of_platform_bus_create+0xe8/0x178
[cf039e90] [c022f7a0] of_platform_bus_probe+0xac/0xdc
[cf039eb0] [c0415488] mpc85xx_common_publish_devices+0x20/0x30
[cf039ec0] [c0415578] __machine_initcall_p1020_rdb_mpc85xx_common_publish_devices+0x2c/0x3c
[cf039ed0] [c040e83c] do_one_initcall+0xdc/0x1b4
[cf039f00] [c040ea24] kernel_init_freeable+0x110/0x1a8
[cf039f30] [c00025e8] kernel_init+0x18/0xf8
[cf039f40] [c000b868] ret_from_kernel_thread+0x64/0x6c
Instruction dump:
2f870000 41be0030 80bf002c 3c80c033 3c60c03e 38846be0 38840260 38a50055 
38df0055 38e70055 38631770 48260331 <0fe00000> 7fe3fb78 4bfffb41 48000018 
---[ end trace 9af370ce0e147530 ]---

So, is irq_dispose_mapping() the right thing to do when irq request fails?

A simple grep shows that irq_dispose_mapping() calls are mostly limited to
powerpc code. Is there a reason for that?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] irq_dispose_mapping after irq request failure

[Michael Ellerman]

On Mon, Feb 11, 2013 at 07:31:00AM +0200, Baruch Siach wrote:
> Hi lkml,

Hi Baruch,

> The drivers/edac/mpc85xx_edac.c driver contains the following (abbreviated)
> code snippet it its .probe:

You dropped an important detail which is the preceeding line:

	pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);

> 		res = devm_request_irq(&op->dev, pdata->irq,
> 				       mpc85xx_pci_isr, IRQF_DISABLED,
> 				       "[EDAC] PCI err", pci);
> 		if (res < 0) {
> 			irq_dispose_mapping(pdata->irq);
> 			goto err2;
> 		}
> 
> Now, since the requested irq is already in use, and IRQF_SHARED is not set,
> devm_request_irq errors() out, which is OK. Less OK is the
> irq_dispose_mapping() call, which gives me this:
> 
> EDAC PCI1: Giving out device to module 'MPC85xx_edac' controller 'mpc85xx_pci_err': DEV 'ffe0a000.pcie' (INTERRUPT)
> genirq: Flags mismatch irq 16. 00000020 ([EDAC] PCI err) vs. 00000020 ([EDAC] PCI err)

The hint here is to notice which other irq you're clashing with          ^^
ie. yourself. Which is odd, that is the root of the problem.

The badness you're getting from irq_dispose_mapping() is caused because you're
disposing of that mapping which is currently still in use, by the same interrupt.

That is caused by a "feature" in the irq mapping code, where if you ask to map an
already mapped hwirq, it will give you back the same virq. So in your case when
you called irq_of_parse_and_map() it noticed that someone had already mapped
that hwirq, and gave you back an existing (in use) virq.

> mpc85xx_pci_err_probe: Unable to requiest irq 16 for MPC85xx PCI err
                                       ^
While you're there, can you fix the typo :)


> So, is irq_dispose_mapping() the right thing to do when irq request fails?

It's the right thing to do to undo the effect of irq_create_mapping(), or in your case irq_of_parse_and_map().

It just falls down in this case, because you're inadvertently disposing of something that's still in use.

> A simple grep shows that irq_dispose_mapping() calls are mostly limited to
> powerpc code. Is there a reason for that?

That's because the irq domain code began life as powerpc specific code. It's now become generic and will start to appear in more places.

cheers

Re: [BUG] irq_dispose_mapping after irq request failure

[Baruch Siach]

Hi Michael,

On Mon, Feb 11, 2013 at 05:19:49PM +1100, Michael Ellerman wrote:
> On Mon, Feb 11, 2013 at 07:31:00AM +0200, Baruch Siach wrote:

[...]

> > mpc85xx_pci_err_probe: Unable to requiest irq 16 for MPC85xx PCI err
> While you're there, can you fix the typo :)

The patch fixing it is already queued at 
http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commitdiff;h=e7d2c215e56dc9fa0a01e26f2acfc3d73c889ba3.

Thanks for your details explanation. I'll now try to figure out what's wrong 
with my device tree.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] irq_dispose_mapping after irq request failure

[Grant Likely]

On Mon, 11 Feb 2013 17:19:49 +1100, Michael Ellerman <michael@ellerman.id.au> wrote:
> On Mon, Feb 11, 2013 at 07:31:00AM +0200, Baruch Siach wrote:
> > Hi lkml,
> 
> Hi Baruch,
> 
> > The drivers/edac/mpc85xx_edac.c driver contains the following (abbreviated)
> > code snippet it its .probe:
> 
> You dropped an important detail which is the preceeding line:
> 
> 	pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);
> 
> > 		res = devm_request_irq(&op->dev, pdata->irq,
> > 				       mpc85xx_pci_isr, IRQF_DISABLED,
> > 				       "[EDAC] PCI err", pci);
> > 		if (res < 0) {
> > 			irq_dispose_mapping(pdata->irq);
> > 			goto err2;
> > 		}
> > 
> > Now, since the requested irq is already in use, and IRQF_SHARED is not set,
> > devm_request_irq errors() out, which is OK. Less OK is the
> > irq_dispose_mapping() call, which gives me this:
> > 
> > EDAC PCI1: Giving out device to module 'MPC85xx_edac' controller 'mpc85xx_pci_err': DEV 'ffe0a000.pcie' (INTERRUPT)
> > genirq: Flags mismatch irq 16. 00000020 ([EDAC] PCI err) vs. 00000020 ([EDAC] PCI err)
> 
> The hint here is to notice which other irq you're clashing with          ^^
> ie. yourself. Which is odd, that is the root of the problem.
> 
> The badness you're getting from irq_dispose_mapping() is caused because you're
> disposing of that mapping which is currently still in use, by the same interrupt.
> 
> That is caused by a "feature" in the irq mapping code, where if you ask to map an
> already mapped hwirq, it will give you back the same virq. So in your case when
> you called irq_of_parse_and_map() it noticed that someone had already mapped
> that hwirq, and gave you back an existing (in use) virq.

Really the irq mappings should be using reference counting. The existing
code is naive on this count and just releases the irq on the first call
to irq_dispose_mapping().  I've not gotten around to fixing that. Anyone
want to take that task on?

g.

Re: [BUG] irq_dispose_mapping after irq request failure

[Benjamin Herrenschmidt]

On Mon, 2013-02-11 at 20:52 +0000, Grant Likely wrote:
> Really the irq mappings should be using reference counting. The existing
> code is naive on this count and just releases the irq on the first call
> to irq_dispose_mapping().  I've not gotten around to fixing that. Anyone
> want to take that task on?

Is this the best approach ?

The original idea was that there was no point disposing of mappings in most
cases and keeping the mapping around would provide a bit of stability of
interrupt numbers which might come in handy for debugging etc...

The few cases where disposing of a mapping might be useful is if the underlying
physical interrupts completely disappear, as in a cascaded controller gets
removed or that sort of thing, which is a very rare case... And even then...

Could you just make irq_dispose_mapping() check if the irq desc is
active and fail/WARN/BUG if it is ? I don't see the point of adding a refcount,
that feels overkill.

Cheers,
Ben.

Re: [BUG] irq_dispose_mapping after irq request failure

[Michael Ellerman]

On Tue, Feb 12, 2013 at 11:51:13AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2013-02-11 at 20:52 +0000, Grant Likely wrote:
> > Really the irq mappings should be using reference counting. The existing
> > code is naive on this count and just releases the irq on the first call
> > to irq_dispose_mapping().  I've not gotten around to fixing that. Anyone
> > want to take that task on?
> 
> Is this the best approach ?
> 
> The original idea was that there was no point disposing of mappings in most
> cases and keeping the mapping around would provide a bit of stability of
> interrupt numbers which might come in handy for debugging etc...
> 
> The few cases where disposing of a mapping might be useful is if the underlying
> physical interrupts completely disappear, as in a cascaded controller gets
> removed or that sort of thing, which is a very rare case... And even then...

That may have been the intent, but we forgot to tell driver writers, ourselves
included.

> Could you just make irq_dispose_mapping() check if the irq desc is
> active and fail/WARN/BUG if it is ? I don't see the point of adding a refcount,
> that feels overkill.

I don't think you can, "active" is not well defined. Other code may have
done nothing other than create the mapping and remembered the virq,
which will break if you destroy the mapping. Or?

I agree refcounting is not fun. It'll end up with the same mess as
of_node_get/put() where practically every 2nd piece of code leaks
references.

I guess we can't go the other way, and say that mapping the same hwirq
twice is an error.

cheers

Re: [BUG] irq_dispose_mapping after irq request failure

[Benjamin Herrenschmidt]

On Tue, 2013-02-12 at 17:18 +1100, Michael Ellerman wrote:
> 
> I don't think you can, "active" is not well defined. Other code may have
> done nothing other than create the mapping and remembered the virq,
> which will break if you destroy the mapping. Or?

Active as in "requested". Yes there's a potential problems with multiple
requests for mappings & shared interrupts. This is not a problem for PCI
on powerpc because we don't free those mappings afaik.

> I agree refcounting is not fun. It'll end up with the same mess as
> of_node_get/put() where practically every 2nd piece of code leaks
> references.
> 
> I guess we can't go the other way, and say that mapping the same hwirq
> twice is an error.

Might be worth it, and force the sharing case to be handled at some kind
of upper level (bus or platform).

Ben.