Mailing List Archive: [PATCH] irq: Remove irqaction.free_on

[PATCH] irq: Remove irqaction.free_on_release

Oct 8, 2012, 6:09 AM

Post #1 of 3 (95 views)

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;
};

/*

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] irq: Remove irqaction.free_on_release [ In reply to ]

keir at xen

Oct 9, 2012, 4:44 AM

Post #2 of 3 (89 views)

Permalink

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;
> };
>
> /*

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] irq: Remove irqaction.free_on_release [ In reply to ]

andrew.cooper3 at citrix

Oct 9, 2012, 7:38 AM

Post #3 of 3 (89 views)

Permalink

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel