Re: [PATCH] irq: Remove irqaction.free_on_release

[Andrew Cooper <andrew.cooper3@citrix.com>]

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