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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-08 at 12:46 +0100, Julien Grall wrote:
> 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.

Do we know if this is because ePAPR or some other standard say this is
the default or just a random choice by Linux?

>  I've just noticed we do the same thing in gic_dist_init.

Whatever the reason for them doing it it probably make sense to do the
same, otherwise we are just making pain for ourselves.

I'm not convinced that the exynos DT doesn't also need to be fixed
though.


> >>
> >>>> +
> >>>> +    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...

Sure, if INVALID_IRQ is used then it should be used, and we should
probably make it ~0 to avoid people mistreating it.

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

Well, this is a more global thing than this one function really.

> >>>> +    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);

If you like.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel