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@xxxxxxxxxxx>
>> ---
>>
>> 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