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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

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

> >> @@ -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?

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

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

> >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> >> index b52c26f..107c13a 100644
> >> --- a/xen/include/asm-arm/irq.h
> >> +++ b/xen/include/asm-arm/irq.h
> >> @@ -16,6 +16,7 @@ struct arch_pirq
> >>  
> >>  struct arch_irq_desc {
> >>      int eoi_cpu;
> >> +    unsigned int type;
> > 
> > I was wondering through the above if this ought to be a "bool_t level"
> > or not. Thoughts?
> 
> We have 5 possibles types:
>       - default : we don't have to set the edge bit
>         - level high/low
>         - edge rising/falling

OK.

> Even if GICv2 is only handling 2 types, it can change for the next
> version of GIC.

Well, there aren't all that many more ways you can frob a physical line.
I suppose MSIs will account for one more though.

Ian.


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