Re: [RFC PATCH] genirq: Avoid unnecessary low level irq function calls

[Thomas Gleixner]

Jeffy,

On Mon, 26 Jun 2017, Jeffy Chen wrote:
>  void irq_enable(struct irq_desc *desc)
>  {
> -     irq_state_clr_disabled(desc);
> -     if (desc->irq_data.chip->irq_enable)
> -             desc->irq_data.chip->irq_enable(&desc->irq_data);
> -     else
> -             desc->irq_data.chip->irq_unmask(&desc->irq_data);
> -     irq_state_clr_masked(desc);
> +     if (irqd_is_started(&desc->irq_data) &&
> +         !irqd_irq_disabled(&desc->irq_data)) {
> +             unmask_irq(desc);

I'm having a hard time to understand the logic here.

    if (started && !disabled)
        unmask()

That does not make any sense. If you need that to work around a state
inconsistency then it needs to be addressed there and not worked around
here.

> +     } else {
> +             irq_state_clr_disabled(desc);
> +             if (desc->irq_data.chip->irq_enable) {
> +                     desc->irq_data.chip->irq_enable(&desc->irq_data);
> +                     irq_state_clr_masked(desc);
> +             } else {
> +                     unmask_irq(desc);
> +             }
> +     }
>  }
>  
>  static void __irq_disable(struct irq_desc *desc, bool mask)
>  {
> -     irq_state_set_disabled(desc);
> -     if (desc->irq_data.chip->irq_disable) {
> -             desc->irq_data.chip->irq_disable(&desc->irq_data);
> -             irq_state_set_masked(desc);
> -     } else if (mask) {
> -             mask_irq(desc);
> +     if (irqd_irq_disabled(&desc->irq_data)) {
> +             if (mask)
> +                     mask_irq(desc);
> +     } else {
> +             irq_state_set_disabled(desc);
> +             if (desc->irq_data.chip->irq_disable) {
> +                     desc->irq_data.chip->irq_disable(&desc->irq_data);
> +                     irq_state_set_masked(desc);
> +             } else if (mask) {
> +                     mask_irq(desc);
> +             }
>       }
>  }
>  
> @@ -311,18 +322,21 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned 
> int cpu)
>  
>  static inline void mask_ack_irq(struct irq_desc *desc)
>  {
> -     if (desc->irq_data.chip->irq_mask_ack)
> +     if (desc->irq_data.chip->irq_mask_ack) {
>               desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
> -     else {
> -             desc->irq_data.chip->irq_mask(&desc->irq_data);
> +             irq_state_set_masked(desc);
> +     } else {
> +             mask_irq(desc);
>               if (desc->irq_data.chip->irq_ack)
>                       desc->irq_data.chip->irq_ack(&desc->irq_data);
>       }
> -     irq_state_set_masked(desc);
>  }
>  
>  void mask_irq(struct irq_desc *desc)
>  {
> +     if (irqd_irq_masked(&desc->irq_data))
> +             return;
> +
>       if (desc->irq_data.chip->irq_mask) {
>               desc->irq_data.chip->irq_mask(&desc->irq_data);
>               irq_state_set_masked(desc);
> @@ -331,6 +345,10 @@ void mask_irq(struct irq_desc *desc)
>  
>  void unmask_irq(struct irq_desc *desc)
>  {
> +     if (irqd_is_started(&desc->irq_data) &&
> +         !irqd_irq_masked(&desc->irq_data))
> +             return;

Ditto.

Thanks,

        tglx