Message-ID: <544E67D8.3020504@arm.com>
Date:	Mon, 27 Oct 2014 15:42:16 +0000
From:	Marc Zyngier <marc.zyngier@....com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Jason Cooper <jason@...edaemon.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/3] genirq: Add support for priority-drop/deactivate
 interrupt controllers

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists