[PATCH] irq: Remove irqaction.free_on_release

[PATCH] irq: Remove irqaction.free_on_release

[Andrew Cooper]

It is always set to 1, and only checked on some of the codepaths which free an
irqaction.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
This patch does touch common code as well as x86 and arm architectures,
so probably needs quite a few acks.

diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/arm/gic.c
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -373,7 +373,7 @@ void __init release_irq(unsigned int irq
     /* Wait to make sure it's not being used on another CPU */
     do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
 
-    if (action && action->free_on_release)
+    if ( action )
         xfree(action);
 }
 
diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/arm/irq.c
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -97,7 +97,6 @@ int __init request_irq(unsigned int irq,
     action->handler = handler;
     action->name = devname;
     action->dev_id = dev_id;
-    action->free_on_release = 1;
 
     retval = setup_irq(irq, action);
     if (retval)
diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -952,7 +952,6 @@ int __init request_irq(unsigned int irq,
     action->handler = handler;
     action->name = devname;
     action->dev_id = dev_id;
-    action->free_on_release = 1;
 
     retval = setup_irq(irq, action);
     if (retval)
@@ -979,7 +978,7 @@ void __init release_irq(unsigned int irq
     /* Wait to make sure it's not being used on another CPU */
     do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
 
-    if (action && action->free_on_release)
+    if ( action )
         xfree(action);
 }
 
diff -r 5fbdbf585f5f -r 37f1afbec80b xen/include/xen/irq.h
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -13,7 +13,6 @@ struct irqaction {
     void (*handler)(int, void *, struct cpu_user_regs *);
     const char *name;
     void *dev_id;
-    bool_t free_on_release;
 };
 
 /*

Re: [PATCH] irq: Remove irqaction.free_on_release

[Keir Fraser]

But reasoning behind c/s 20153, which introduced it, still applies? Callers
using setup_irq() don't want their passed-in irqaction to be xfree()ed?

 -- Keir

On 08/10/2012 14:09, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> It is always set to 1, and only checked on some of the codepaths which free an
> irqaction.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> --
> This patch does touch common code as well as x86 and arm architectures,
> so probably needs quite a few acks.
> 
> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/arm/gic.c
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -373,7 +373,7 @@ void __init release_irq(unsigned int irq
>      /* Wait to make sure it's not being used on another CPU */
>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
>  
> -    if (action && action->free_on_release)
> +    if ( action )
>          xfree(action);
>  }
>  
> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/arm/irq.c
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -97,7 +97,6 @@ int __init request_irq(unsigned int irq,
>      action->handler = handler;
>      action->name = devname;
>      action->dev_id = dev_id;
> -    action->free_on_release = 1;
>  
>      retval = setup_irq(irq, action);
>      if (retval)
> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/x86/irq.c
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -952,7 +952,6 @@ int __init request_irq(unsigned int irq,
>      action->handler = handler;
>      action->name = devname;
>      action->dev_id = dev_id;
> -    action->free_on_release = 1;
>  
>      retval = setup_irq(irq, action);
>      if (retval)
> @@ -979,7 +978,7 @@ void __init release_irq(unsigned int irq
>      /* Wait to make sure it's not being used on another CPU */
>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
>  
> -    if (action && action->free_on_release)
> +    if ( action )
>          xfree(action);
>  }
>  
> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/include/xen/irq.h
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -13,7 +13,6 @@ struct irqaction {
>      void (*handler)(int, void *, struct cpu_user_regs *);
>      const char *name;
>      void *dev_id;
> -    bool_t free_on_release;
>  };
>  
>  /*

Re: [PATCH] irq: Remove irqaction.free_on_release

[Andrew Cooper]

On 09/10/12 12:44, Keir Fraser wrote:
> But reasoning behind c/s 20153, which introduced it, still applies? Callers
> using setup_irq() don't want their passed-in irqaction to be xfree()ed?
>
>  -- Keir

Yikes - yes it does.  My original reading of the code was that it was
being set, hence the patch, but upon closer reading I am wrong.

As for grep'ing, it is only ever implicitly set to 0 by compound struct
initialisation with { 0 }

Furthermore, there is now a case where an ns16550 uart->irq can be set
higher than nr_irqs_gsi, although it is unclear whether this will
actually cause a problem and hit the unconditional xfree(action) in
dynamic_irq_cleanip(), which is only protected by BUG()'ing if the irq
index is within the gsi range.

~Andrew

>
> On 08/10/2012 14:09, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
>> It is always set to 1, and only checked on some of the codepaths which free an
>> irqaction.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> --
>> This patch does touch common code as well as x86 and arm architectures,
>> so probably needs quite a few acks.
>>
>> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/arm/gic.c
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -373,7 +373,7 @@ void __init release_irq(unsigned int irq
>>      /* Wait to make sure it's not being used on another CPU */
>>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
>>  
>> -    if (action && action->free_on_release)
>> +    if ( action )
>>          xfree(action);
>>  }
>>  
>> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/arm/irq.c
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -97,7 +97,6 @@ int __init request_irq(unsigned int irq,
>>      action->handler = handler;
>>      action->name = devname;
>>      action->dev_id = dev_id;
>> -    action->free_on_release = 1;
>>  
>>      retval = setup_irq(irq, action);
>>      if (retval)
>> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/arch/x86/irq.c
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -952,7 +952,6 @@ int __init request_irq(unsigned int irq,
>>      action->handler = handler;
>>      action->name = devname;
>>      action->dev_id = dev_id;
>> -    action->free_on_release = 1;
>>  
>>      retval = setup_irq(irq, action);
>>      if (retval)
>> @@ -979,7 +978,7 @@ void __init release_irq(unsigned int irq
>>      /* Wait to make sure it's not being used on another CPU */
>>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
>>  
>> -    if (action && action->free_on_release)
>> +    if ( action )
>>          xfree(action);
>>  }
>>  
>> diff -r 5fbdbf585f5f -r 37f1afbec80b xen/include/xen/irq.h
>> --- a/xen/include/xen/irq.h
>> +++ b/xen/include/xen/irq.h
>> @@ -13,7 +13,6 @@ struct irqaction {
>>      void (*handler)(int, void *, struct cpu_user_regs *);
>>      const char *name;
>>      void *dev_id;
>> -    bool_t free_on_release;
>>  };
>>  
>>  /*
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com