[PATCH] IPIC: Don't call set_irq_handler with desc->lock held.

[PATCH] IPIC: Don't call set_irq_handler with desc->lock held.

[Scott Wood]

This patch causes ipic_set_irq_type to set the handler directly rather
than call set_irq_handler, which causes spinlock recursion because
the lock is already held when ipic_set_irq_type is called.

I'm also not convinced that ipic_set_irq_type should be changing the
handler at all.  There seem to be several controllers that don't and
several that do.  Those that do would break what appears to be a common
usage of calling set_irq_chip_and_handler followed by set_irq_type, if a
non-standard handler were to be used.  OTOH, irq_create_of_mapping()
doesn't set the handler, but only calls set_irq_type().

This patch gets things working in the spinlock-debugging-enabled case,
but I'm curious as to where the handler setting is ideally supposed to be
done.  I don't see any documentation on set_irq_type() that clarifies
what the semantics are supposed to be.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/sysdev/ipic.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
index bc4d4a7..746f78c 100644
--- a/arch/powerpc/sysdev/ipic.c
+++ b/arch/powerpc/sysdev/ipic.c
@@ -473,9 +473,9 @@ static int ipic_set_irq_type(unsigned in
 	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
 	if (flow_type & IRQ_TYPE_LEVEL_LOW)  {
 		desc->status |= IRQ_LEVEL;
-		set_irq_handler(virq, handle_level_irq);
+		desc->handle_irq = handle_level_irq;
 	} else {
-		set_irq_handler(virq, handle_edge_irq);
+		desc->handle_irq = handle_edge_irq;
 	}
 
 	/* only EXT IRQ senses are programmable on ipic
-- 
1.4.2.3

Re: [PATCH] IPIC: Don't call set_irq_handler with desc->lock held.

[Benjamin Herrenschmidt]

> This patch gets things working in the spinlock-debugging-enabled case,
> but I'm curious as to where the handler setting is ideally supposed to be
> done.  I don't see any documentation on set_irq_type() that clarifies
> what the semantics are supposed to be.

It's unclear... the common code calls it, I think, when request_irq() is
passed explicit type. When changing the type, some PICs need to change
the handler, some don't. It looks that none of the PICs I've dealt with
have this problem though...

Ben.

Re: [PATCH] IPIC: Don't call set_irq_handler with desc->lock held.

[Kumar Gala]

On Oct 23, 2006, at 11:35 AM, Scott Wood wrote:

> This patch causes ipic_set_irq_type to set the handler directly rather
> than call set_irq_handler, which causes spinlock recursion because
> the lock is already held when ipic_set_irq_type is called.
>
> I'm also not convinced that ipic_set_irq_type should be changing the
> handler at all.  There seem to be several controllers that don't and
> several that do.  Those that do would break what appears to be a  
> common
> usage of calling set_irq_chip_and_handler followed by set_irq_type,  
> if a
> non-standard handler were to be used.  OTOH, irq_create_of_mapping()
> doesn't set the handler, but only calls set_irq_type().
>
> This patch gets things working in the spinlock-debugging-enabled case,
> but I'm curious as to where the handler setting is ideally supposed  
> to be
> done.  I don't see any documentation on set_irq_type() that clarifies
> what the semantics are supposed to be.

Guys, Scott pointed this problem out on a PPC interrupt controller,  
and wanted to raise it in a larger forum since it appears to exist on  
at least one ARM interrupt controller I looked at (ixp4xx).  What is  
the proper solution to handle this.

The callers of set_type() I found all grab desc->lock.

- kumar

>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  arch/powerpc/sysdev/ipic.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
> index bc4d4a7..746f78c 100644
> --- a/arch/powerpc/sysdev/ipic.c
> +++ b/arch/powerpc/sysdev/ipic.c
> @@ -473,9 +473,9 @@ static int ipic_set_irq_type(unsigned in
>  	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
>  	if (flow_type & IRQ_TYPE_LEVEL_LOW)  {
>  		desc->status |= IRQ_LEVEL;
> -		set_irq_handler(virq, handle_level_irq);
> +		desc->handle_irq = handle_level_irq;
>  	} else {
> -		set_irq_handler(virq, handle_edge_irq);
> +		desc->handle_irq = handle_edge_irq;
>  	}
>
>  	/* only EXT IRQ senses are programmable on ipic
> -- 
> 1.4.2.3
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev