linux-kernel - Re: [PATCH v2] irqdomain: factorise irq_domain_xlate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 1 Aug 2016 19:07:37 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Sebastian Frias <sf84@...oste.net>
cc:	Grant Likely <grant.likely@...retlab.ca>,
	Marc Zyngier <marc.zyngier@....com>,
	Jason Cooper <jason@...edaemon.net>,
	LKML <linux-kernel@...r.kernel.org>, Mason <slash.tmp@...e.fr>
Subject: Re: [PATCH v2] irqdomain: factorise irq_domain_xlate_onetwocell()

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