Re: [PATCH] of/irq: Consider device address size in interrupt map walk

[Stefan Wiehler]

> You've missed a bunch of people/lists. Use get_maintainers.pl.

Sorry, indeed, did not think about about PCI...

> Can you provide some details on what these nodes look like. The
> interrupt provider to an SoC device is a PCI device? That's weird...

The DTO looks like this:

watchdog {
	...
	reg = <0x00002064 0x00000028>;
	...
	interrupt-parent = <&gpio_17_0>;
	interrupts = <4 0x8>; // 8 -> IRQ_TYPE_LEVEL_LOW
	...
};

And the base DT:

ecam0: pci@878000000000 {
	...
	#size-cells = <2>;
	#address-cells = <3>;
	...
	gpio_17_0: gpio0@17,0 {
		...
		reg = <0x8800 0 0 0 0>; /*  DEVFN = 0x88 (17:0) */
		...
	};
	...
};

I completely agree it's a bit sketchy, but it's not me who came up with
this ;-) Nevertheless I think other people might run into this issue of
mismatching address sizes as well when no interrupt mapping was intended.

> Note that of_irq_parse_raw() was refactored in 6.10, so your patch is
> not going to apply.

I'm aware of that and have adapted the patch accordingly.

> That's not the right information to parse the address correctly. You
> would need the device's #address-cells. However, in most cases we
> don't really need to parse the address. The address is not really used
> except in cases where interrupt routing matches the bus and so there
> is only one size. That's effectively only PCI devices today. In that
> case, the address size would always be equal, and the code implicitly
> assumes that. It would be better if we could just detect when to use
> the address or not. I think we'd have to look at 'interrupt-map-mask'
> first to see whether or not to read the address cells. Or maybe we
> could detect when the interrupt parent is the device's parent node.
> Either way, this code is tricky and hard to change without breaking
> something.

Thanks for confirming that it's PCI only and no address size mismatch
should occur. I also was thinking in the direction of checking first if
interrupt mapping is intended and return early otherwise, but was
worried to break something along the way...

> A simpler way to fix this is just always pass in an address buffer of
> 3 cells to of_irq_parse_raw. You would just need to copy the cells in
> of_irq_parse_one() to a temporary buffer.

That indeed sounds like the easiest solution to me; I'll send a new patch
shortly. However I don't understand how you came up with an address
buffer size of 3 - shouldn't it be MAX_PHANDLE_ARGS
(addrsize = MAX_PHANDLE_ARGS and intsize = 0 in the worst case)?

> Please don't send new versions right after the prior version. Give
> people a chance to review.

Sorry about that, I wanted to fix checkpatch because I thought
patch-applied failed due to that, but that seems to be another
issue...

Kind regards,

Stefan