Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

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

On 04/07/2014 05:26 PM, Ian Campbell wrote:
> On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote:
>>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>>>      /* Set edge / level */
>>>>      cfg = GICD[GICD_ICFGR + irq / 16];
>>>>      edgebit = 2u << (2 * (irq % 16));
>>>> -    if ( level )
>>>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
>>>
>>> Is getting DT_IRQ_TYPE_NONE here not an error?
>>
>> No, there is some DT like the exynos one which is using 0 (i.e
>> DT_IRQ_TYPE_NONE) for the IRQ type.
> 
> The underlying physical interrupt must be one or the other though,
> surely?
> 
> So either there is some implicit (or perhaps documented?) assumption
> that NONE==something or the DT is buggy.

By default Linux is setting every interrupt to be level triggered,
active low. I've just noticed we do the same thing in gic_dist_init.

>>
>> I guess we have to skip setting level/edge property in this case.
>>
>>> Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
>>> be refactored e.g. into dt_irq_type_is_level_triggered(const something
>>> type)?
>>
>> I was wondering something like that instead:
>>
>> if ( (type & DT_IRQ_TYPE_LEVEL_MASK) )
>> ...
>> else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) )
>> ...
>>
>> So we skip the DT_IRQ_TYPE_NONE.
> 
> Well, it seems the existing code treats NONE as == level, I don't know
> if that is deliberate or not.

I wrote this code, until now I had forgotten why I was using NONE :).

>>>> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>>>>      BUG();
>>>>  }
>>>>  
>>>> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
>>>> +{
>>>> +    unsigned int flags;
>>>> +    int ret = -EBUSY;
>>>> +
>>>> +    if ( type == DT_IRQ_TYPE_NONE )
>>>> +        return 0;
>>>> +
>>>> +    spin_lock_irqsave(&desc->lock, flags);
>>>> +
>>>> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
>>>> +        goto err;
>>>> +
>>>> +    desc->arch.type = type;
>>>
>>> There was an open coded assignment in the guest path which unfortunately
>>> I already trimmed. Shouldn't that have all these checks too?
>>
>> No, because with patch #11 the desc->arch.type is only set once by IRQ.
> 
> I don't follow. What is all this stuff above for if that is the case?

> Was I misremembering the other instance of desc->arch.type = type?

Sorry, I was talking about desc->arch.type = type in route_dt_irq_to_guest.

>>
>>>> +
>>>> +    ret = 0;
>>>> +
>>>> +err:
>>>> +    spin_unlock_irqrestore(&desc->lock, flags);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +unsigned int platform_get_irq(const struct dt_device_node *device,
>>>> +                              int index)
>>>> +{
>>>> +    struct dt_irq dt_irq;
>>>> +    struct irq_desc *desc;
>>>> +    unsigned int type, irq;
>>>> +    int res;
>>>> +
>>>> +    res = dt_device_get_irq(device, index, &dt_irq);
>>>> +    if ( res )
>>>> +        return 0;
>>>
>>> Not an error? Do we take precautions against IRQ0 being actually used
>>> somewhere?
>>
>> Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI.
> 
> Ah yes.
> 
>>> We should have an explicit #define for an invalid IRQ number.
>>
>> I don't think it's useful because the device tree can't provide an IRQ
>> smaller than 16.
> 
> It would also potentially serve to make the code more self-documenting.
> "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more
> obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok
> and more normal though)

I would prefer to use either both either nothing. It's confusing to
return INVALID_IRQ and assuming after it's always 0...

If you prefer I can add a common above the function to say 0 is used
when an error is occured.

>>
>>>> +    irq = dt_irq.irq;
>>>> +    type = dt_irq.type;
>>>> +
>>>> +    /* Setup the IRQ type */
>>>> +
>>>> +    if ( irq < NR_LOCAL_IRQS )
>>>> +    {
>>>> +        unsigned int cpu;
>>>> +        /* For PPIs, we need to set IRQ type on every online CPUs */
>>>> +        for_each_cpu( cpu, &cpu_online_map )
>>>> +        {
>>>> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
>>>> +            res = irq_set_type(desc, type);
>>>> +            if ( res )
>>>> +                return 0;
>>>
>>> Error?
>>>
>>> Also no need to undo any partial work?
>>
>> desc->arch.type should be sync on every CPU. It would be crazy to have a
>> different IRQ type on every CPU.
> 
> Well, the code as it stands appears to make a partial attempt at
> handling just that. If that weren't the case irq_set_type wouldn't be
> able to fail for cpu > 0.

I just use the irq_set_type handler for more convenience. If you want I
can add an ASSERT(cpu > 0 && !res);

Regards,

-- 
Julien Grall