Re: [PATCH 1/3] genirq: Add support for priority-drop/deactivate interrupt controllers

[Thomas Gleixner]

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.

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

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

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

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

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.

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? 

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/