Re: [PATCH] arch/mips: remove duplicate structure field initialization

Kevin D. Kissell
2009-09-21T22:28:37Z

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 <julia@diku.dk>
>>>
>>> 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 <julia@diku.dk>
>>>
>>> ---
>>> 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
>>
>
>