[PATCH] irqdomain: factorise irq_domain_xlate_onetwocell()

[PATCH] irqdomain: factorise irq_domain_xlate_onetwocell()

[Sebastian Frias]

Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use") introduced three similar functions:

irq_domain_xlate_onecell()
irq_domain_xlate_twocell()
irq_domain_xlate_onetwocell()

yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the
two previous ones to avoid code duplication.

Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---

NOTE: the factored code is not strictly the same in the sense that
16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would
make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

Feel free to comment on that matter.

---
 kernel/irq/irqdomain.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index bee8b02..ea2df0e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -839,9 +839,20 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 {
 	if (WARN_ON(intsize < 1))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
-	return 0;
+	if (intsize == 1)
+		return irq_domain_xlate_onecell(d,
+						ctrlr,
+						intspec,
+						intsize,
+						out_hwirq,
+						out_type);
+	else
+		return irq_domain_xlate_twocell(d,
+						ctrlr,
+						intspec,
+						intsize,
+						out_hwirq,
+						out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);
 
-- 
1.7.11.2

[PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell()

[Sebastian Frias]

Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use") introduced three similar functions:

irq_domain_xlate_onecell()
irq_domain_xlate_twocell()
irq_domain_xlate_onetwocell()

yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the
two previous ones to avoid code duplication.

Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---

NOTE: the factored code is not strictly the same in the sense that
16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would
make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

Feel free to comment on that matter.

---
 kernel/irq/irqdomain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index bee8b02..125a28c 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 {
 	if (WARN_ON(intsize < 1))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
-	return 0;
+	if (intsize == 1)
+		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
+	else
+		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);
 
-- 
1.7.11.2

Re: [PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell()

[Thomas Gleixner]

On Mon, 1 Aug 2016, Sebastian Frias wrote:
> Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use") introduced three similar functions:
> 
> irq_domain_xlate_onecell()
> irq_domain_xlate_twocell()
> irq_domain_xlate_onetwocell()
> 
> yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the
> two previous ones to avoid code duplication.
> 
> Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use")

That does not fix anything. It optimizes code. We use the "Fixes" tag only
when the existing code is buggy.

> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
> 
> NOTE: the factored code is not strictly the same in the sense that
> 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would
> make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

So the proper way to do that is to split this into two patches:

 #1 Change the existing code to do the masking and explain why it is correct.

 #2 Refactor the code and get rid of the duplicated implementation.

 
> Feel free to comment on that matter.
> 
> ---
>  kernel/irq/irqdomain.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index bee8b02..125a28c 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
>  {
>  	if (WARN_ON(intsize < 1))
>  		return -EINVAL;
> -	*out_hwirq = intspec[0];
> -	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
> -	return 0;
> +	if (intsize == 1)
> +		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
> +						out_hwirq, out_type);
> +	else
> +		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
> +						out_hwirq, out_type);

So I really wonder how much of a saving that change is. I wouldn't be
surprised if it would create worse code on some architectures.

Thanks,

	tglx

Re: [PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell()

[Sebastian Frias]

Hi Thomas,

On 08/01/2016 07:07 PM, Thomas Gleixner wrote:
> On Mon, 1 Aug 2016, Sebastian Frias wrote:
>> Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
>> drivers can use") introduced three similar functions:
>>
>> irq_domain_xlate_onecell()
>> irq_domain_xlate_twocell()
>> irq_domain_xlate_onetwocell()
>>
>> yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the
>> two previous ones to avoid code duplication.
>>
>> Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
>> drivers can use")
> 
> That does not fix anything. It optimizes code. We use the "Fixes" tag only
> when the existing code is buggy.

Ok, I will remove that.

> 
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>>
>> NOTE: the factored code is not strictly the same in the sense that
>> 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would
>> make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".
> 
> So the proper way to do that is to split this into two patches:
> 
>  #1 Change the existing code to do the masking and explain why it is correct.
> 
>  #2 Refactor the code and get rid of the duplicated implementation.

Ok, I can do two patches.

> 
>  
>> Feel free to comment on that matter.
>>
>> ---
>>  kernel/irq/irqdomain.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index bee8b02..125a28c 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
>>  {
>>  	if (WARN_ON(intsize < 1))
>>  		return -EINVAL;
>> -	*out_hwirq = intspec[0];
>> -	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
>> -	return 0;
>> +	if (intsize == 1)
>> +		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
>> +						out_hwirq, out_type);
>> +	else
>> +		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
>> +						out_hwirq, out_type);
> 
> So I really wonder how much of a saving that change is. I wouldn't be
> surprised if it would create worse code on some architectures.
> 

Maybe it does, although I looked at this from the point of view of reducing
duplicated code because of the well known issues duplicated code entails.
This case is a good example, since the code was duplicated we ended up with
slightly different versions of it.

Best regards,

Sebastian

Re: [PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell()

[Sebastian Frias]

Hi Thomas,

On 08/02/2016 10:31 AM, Sebastian Frias wrote:
>> So the proper way to do that is to split this into two patches:
>>
>>  #1 Change the existing code to do the masking and explain why it is correct.
>>
>>  #2 Refactor the code and get rid of the duplicated implementation.
> 
> Ok, I can do two patches.

I splitted it in two patches, one to fix it, and another one to refactor.
However, I sent the first one (the one for the fix) as a separate one, instead of as
part of a set of two patches.

I'm resending both as a set of two patches (since the second one requires the first
one), sorry for the inconvenience.

Best regards,

Sebastian