[PATCH] of/irq: Fix device_node refcount in of_irq_parse_raw()

[PATCH] of/irq: Fix device_node refcount in of_irq_parse_raw()

[Cédric Le Goater]

Commit 2361613206e6, "of/irq: Refactor interrupt-map parsing" changed
the refcount on the device_node causing an error in of_node_put():

ERROR: Bad of_node_put() on /pci@800000020000000
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc3-dirty #2
Call Trace:
[c00000003e403500] [c0000000000144fc] .show_stack+0x7c/0x1f0 (unreliable)
[c00000003e4035d0] [c00000000070f250] .dump_stack+0x88/0xb4
[c00000003e403650] [c0000000005e8768] .of_node_release+0xd8/0xf0
[c00000003e4036e0] [c0000000005eeafc] .of_irq_parse_one+0x10c/0x280
[c00000003e4037a0] [c0000000005efd4c] .of_irq_parse_pci+0x3c/0x1d0
[c00000003e403840] [c000000000038240] .pcibios_setup_device+0xa0/0x2e0
[c00000003e403910] [c0000000000398f0] .pcibios_setup_bus_devices+0x60/0xd0
[c00000003e403990] [c00000000003b3a4] .__of_scan_bus+0x1a4/0x2b0
[c00000003e403a80] [c00000000003a62c] .pcibios_scan_phb+0x30c/0x410
[c00000003e403b60] [c0000000009fe430] .pcibios_init+0x7c/0xd4

This patch adjusts the refcount in the walk of the interrupt tree.
When a match is found, there is no need to increase the refcount 
on 'out_irq->np' as 'newpar' is already holding a ref. The refcount
balance between 'ipar' and 'newpar' is maintained in the skiplevel:
goto label.

This patch also removes the usage of the device_node variable 'old' 
which seems useless after the latest changes.

Signed-off-by: Cédric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>
---

This patch was tested on powerpc, pseries and powernv. This is a 
new area for me so I might have missed a path. Please take a look.

We could now introduce an helper routine to look for #address-cells in
of_irq_parse_raw(). This can be the subject of another patch.

Thanks,

C.

 drivers/of/irq.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 786b0b47fae4..27212402c532 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -165,7 +165,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 		if (of_get_property(ipar, "interrupt-controller", NULL) !=
 				NULL) {
 			pr_debug(" -> got it !\n");
-			of_node_put(old);
 			return 0;
 		}
 
@@ -250,8 +249,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 		 * Successfully parsed an interrrupt-map translation; copy new
 		 * interrupt specifier into the out_irq structure
 		 */
-		of_node_put(out_irq->np);
-		out_irq->np = of_node_get(newpar);
+		out_irq->np = newpar;
 
 		match_array = imap - newaddrsize - newintsize;
 		for (i = 0; i < newintsize; i++)
@@ -268,7 +266,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 	}
  fail:
 	of_node_put(ipar);
-	of_node_put(out_irq->np);
 	of_node_put(newpar);
 
 	return -EINVAL;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/irq: Fix device_node refcount in of_irq_parse_raw()

[Benjamin Herrenschmidt]

On Tue, 2013-12-17 at 18:32 +0100, Cédric Le Goater wrote:
> Commit 2361613206e6, "of/irq: Refactor interrupt-map parsing" changed
> the refcount on the device_node causing an error in of_node_put():

Grant, Thierry, this is a regression, please send to Linus ASAP...

Cheers,
Ben.

> ERROR: Bad of_node_put() on /pci@800000020000000
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc3-dirty #2
> Call Trace:
> [c00000003e403500] [c0000000000144fc] .show_stack+0x7c/0x1f0 (unreliable)
> [c00000003e4035d0] [c00000000070f250] .dump_stack+0x88/0xb4
> [c00000003e403650] [c0000000005e8768] .of_node_release+0xd8/0xf0
> [c00000003e4036e0] [c0000000005eeafc] .of_irq_parse_one+0x10c/0x280
> [c00000003e4037a0] [c0000000005efd4c] .of_irq_parse_pci+0x3c/0x1d0
> [c00000003e403840] [c000000000038240] .pcibios_setup_device+0xa0/0x2e0
> [c00000003e403910] [c0000000000398f0] .pcibios_setup_bus_devices+0x60/0xd0
> [c00000003e403990] [c00000000003b3a4] .__of_scan_bus+0x1a4/0x2b0
> [c00000003e403a80] [c00000000003a62c] .pcibios_scan_phb+0x30c/0x410
> [c00000003e403b60] [c0000000009fe430] .pcibios_init+0x7c/0xd4
> 
> This patch adjusts the refcount in the walk of the interrupt tree.
> When a match is found, there is no need to increase the refcount 
> on 'out_irq->np' as 'newpar' is already holding a ref. The refcount
> balance between 'ipar' and 'newpar' is maintained in the skiplevel:
> goto label.
> 
> This patch also removes the usage of the device_node variable 'old' 
> which seems useless after the latest changes.
> 
> Signed-off-by: Cédric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>
> ---
> 
> This patch was tested on powerpc, pseries and powernv. This is a 
> new area for me so I might have missed a path. Please take a look.
> 
> We could now introduce an helper routine to look for #address-cells in
> of_irq_parse_raw(). This can be the subject of another patch.
> 
> Thanks,
> 
> C.
> 
>  drivers/of/irq.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 786b0b47fae4..27212402c532 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -165,7 +165,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>  		if (of_get_property(ipar, "interrupt-controller", NULL) !=
>  				NULL) {
>  			pr_debug(" -> got it !\n");
> -			of_node_put(old);
>  			return 0;
>  		}
>  
> @@ -250,8 +249,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>  		 * Successfully parsed an interrrupt-map translation; copy new
>  		 * interrupt specifier into the out_irq structure
>  		 */
> -		of_node_put(out_irq->np);
> -		out_irq->np = of_node_get(newpar);
> +		out_irq->np = newpar;
>  
>  		match_array = imap - newaddrsize - newintsize;
>  		for (i = 0; i < newintsize; i++)
> @@ -268,7 +266,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>  	}
>   fail:
>  	of_node_put(ipar);
> -	of_node_put(out_irq->np);
>  	of_node_put(newpar);
>  
>  	return -EINVAL;


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/irq: Fix device_node refcount in of_irq_parse_raw()

[Rob Herring]

[-- Attachment #1.1: Type: text/plain, Size: 3817 bytes --]

On Sun, Dec 29, 2013 at 8:37 PM, Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

> On Tue, 2013-12-17 at 18:32 +0100, Cédric Le Goater wrote:
> > Commit 2361613206e6, "of/irq: Refactor interrupt-map parsing" changed
> > the refcount on the device_node causing an error in of_node_put():
>
> Grant, Thierry, this is a regression, please send to Linus ASAP...
>
>
I'm working on doing that right this moment.

Rob


> Cheers,
> Ben.
>
> > ERROR: Bad of_node_put() on /pci@800000020000000
> > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc3-dirty #2
> > Call Trace:
> > [c00000003e403500] [c0000000000144fc] .show_stack+0x7c/0x1f0 (unreliable)
> > [c00000003e4035d0] [c00000000070f250] .dump_stack+0x88/0xb4
> > [c00000003e403650] [c0000000005e8768] .of_node_release+0xd8/0xf0
> > [c00000003e4036e0] [c0000000005eeafc] .of_irq_parse_one+0x10c/0x280
> > [c00000003e4037a0] [c0000000005efd4c] .of_irq_parse_pci+0x3c/0x1d0
> > [c00000003e403840] [c000000000038240] .pcibios_setup_device+0xa0/0x2e0
> > [c00000003e403910] [c0000000000398f0]
> .pcibios_setup_bus_devices+0x60/0xd0
> > [c00000003e403990] [c00000000003b3a4] .__of_scan_bus+0x1a4/0x2b0
> > [c00000003e403a80] [c00000000003a62c] .pcibios_scan_phb+0x30c/0x410
> > [c00000003e403b60] [c0000000009fe430] .pcibios_init+0x7c/0xd4
> >
> > This patch adjusts the refcount in the walk of the interrupt tree.
> > When a match is found, there is no need to increase the refcount
> > on 'out_irq->np' as 'newpar' is already holding a ref. The refcount
> > balance between 'ipar' and 'newpar' is maintained in the skiplevel:
> > goto label.
> >
> > This patch also removes the usage of the device_node variable 'old'
> > which seems useless after the latest changes.
> >
> > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> > ---
> >
> > This patch was tested on powerpc, pseries and powernv. This is a
> > new area for me so I might have missed a path. Please take a look.
> >
> > We could now introduce an helper routine to look for #address-cells in
> > of_irq_parse_raw(). This can be the subject of another patch.
> >
> > Thanks,
> >
> > C.
> >
> >  drivers/of/irq.c |    5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 786b0b47fae4..27212402c532 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -165,7 +165,6 @@ int of_irq_parse_raw(const __be32 *addr, struct
> of_phandle_args *out_irq)
> >               if (of_get_property(ipar, "interrupt-controller", NULL) !=
> >                               NULL) {
> >                       pr_debug(" -> got it !\n");
> > -                     of_node_put(old);
> >                       return 0;
> >               }
> >
> > @@ -250,8 +249,7 @@ int of_irq_parse_raw(const __be32 *addr, struct
> of_phandle_args *out_irq)
> >                * Successfully parsed an interrrupt-map translation; copy
> new
> >                * interrupt specifier into the out_irq structure
> >                */
> > -             of_node_put(out_irq->np);
> > -             out_irq->np = of_node_get(newpar);
> > +             out_irq->np = newpar;
> >
> >               match_array = imap - newaddrsize - newintsize;
> >               for (i = 0; i < newintsize; i++)
> > @@ -268,7 +266,6 @@ int of_irq_parse_raw(const __be32 *addr, struct
> of_phandle_args *out_irq)
> >       }
> >   fail:
> >       of_node_put(ipar);
> > -     of_node_put(out_irq->np);
> >       of_node_put(newpar);
> >
> >       return -EINVAL;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #1.2: Type: text/html, Size: 5511 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] of/irq: Fix device_node refcount in of_irq_parse_raw()

[Benjamin Herrenschmidt]

On Sun, 2013-12-29 at 20:42 -0600, Rob Herring wrote:
> On Sun, Dec 29, 2013 at 8:37 PM, Benjamin Herrenschmidt
> <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> 
>         On Tue, 2013-12-17 at 18:32 +0100, Cédric Le Goater wrote:
>         > Commit 2361613206e6, "of/irq: Refactor interrupt-map
>         parsing" changed
>         > the refcount on the device_node causing an error in
>         of_node_put():
>         
>         
>         Grant, Thierry, this is a regression, please send to Linus
>         ASAP...
>         
> 
> 
> I'm working on doing that right this moment.

Thanks, looks like I'm not the only trying to get some work done
today :)

Cheers,
Ben.

> 
> Rob
> 
>         Cheers,
>         Ben.
>         
>         > ERROR: Bad of_node_put() on /pci@800000020000000
>         > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc3-dirty
>         #2
>         > Call Trace:
>         > [c00000003e403500] [c0000000000144fc] .show_stack+0x7c/0x1f0
>         (unreliable)
>         > [c00000003e4035d0] [c00000000070f250] .dump_stack+0x88/0xb4
>         > [c00000003e403650] [c0000000005e8768] .of_node_release
>         +0xd8/0xf0
>         > [c00000003e4036e0] [c0000000005eeafc] .of_irq_parse_one
>         +0x10c/0x280
>         > [c00000003e4037a0] [c0000000005efd4c] .of_irq_parse_pci
>         +0x3c/0x1d0
>         > [c00000003e403840] [c000000000038240] .pcibios_setup_device
>         +0xa0/0x2e0
>         > [c00000003e403910]
>         [c0000000000398f0] .pcibios_setup_bus_devices+0x60/0xd0
>         > [c00000003e403990] [c00000000003b3a4] .__of_scan_bus
>         +0x1a4/0x2b0
>         > [c00000003e403a80] [c00000000003a62c] .pcibios_scan_phb
>         +0x30c/0x410
>         > [c00000003e403b60] [c0000000009fe430] .pcibios_init
>         +0x7c/0xd4
>         >
>         > This patch adjusts the refcount in the walk of the interrupt
>         tree.
>         > When a match is found, there is no need to increase the
>         refcount
>         > on 'out_irq->np' as 'newpar' is already holding a ref. The
>         refcount
>         > balance between 'ipar' and 'newpar' is maintained in the
>         skiplevel:
>         > goto label.
>         >
>         > This patch also removes the usage of the device_node
>         variable 'old'
>         > which seems useless after the latest changes.
>         >
>         > Signed-off-by: Cédric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>
>         > ---
>         >
>         > This patch was tested on powerpc, pseries and powernv. This
>         is a
>         > new area for me so I might have missed a path. Please take a
>         look.
>         >
>         > We could now introduce an helper routine to look for
>         #address-cells in
>         > of_irq_parse_raw(). This can be the subject of another
>         patch.
>         >
>         > Thanks,
>         >
>         > C.
>         >
>         >  drivers/of/irq.c |    5 +----
>         >  1 file changed, 1 insertion(+), 4 deletions(-)
>         >
>         > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>         > index 786b0b47fae4..27212402c532 100644
>         > --- a/drivers/of/irq.c
>         > +++ b/drivers/of/irq.c
>         > @@ -165,7 +165,6 @@ int of_irq_parse_raw(const __be32 *addr,
>         struct of_phandle_args *out_irq)
>         >               if (of_get_property(ipar,
>         "interrupt-controller", NULL) !=
>         >                               NULL) {
>         >                       pr_debug(" -> got it !\n");
>         > -                     of_node_put(old);
>         >                       return 0;
>         >               }
>         >
>         > @@ -250,8 +249,7 @@ int of_irq_parse_raw(const __be32 *addr,
>         struct of_phandle_args *out_irq)
>         >                * Successfully parsed an interrrupt-map
>         translation; copy new
>         >                * interrupt specifier into the out_irq
>         structure
>         >                */
>         > -             of_node_put(out_irq->np);
>         > -             out_irq->np = of_node_get(newpar);
>         > +             out_irq->np = newpar;
>         >
>         >               match_array = imap - newaddrsize - newintsize;
>         >               for (i = 0; i < newintsize; i++)
>         > @@ -268,7 +266,6 @@ int of_irq_parse_raw(const __be32 *addr,
>         struct of_phandle_args *out_irq)
>         >       }
>         >   fail:
>         >       of_node_put(ipar);
>         > -     of_node_put(out_irq->np);
>         >       of_node_put(newpar);
>         >
>         >       return -EINVAL;
>         
>         
>         --
>         
>         To unsubscribe from this list: send the line "unsubscribe
>         devicetree" in
>         the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>         More majordomo info at
>          http://vger.kernel.org/majordomo-info.html
>         
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html