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

 
> 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