Hi Thomas,
Thanks for looking into this.
On 25/10/14 21:27, Thomas Gleixner wrote:
> On Sat, 25 Oct 2014, Marc Zyngier wrote:
>
> @@ -330,6 +330,7 @@ struct irq_chip {
> void (*irq_mask)(struct irq_data *data);
> void (*irq_mask_ack)(struct irq_data *data);
> void (*irq_unmask)(struct irq_data *data);
> + void (*irq_priority_drop)(struct irq_data *data);
>
> Lacks the docbook comment.
Yup, will add.
>> +static void mask_threaded_irq(struct irq_desc *desc)
>
> There is only one caller for this, i.e handle_fasteoi_irq, right? So
> this should go to the other eoi handler specific helpers and have eoi
> in its name.
I was seeing it as the pendent of unmask_threaded_irq(). But reading
below, you seem to have a very different approach
>> +{
>> + struct irq_chip *chip = desc->irq_data.chip;
>> +
>> + /* If we can do priority drop, then masking comes for free */
>> + if (chip->irq_priority_drop)
>> + irq_state_set_masked(desc);
>> + else
>> + mask_irq(desc);
>> +}
>
>> void unmask_irq(struct irq_desc *desc)
>> {
>> - if (desc->irq_data.chip->irq_unmask) {
>> - desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> + struct irq_chip *chip = desc->irq_data.chip;
>> +
>> + if (chip->irq_unmask && !chip->irq_priority_drop)
>> + chip->irq_unmask(&desc->irq_data);
>
> I have a hard time to understand that logic. Assume the interrupt
> being masked at the hardware level after boot. Now at request_irq()
> time what is going to unmask that very interrupt? Ditto for masking
> after disable_irq(). Probably not what you really want.
Peering at the code (and assuming I'm finally awake), request_irq() uses
irq_startup() -> irq_enable() -> chip->irq_unmask().
But you're perfectly right, it breaks an independent use of
unmask_irq(), which is pretty bad.
>> +static void eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>> +{
>> + if (chip->irq_priority_drop)
>> + chip->irq_priority_drop(&desc->irq_data);
>> + if (chip->irq_eoi)
>> + chip->irq_eoi(&desc->irq_data);
>> +}
>
> So if you are using that priority drop stuff, you need both calls even
> for the non threaded case?
Yes. This is a global property (all interrupt lines for this irqchip are
affected), so even the non-threaded case has to issue both calls.
>> static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip
>> *chip)
>> {
>> if (!(desc->istate & IRQS_ONESHOT)) {
>> - chip->irq_eoi(&desc->irq_data);
>> + eoi_irq(desc, chip);
>> return;
>> }
>> +
>> + if (chip->irq_priority_drop)
>> + chip->irq_priority_drop(&desc->irq_data);
>> +
>> /*
>> * We need to unmask in the following cases:
>> * - Oneshot irq which did not wake the thread (caused by a
>> @@ -485,7 +507,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc,
>> struct irq_chip *chip)
>> if (!irqd_irq_disabled(&desc->irq_data) &&
>> irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
>> chip->irq_eoi(&desc->irq_data);
>> - unmask_irq(desc);
>> + if (!chip->irq_priority_drop)
>> + unmask_irq(desc);
>
> This is really completely obfuscated: Brain starts melting and
> spiraling towards some unidentified universe.
Ah! I'm glad I'm not the only one with that feeling ;-).
> Seriously, I don't think it's a good idea to bandaid this
> functionality into the existing handle_fasteoi_irq() mechanism. It's
> complex enough already.
That was the other option. I may have to duplicate (or tweak)
handle_percpu_devid_irq as well though.
> So what you really want is a separate handler for this. But aside of
> adding the drop prio callback you probably want to handle the other
> existing callbacks completely differently than for the regular mode of
> that irq controller.
>
> Can you please explain detailed how this "priority drop" mode
> works?
The basics of this mode are pretty simple:
- Interrupt signalled, CPU enter the GIC code
- Read the IAR register, interrupt becomes active:
-> no other interrupt can be taken
- Run whatever interrupt handler
- Write to the EOI register:
-> interrupt is still active, and cannot be taken again, but other
interrupts can now be taken
- Write to the DIR register:
-> interrupt is now inactive, and can be taken again.
A few interesting things here:
- EOI (which causes priority drop) acts as a mask
- DIR (which causes deactivate) acts as unmask+EOI
To me, it looks like DIR operation is exactly what we need when running
a threaded interrupt with IRQCHIP_EOI_THREADED, saving the whole
mask/unmask that is rather slow on ARM.
With that in mind, I end up mapping mask to priority_drop_irq (write to
EOI), and unmask to eoi_irq (write to DIR). Which is admittedly an
interesting brainfuck when trying to wire it into the existing framework.
So yeah, having a different handler will make it much simpler. My main
concern is how to plug this "elegantly" into the epilogue for a threaded
interrupt (irq_finalize_oneshot).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
