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