From: Julia Lawall <[email protected]>
The definition of the irq_ipi structure has two initializations of the
flags field. This combines them.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 { ... .fld@p = E, ...};
@s@
identifier I, s, r.fld;
position r.p0,p;
expression E;
@@
struct I s =@p0 { ... .fld@p = E, ...};
@script:python@
p0 << r.p0;
fld << r.fld;
ps << s.p;
pr << r.p;
@@
if int(ps[0].line)!=int(pr[0].line) or int(ps[0].column)!=int(pr[0].column):
cocci.print_main(fld,p0)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
arch/mips/kernel/smtc.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
index 67153a0..4d181df 100644
--- a/arch/mips/kernel/smtc.c
+++ b/arch/mips/kernel/smtc.c
@@ -1098,9 +1098,8 @@ static void ipi_irq_dispatch(void)
static struct irqaction irq_ipi = {
.handler = ipi_interrupt,
- .flags = IRQF_DISABLED,
- .name = "SMTC_IPI",
- .flags = IRQF_PERCPU
+ .flags = IRQF_DISABLED | IRQF_PERCPU,
+ .name = "SMTC_IPI"
};
static void setup_cross_vpe_interrupts(unsigned int nvpe)
On Mon, Sep 21, 2009 at 05:08:55PM +0200, Julia Lawall wrote:
Adding Kevin "SMTC Kissel to cc.
> From: Julia Lawall <[email protected]>
>
> The definition of the irq_ipi structure has two initializations of the
> flags field. This combines them.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> identifier I, s, fld;
> position p0,p;
> expression E;
> @@
>
> struct I s =@p0 { ... .fld@p = E, ...};
>
> @s@
> identifier I, s, r.fld;
> position r.p0,p;
> expression E;
> @@
>
> struct I s =@p0 { ... .fld@p = E, ...};
>
> @script:python@
> p0 << r.p0;
> fld << r.fld;
> ps << s.p;
> pr << r.p;
> @@
>
> if int(ps[0].line)!=int(pr[0].line) or int(ps[0].column)!=int(pr[0].column):
> cocci.print_main(fld,p0)
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> arch/mips/kernel/smtc.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
> index 67153a0..4d181df 100644
> --- a/arch/mips/kernel/smtc.c
> +++ b/arch/mips/kernel/smtc.c
> @@ -1098,9 +1098,8 @@ static void ipi_irq_dispatch(void)
>
> static struct irqaction irq_ipi = {
> .handler = ipi_interrupt,
> - .flags = IRQF_DISABLED,
> - .name = "SMTC_IPI",
> - .flags = IRQF_PERCPU
> + .flags = IRQF_DISABLED | IRQF_PERCPU,
> + .name = "SMTC_IPI"
> };
>
> static void setup_cross_vpe_interrupts(unsigned int nvpe)
The actual semantic of this code as implemented by gcc is that all but the
last initializer are ignored so until somebody actually tests your code
I'll just remove the first of the two initializers and put a comment there.
Ralf
I'm still on the mailing list, and had seen this going by. I'm not sure
where that second .flags declaration got added. Way, way back when I
was pretty much the only maintainer of the file, irq_ipi.flags was
explicitly initialized to IRQF_DISABLED by an actual assignment
statement in setp_cross_vpe_interrupts(), and the per-CPUness was
handled by an "irq_desc[cpu_ipi_irq].status |= IRQ_PER_CPU". My guess
is that first someone (maybe me) migrated the IRQF_DISABLED assignment
into the declaration of the struct, and that later someone found the
IRQ_PER_CPU thing bogus or deprecated and converted it into a second
.flags line in the struct declaration, missing the fact that there was
already one there.
In any case, I'm willing to sign off on Julia's patch. It's certainly
more important that the IRQ be PER_CPU than initially DISABLED, but
during the time when SMTC was seeing its heaviest testing at MIPS, both
attributes were true.
Regards,
Kevin K.
Ralf Baechle wrote:
> On Mon, Sep 21, 2009 at 05:08:55PM +0200, Julia Lawall wrote:
>
> Adding Kevin "SMTC Kissel to cc.
>
>
>> From: Julia Lawall <[email protected]>
>>
>> The definition of the irq_ipi structure has two initializations of the
>> flags field. This combines them.
>>
>> The semantic match that finds this problem is as follows:
>> (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @r@
>> identifier I, s, fld;
>> position p0,p;
>> expression E;
>> @@
>>
>> struct I s =@p0 { ... .fld@p = E, ...};
>>
>> @s@
>> identifier I, s, r.fld;
>> position r.p0,p;
>> expression E;
>> @@
>>
>> struct I s =@p0 { ... .fld@p = E, ...};
>>
>> @script:python@
>> p0 << r.p0;
>> fld << r.fld;
>> ps << s.p;
>> pr << r.p;
>> @@
>>
>> if int(ps[0].line)!=int(pr[0].line) or int(ps[0].column)!=int(pr[0].column):
>> cocci.print_main(fld,p0)
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> arch/mips/kernel/smtc.c | 5 ++---
>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
>> index 67153a0..4d181df 100644
>> --- a/arch/mips/kernel/smtc.c
>> +++ b/arch/mips/kernel/smtc.c
>> @@ -1098,9 +1098,8 @@ static void ipi_irq_dispatch(void)
>>
>> static struct irqaction irq_ipi = {
>> .handler = ipi_interrupt,
>> - .flags = IRQF_DISABLED,
>> - .name = "SMTC_IPI",
>> - .flags = IRQF_PERCPU
>> + .flags = IRQF_DISABLED | IRQF_PERCPU,
>> + .name = "SMTC_IPI"
>> };
>>
>> static void setup_cross_vpe_interrupts(unsigned int nvpe)
>>
>
> The actual semantic of this code as implemented by gcc is that all but the
> last initializer are ignored so until somebody actually tests your code
> I'll just remove the first of the two initializers and put a comment there.
>
> Ralf
>
Ha! Found the breakage. It wasn't a two-phase commit. It was just an
overworked mantainer in a hurry, in July 2007. See commit
033890b084adfa367c544864451d7730552ce8bf
Regards,
Kevin K.
Kevin D. Kissell wrote:
> I'm still on the mailing list, and had seen this going by. I'm not
> sure where that second .flags declaration got added. Way, way back
> when I was pretty much the only maintainer of the file, irq_ipi.flags
> was explicitly initialized to IRQF_DISABLED by an actual assignment
> statement in setp_cross_vpe_interrupts(), and the per-CPUness was
> handled by an "irq_desc[cpu_ipi_irq].status |= IRQ_PER_CPU". My guess
> is that first someone (maybe me) migrated the IRQF_DISABLED assignment
> into the declaration of the struct, and that later someone found the
> IRQ_PER_CPU thing bogus or deprecated and converted it into a second
> .flags line in the struct declaration, missing the fact that there was
> already one there.
>
> In any case, I'm willing to sign off on Julia's patch. It's certainly
> more important that the IRQ be PER_CPU than initially DISABLED, but
> during the time when SMTC was seeing its heaviest testing at MIPS,
> both attributes were true.
>
> Regards,
>
> Kevin K.
>
> Ralf Baechle wrote:
>> On Mon, Sep 21, 2009 at 05:08:55PM +0200, Julia Lawall wrote:
>>
>> Adding Kevin "SMTC Kissel to cc.
>>
>>
>>> From: Julia Lawall <[email protected]>
>>>
>>> The definition of the irq_ipi structure has two initializations of the
>>> flags field. This combines them.
>>>
>>> The semantic match that finds this problem is as follows:
>>> (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @r@
>>> identifier I, s, fld;
>>> position p0,p;
>>> expression E;
>>> @@
>>>
>>> struct I s =@p0 { ... .fld@p = E, ...};
>>>
>>> @s@
>>> identifier I, s, r.fld;
>>> position r.p0,p;
>>> expression E;
>>> @@
>>>
>>> struct I s =@p0 { ... .fld@p = E, ...};
>>>
>>> @script:python@
>>> p0 << r.p0;
>>> fld << r.fld;
>>> ps << s.p;
>>> pr << r.p;
>>> @@
>>>
>>> if int(ps[0].line)!=int(pr[0].line) or
>>> int(ps[0].column)!=int(pr[0].column):
>>> cocci.print_main(fld,p0)
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <[email protected]>
>>>
>>> ---
>>> arch/mips/kernel/smtc.c | 5 ++---
>>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
>>> index 67153a0..4d181df 100644
>>> --- a/arch/mips/kernel/smtc.c
>>> +++ b/arch/mips/kernel/smtc.c
>>> @@ -1098,9 +1098,8 @@ static void ipi_irq_dispatch(void)
>>>
>>> static struct irqaction irq_ipi = {
>>> .handler = ipi_interrupt,
>>> - .flags = IRQF_DISABLED,
>>> - .name = "SMTC_IPI",
>>> - .flags = IRQF_PERCPU
>>> + .flags = IRQF_DISABLED | IRQF_PERCPU,
>>> + .name = "SMTC_IPI"
>>> };
>>>
>>> static void setup_cross_vpe_interrupts(unsigned int nvpe)
>>>
>>
>> The actual semantic of this code as implemented by gcc is that all
>> but the
>> last initializer are ignored so until somebody actually tests your code
>> I'll just remove the first of the two initializers and put a comment
>> there.
>>
>> Ralf
>>
>
>
On Mon, Sep 21, 2009 at 01:41:49PM -0700, Kevin D. Kissell wrote:
> Ha! Found the breakage. It wasn't a two-phase commit. It was just an
> overworked mantainer in a hurry, in July 2007. See commit
> 033890b084adfa367c544864451d7730552ce8bf
An overworked maintainer in cooperation with a broken compiler. Worse -
it's not the first time this type of bug strikes but probably this was
never brought to the gcc maintainers' attention.
Ralf
On Mon, Sep 21, 2009 at 01:23:45PM -0700, Kevin D. Kissell wrote:
> I'm still on the mailing list, and had seen this going by. I'm not sure
> where that second .flags declaration got added. Way, way back when I
> was pretty much the only maintainer of the file, irq_ipi.flags was
> explicitly initialized to IRQF_DISABLED by an actual assignment
> statement in setp_cross_vpe_interrupts(), and the per-CPUness was
> handled by an "irq_desc[cpu_ipi_irq].status |= IRQ_PER_CPU". My guess
> is that first someone (maybe me) migrated the IRQF_DISABLED assignment
> into the declaration of the struct, and that later someone found the
> IRQ_PER_CPU thing bogus or deprecated and converted it into a second
> .flags line in the struct declaration, missing the fact that there was
> already one there.
>
> In any case, I'm willing to sign off on Julia's patch. It's certainly
> more important that the IRQ be PER_CPU than initially DISABLED, but
> during the time when SMTC was seeing its heaviest testing at MIPS, both
> attributes were true.
I've reverted my patch and merged Julia's original patch with an extra
comment added.
Thanks Julia,
Ralf