Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 02/19/2014 03:07 PM, Jan Beulich wrote:
>>>> On 19.02.14 at 15:51, Julien Grall <julien.grall@linaro.org> wrote:
>> Adding Keir and Jan.
>>
>> On 02/19/2014 02:38 PM, Ian Campbell wrote:
>>> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote:
>>>
>>>>>> -static void gic_irq_enable(struct irq_desc *desc)
>>>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc)
>>>>>
>>>>> unsigned? What are the error codes here going to be?
>>>>
>>>> This is the return type requested by hw_interrupt_type.startup.
>>>>
>>>> It seems that the return is never checked (even in x86 code). Maybe we
>>>> should change the prototype of hw_interrupt_type.startup.
>>>
>>> Worth investigating. I wonder if someone thought this might return the
>>> resulting interrupt number (those are normally unsigned int I think) or
>>> if it actually did used to etc.
>>
>> I think it was copied from Linux which also have unsigned int. I gave a
>> quick look to the code and this callback is only used in 2 places which
>> always return 0.
>>
>> Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning
>> an int...
>>
>> I can create a patch to return void instead of unsigned if everyone is
>> happy with this solution.
> 
> I'd be fine with such a change; I'd like to ask though that if you
> do this, you at the same time do the resulting possible cleanup:
> As an example, xen/arch/x86/msi.c:startup_msi_irq() becomes
> unnecessary then. It will in fact be interesting to see how many
> distinct startup routines actually remain.

Before the clean up there was 8 distinct startup routines for x86. No
there is only 2:
  - drivers/passthrough/amd/iommu_init.c: iommu_maskable_msi_startup
  - arch/x86/ioapic.c: startup_edge_ioapic_irq

For a latter one, I'm a bit surprised that the function can return 1,
but the result is never used.

Cheers,

-- 
Julien Grall