I2C OF IRQ parsing issue due to probe ordering

I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hello,

I recently ran into an issue with the OF IRQ parsing code in the I2C core
(of_i2c_register_devices in drivers/i2c/i2c-core.c).

My DT contains the following nodes.

        gpio1: gpio@e6051000 {
                ...
                #interrupt-cells = <2>;
                interrupt-controller;
                clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
        };

        iic2: i2c@e6520000 {
                #address-cells = <1>;
                #size-cells = <0>;
                ...
                hdmi@39 {
                        compatible = "adi,adv7511w";
                        reg = <0x39>;
                        interrupt-parent = <&gpio1>;
                        interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
                        ...
                };
        };

        mstp9_clks: mstp9_clks@e6150994 {
                ...
        };

The i2c@e6520000 node is probed before the gpio@e6051000 node. The
of_i2c_register_devices() function tries to register all children, including
hdmi@39. It tries to parse and map the I2C client IRQ by calling
irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
probed yet. The adv7511 driver later probes the hdmi@39 device and gets
client->irq set to 0.

We can't control the probe order. One option to solve the problem would be to
defer probing of the I2C client until the interrupt control is available. The
following patch is a naive implementation. Before turning it into a real
upstream candidate (changing the return type of irq_create_of_mapping without
checking the callers is a no-go) I'd like to get feedback on the approach.

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6a7f79..73ff1e7d2382 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -631,6 +631,14 @@ static int i2c_device_probe(struct device *dev)
 	if (!client)
 		return 0;
 
+	if (!client->irq && dev->of_node) {
+		int irq = irq_of_parse_and_map(dev->of_node, 0);
+		if (irq < 0)
+			return irq;
+
+		client->irq = irq;
+	}
+
 	driver = to_i2c_driver(dev->driver);
 	if (!driver->probe || !driver->id_table)
 		return -ENODEV;
@@ -1409,7 +1417,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 			continue;
 		}
 
-		info.irq = irq_of_parse_and_map(node, 0);
 		info.of_node = of_node_get(node);
 		info.archdata = &dev_ad;
 
@@ -1423,7 +1430,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 			dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
 				node->full_name);
 			of_node_put(node);
-			irq_dispose_mapping(info.irq);
 			continue;
 		}
 	}
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index bfec136a6d1e..81a14d89a5bd 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -34,7 +34,7 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index,
 extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
 extern int of_irq_parse_one(struct device_node *device, int index,
 			  struct of_phandle_args *out_irq);
-extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
+extern int irq_create_of_mapping(struct of_phandle_args *irq_data);
 extern int of_irq_to_resource(struct device_node *dev, int index,
 			      struct resource *r);
 extern int of_irq_to_resource_table(struct device_node *dev,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 6534ff6ce02e..386fd09f4c85 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -466,7 +466,7 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
 }
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);
 
-unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
+int irq_create_of_mapping(struct of_phandle_args *irq_data)
 {
 	struct irq_domain *domain;
 	irq_hw_number_t hwirq;
@@ -477,7 +477,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
 	if (!domain) {
 		pr_warn("no irq domain found for %s !\n",
 			of_node_full_name(irq_data->np));
-		return 0;
+		return -EPROBE_DEFER;
 	}
 
 	/* If domain has no translation, then we assume interrupt line */

-- 
Regards,

Laurent Pinchart

Re: I2C OF IRQ parsing issue due to probe ordering

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]


> The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> of_i2c_register_devices() function tries to register all children, including
> hdmi@39. It tries to parse and map the I2C client IRQ by calling
> irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
> probed yet. The adv7511 driver later probes the hdmi@39 device and gets
> client->irq set to 0.

I've got this strange feeling of deja vu... Ah, here: Thierry Reding
tackled this problem a year ago. His series:

https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
resolution)

He did a V2 (which never made it to the i2c list). Seems like the first
two patches made it and the rest got stalled without discussion?

https://lkml.org/lkml/2013/9/18/216

Adding Thierry to the queue. Maybe he can bring some light to what
happened to his series.

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: I2C OF IRQ parsing issue due to probe ordering

[Ezequiel Garcia]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

Hi Laurent,

On 10/25/2014 07:13 PM, Laurent Pinchart wrote:
> Hello,
> 
> I recently ran into an issue with the OF IRQ parsing code in the I2C core
> (of_i2c_register_devices in drivers/i2c/i2c-core.c).
> 
> My DT contains the following nodes.
> 
>         gpio1: gpio@e6051000 {
>                 ...
>                 #interrupt-cells = <2>;
>                 interrupt-controller;
>                 clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
>         };
> 
>         iic2: i2c@e6520000 {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 ...
>                 hdmi@39 {
>                         compatible = "adi,adv7511w";
>                         reg = <0x39>;
>                         interrupt-parent = <&gpio1>;
>                         interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
>                         ...
>                 };
>         };
> 
>         mstp9_clks: mstp9_clks@e6150994 {
>                 ...
>         };
> 
> The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> of_i2c_register_devices() function tries to register all children, including
> hdmi@39. It tries to parse and map the I2C client IRQ by calling
> irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
> probed yet. The adv7511 driver later probes the hdmi@39 device and gets
> client->irq set to 0.
> 
> We can't control the probe order.

Maybe I'm missing something, but I think your i2c adapter is probed with
a subsys_initcall (as many other adapters). Otherwise, I can't see why
it would be probed before the the gpio controller.

I think this initcall is your problem. Have you tried just using
platform_driver's probe?

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Ezequiel,

On Thursday 30 October 2014 08:58:28 Ezequiel Garcia wrote:
> On 10/25/2014 07:13 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > I recently ran into an issue with the OF IRQ parsing code in the I2C core
> > (of_i2c_register_devices in drivers/i2c/i2c-core.c).
> > 
> > My DT contains the following nodes.
> > 
> >         gpio1: gpio@e6051000 {
> >                 ...
> >                 #interrupt-cells = <2>;
> >                 interrupt-controller;
> >                 clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
> >         };
> >         
> >         iic2: i2c@e6520000 {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >                 ...
> >                 hdmi@39 {
> >                         compatible = "adi,adv7511w";
> >                         reg = <0x39>;
> >                         interrupt-parent = <&gpio1>;
> >                         interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> >                         ...
> >                 };
> >         };
> >         
> >         mstp9_clks: mstp9_clks@e6150994 {
> >                 ...
> >         };
> > 
> > The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> > of_i2c_register_devices() function tries to register all children,
> > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > device and gets client->irq set to 0.
> > 
> > We can't control the probe order.
> 
> Maybe I'm missing something, but I think your i2c adapter is probed with
> a subsys_initcall (as many other adapters). Otherwise, I can't see why
> it would be probed before the the gpio controller.
> 
> I think this initcall is your problem. Have you tried just using
> platform_driver's probe?

My I2C controller driver uses module_platform_driver(). The reason why the 
GPIO controller is probed later is because the GPIO requires a clock, and the 
clock device is probed after the I2C controller, resulting in a deferred 
probing the first time the GPIO controller is probed.

In the general case probe ordering can't be controlled, especially with DT. I 
believe we thus need a generic solution.

-- 
Regards,

Laurent Pinchart

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Wolfram and Thierry,

On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
> > The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> > of_i2c_register_devices() function tries to register all children,
> > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > device and gets client->irq set to 0.
> 
> I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> tackled this problem a year ago. His series:

Thanks for the pointer.

> https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> resolution)
> 
> He did a V2 (which never made it to the i2c list). Seems like the first
> two patches made it and the rest got stalled without discussion?
> 
> https://lkml.org/lkml/2013/9/18/216
> 
> Adding Thierry to the queue. Maybe he can bring some light to what
> happened to his series.

That's exactly what I need :-) Thierry, do you plan to respin the series ?

-- 
Regards,

Laurent Pinchart

Re: I2C OF IRQ parsing issue due to probe ordering

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Mon, Oct 27, 2014 at 01:58:19PM +0100, Wolfram Sang wrote:
> 
> > The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> > of_i2c_register_devices() function tries to register all children, including
> > hdmi@39. It tries to parse and map the I2C client IRQ by calling
> > irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
> > probed yet. The adv7511 driver later probes the hdmi@39 device and gets
> > client->irq set to 0.
> 
> I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> tackled this problem a year ago. His series:
> 
> https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> resolution)
> 
> He did a V2 (which never made it to the i2c list). Seems like the first
> two patches made it and the rest got stalled without discussion?
> 
> https://lkml.org/lkml/2013/9/18/216
> 
> Adding Thierry to the queue. Maybe he can bring some light to what
> happened to his series.

I tried to fix it in a proper way, but it seems people were uneasy with
how invasive the change was. At some point I lost interest. People ended
up merging something that was similar, but side-stepped the issue of
propagating error codes all the way up by introducing a new API and in
case of of_irq_parse_one() failing doing an additional check to see if
the reason was the missing IRQ domain.

See:

	9ec36cafe43b of/irq: do irq resolution in platform_get_irq

I suspect a similar thing could be done for I2C.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: I2C OF IRQ parsing issue due to probe ordering

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On Thu, Oct 30, 2014 at 02:53:42PM +0200, Laurent Pinchart wrote:
> Hi Wolfram and Thierry,
> 
> On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
> > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> > > of_i2c_register_devices() function tries to register all children,
> > > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > > device and gets client->irq set to 0.
> > 
> > I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> > tackled this problem a year ago. His series:
> 
> Thanks for the pointer.
> 
> > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> > resolution)
> > 
> > He did a V2 (which never made it to the i2c list). Seems like the first
> > two patches made it and the rest got stalled without discussion?
> > 
> > https://lkml.org/lkml/2013/9/18/216
> > 
> > Adding Thierry to the queue. Maybe he can bring some light to what
> > happened to his series.
> 
> That's exactly what I need :-) Thierry, do you plan to respin the series ?

Not really. Like I said in my reply to Wolfram, a different set of
patches was merged subsequently to solve this issue for platform
devices, which makes my patchset mostly obsolete. But I think the
solution that you proposed would work well if you do:

-		int irq = irq_of_parse_and_map(dev->of_node, 0);
+		int irq = of_irq_get(dev->of_node, 0);

And then changing irq_create_of_mapping() should no longer be necessary.
I still think it's kind of lame to handle -EPROBE_DEFER specially as was
done in of_irq_get(), but given how worried people were about the more
invasive changes needed to propagate the correct error code all the way
up it seems like that's as good as it's going to get.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Thierry,

On Thursday 30 October 2014 13:56:46 Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 01:58:19PM +0100, Wolfram Sang wrote:
> > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> > > of_i2c_register_devices() function tries to register all children,
> > > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > > controller isn't probed yet. The adv7511 driver later probes the hdmi@39
> > > device and gets client->irq set to 0.
> > 
> > I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> > tackled this problem a year ago. His series:
> > 
> > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> > resolution)
> > 
> > He did a V2 (which never made it to the i2c list). Seems like the first
> > two patches made it and the rest got stalled without discussion?
> > 
> > https://lkml.org/lkml/2013/9/18/216
> > 
> > Adding Thierry to the queue. Maybe he can bring some light to what
> > happened to his series.
> 
> I tried to fix it in a proper way, but it seems people were uneasy with
> how invasive the change was.

It was a bit invasive indeed and I can share the uneasiness, but on the other 
hand there was no real nack. I think I still prefer your approach, but can 
live with something simpler.

> At some point I lost interest. People ended up merging something that was
> similar, but side-stepped the issue of propagating error codes all the way
> up by introducing a new API and in case of of_irq_parse_one() failing doing
> an additional check to see if the reason was the missing IRQ domain.
> 
> See:
> 
> 	9ec36cafe43b of/irq: do irq resolution in platform_get_irq
> 
> I suspect a similar thing could be done for I2C.

That could work. We would need to introduce a new i2c_get_irq() function 
though. Wolfram, would you be fine with that ?

-- 
Regards,

Laurent Pinchart

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Thierry,

On Thursday 30 October 2014 14:02:09 Thierry Reding wrote:
> On Thu, Oct 30, 2014 at 02:53:42PM +0200, Laurent Pinchart wrote:
> > On Monday 27 October 2014 13:58:19 Wolfram Sang wrote:
> > > > The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> > > > of_i2c_register_devices() function tries to register all children,
> > > > including hdmi@39. It tries to parse and map the I2C client IRQ by
> > > > calling irq_of_parse_and_map(), which returns 0 as the interrupt
> > > > controller isn't probed yet. The adv7511 driver later probes the
> > > > hdmi@39
> > > > device and gets client->irq set to 0.
> > > 
> > > I've got this strange feeling of deja vu... Ah, here: Thierry Reding
> > 
> > > tackled this problem a year ago. His series:
> > Thanks for the pointer.
> > 
> > > https://lkml.org/lkml/2013/9/16/111 (of/irq: Defer interrupt reference
> > > resolution)
> > > 
> > > He did a V2 (which never made it to the i2c list). Seems like the first
> > > two patches made it and the rest got stalled without discussion?
> > > 
> > > https://lkml.org/lkml/2013/9/18/216
> > > 
> > > Adding Thierry to the queue. Maybe he can bring some light to what
> > > happened to his series.
> > 
> > That's exactly what I need :-) Thierry, do you plan to respin the series ?
> 
> Not really. Like I said in my reply to Wolfram, a different set of
> patches was merged subsequently to solve this issue for platform
> devices, which makes my patchset mostly obsolete. But I think the
> solution that you proposed would work well if you do:
> 
> -		int irq = irq_of_parse_and_map(dev->of_node, 0);
> +		int irq = of_irq_get(dev->of_node, 0);
> 
> And then changing irq_create_of_mapping() should no longer be necessary.

It looks like it would work, yes. I'll test it and will resubmit my patch.

> I still think it's kind of lame to handle -EPROBE_DEFER specially as was
> done in of_irq_get(), but given how worried people were about the more
> invasive changes needed to propagate the correct error code all the way
> up it seems like that's as good as it's going to get.

-- 
Regards,

Laurent Pinchart

Re: I2C OF IRQ parsing issue due to probe ordering

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]


> > See:
> > 
> > 	9ec36cafe43b of/irq: do irq resolution in platform_get_irq
> > 
> > I suspect a similar thing could be done for I2C.
> 
> That could work. We would need to introduce a new i2c_get_irq() function 
> though. Wolfram, would you be fine with that ?

I'd think it will look pretty similar to platform_get_irq, no? That is
fine with me.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: I2C OF IRQ parsing issue due to probe ordering

[Laurent Pinchart]

Hi Wolfram,

On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote:
> > > See:
> > > 	9ec36cafe43b of/irq: do irq resolution in platform_get_irq
> > > 
> > > I suspect a similar thing could be done for I2C.
> > 
> > That could work. We would need to introduce a new i2c_get_irq() function
> > though. Wolfram, would you be fine with that ?
> 
> I'd think it will look pretty similar to platform_get_irq, no? That is
> fine with me.

It would, but as Thierry pointed out it should be possible to hide the details 
in the I2C core. I'll submit a patch shortly.

-- 
Regards,

Laurent Pinchart

Re: I2C OF IRQ parsing issue due to probe ordering

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On Thu, Oct 30, 2014 at 03:22:49PM +0200, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> On Thursday 30 October 2014 14:21:36 Wolfram Sang wrote:
> > > > See:
> > > > 	9ec36cafe43b of/irq: do irq resolution in platform_get_irq
> > > > 
> > > > I suspect a similar thing could be done for I2C.
> > > 
> > > That could work. We would need to introduce a new i2c_get_irq() function
> > > though. Wolfram, would you be fine with that ?
> > 
> > I'd think it will look pretty similar to platform_get_irq, no? That is
> > fine with me.
> 
> It would, but as Thierry pointed out it should be possible to hide the details 
> in the I2C core. I'll submit a patch shortly.

i2c_device_probe() seems exactly the right place to do this. It's in
fact the equivalent of what I had proposed in my original patch series
where this was done in platform_drv_probe().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]