On Thu, 5 Dec 2013, Russell King - ARM Linux wrote: > So there's not much point discussing this with you until you: > > (a) calm down Done so :) > (b) analyse it properly and work out the frequency under which each > class of IRQ (those >= 32 and those < 32) call into these functions. Here you go: The frequency of invoking the gic_arch_extn callbacks is exactly equal to the frequency of interrupts in the system which go through the GIC at least for mask/unmask/eoi. The frequency of calls per interrupt depends on the interrupt type, but at minimum it is one. So basically it does for any interrupt independent of >= 32 or < 32: irq_fn(d) { do_something_common(d); if (gic_arch_extn.fn) gic_arch_extn.fn(d); do_something_common(d); } which then does: extn_fn(d) { if (this_irq_is_affected(d)) do_some_more(d); } So when this_irq_is_affected(d) boils down to if (d->irq [<>] n) then I agree, that it's debatable, whether the conditonal function call and the simple this_irq_is_affected(d) check is worth to worry about. Though there are people who care about two pointless conditonals for various reasons. But at the point when this_irq_is_affected(d) is doing a loop lookup, then this really starts to smell badly. Sure you might argue, that it's the problem of that particular patch author to put a loop lookup into this_irq_is_affected(). Fair enough, though you have to admit that the design of the gic_arch_extn actually enforces that, if you can't do a simple "if irq [<>] n" check for whatever reason. The alternative approach to that is to use different irq chip implementations for interrupts affected by gic_arch_extn and those which are not affected as we do in lot of other places do deal with the subtle differences of particular interrupt lines. That definitely would avoid that people try to stick more complex decision functions than "irq [<>] n" into this_irq_is_affected(). Now looking at the locking szenario of GIC, it might eventually create quite some duplicated code, which is undesirable as well. OTOH, the locking requirements especially if I look at gic_[un]mask_irq and gic_eoi_irq are not entirely clear to me from the code. gic_[un]mask_irq(d) { mask = 1 << SFT(gic_irq(d)); lock(controller_lock); if (gic_arch_extn.irq_[un]mask) gic_arch_extn.irq_[un]mask(d); writel(mask, GIC_ENABLE_[CLEAR|SET]); unlock(controller_lock); } while gic_eoi_irq(d) { if (gic_arch_extn.irq_eoi) { lock(controller_lock); gic_arch_extn.irq_eoi(d); unlock(controller_lock); } writel(gic_irq(d), GIC_EOI); } So is there a requirement to serialize the mask/unmask operations for a particular interrupt line against mask/unmask operations on a different core on some other interrupt line? The operations for a particular interrupt line are already serialized at the core code level. The CLEAR/SET registers are designed to avoid locking in contrary to the classic ENABLE reg, where you have to maintain consistency of the full set of interrupts affected by that register. So for the case where gic_arch_extn is not used, the locking is completely pointless, right? Or is this locking required to maintain consistency between the gic_arch_extn.[un]mask and the GIC.[un]mask itself? And even if the locking is required for this, then having two separate chips with two different callbacks makes sense. gic_mask_irq() { writel(mask, GIC_ENABLE_CLEAR); } gic_mask_extn_irq(d) { lock(controller_lock); gic_arch_extn.irq_mask(d); gic_mask_irq(d); unlock(controller_lock); } And then have the gic_chip and gic_extn_chip set for the various interrupt lines. That avoids several things: 1) The locking for non gic_arch_extn interrupts 2) Two conditionals for gic_arch_extn interrupts 3) The enforcement of adding complex decision functions into the gic_extn functions if there is no simple x < N check possible. I might have missed something as always, but at least I did a proper analysis of the code as it is understandable to a mere mortal. Thoughts? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html