All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] genirq: Avoid unnecessary low level irq function calls
@ 2017-06-26  6:12 Jeffy Chen
  2017-06-26  8:20 ` Thomas Gleixner
  0 siblings, 1 reply; 3+ messages in thread
From: Jeffy Chen @ 2017-06-26  6:12 UTC (permalink / raw)
  To: linux-kernel, tglx; +Cc: briannorris, dianders, tfiga, Jeffy Chen

Check irq state in enable/disable/unmask/mask_irq to avoid unnecessary
low level irq function calls.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 kernel/irq/chip.c | 55 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 8a5b597..389cfdf 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -247,22 +247,33 @@ void irq_shutdown(struct irq_desc *desc)
 
 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);
+	} 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;
+
 	if (desc->irq_data.chip->irq_unmask) {
 		desc->irq_data.chip->irq_unmask(&desc->irq_data);
 		irq_state_clr_masked(desc);
@@ -344,10 +362,7 @@ void unmask_threaded_irq(struct irq_desc *desc)
 	if (chip->flags & IRQCHIP_EOI_THREADED)
 		chip->irq_eoi(&desc->irq_data);
 
-	if (chip->irq_unmask) {
-		chip->irq_unmask(&desc->irq_data);
-		irq_state_clr_masked(desc);
-	}
+	unmask_irq(desc);
 }
 
 /*
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [RFC PATCH] genirq: Avoid unnecessary low level irq function calls
  2017-06-26  6:12 [RFC PATCH] genirq: Avoid unnecessary low level irq function calls Jeffy Chen
@ 2017-06-26  8:20 ` Thomas Gleixner
  2017-06-26  9:57   ` jeffy
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Gleixner @ 2017-06-26  8:20 UTC (permalink / raw)
  To: Jeffy Chen; +Cc: linux-kernel, briannorris, dianders, tfiga

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [RFC PATCH] genirq: Avoid unnecessary low level irq function calls
  2017-06-26  8:20 ` Thomas Gleixner
@ 2017-06-26  9:57   ` jeffy
  0 siblings, 0 replies; 3+ messages in thread
From: jeffy @ 2017-06-26  9:57 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: linux-kernel, briannorris, dianders, tfiga

Hi Thomas,

thanx for your comments.

On 06/26/2017 04:20 PM, Thomas Gleixner wrote:
> 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.
right, we already set disabled state in desc_set_defaults, so the 
disabled state should be consistency here. so just
if(!disabled)
   unmask()
else
   enable()
?

and i saw we didn't set masked state in desc_set_defaults, i'll send a 
patch for that, and remove unmask_irq's started check too.
>
>> +	} 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
>
>
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-06-26  9:58 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-26  6:12 [RFC PATCH] genirq: Avoid unnecessary low level irq function calls Jeffy Chen
2017-06-26  8:20 ` Thomas Gleixner
2017-06-26  9:57   ` jeffy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.