[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |