linux-kernel - Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

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] [day] [month] [year] [list]

Message-ID: <871pvidvzv.ffs@tglx>
Date: Fri, 28 Feb 2025 14:44:36 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Yixun Lan <dlan@...too.org>, Alex Elder <elder@...cstar.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
 <brgl@...ev.pl>, Inochi Amaoto <inochiama@...il.com>,
 linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
 linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

On Thu, Feb 27 2025 at 20:41, Yixun Lan wrote:
> On 10:12 Thu 27 Feb     , Alex Elder wrote:
>> On 2/27/25 5:24 AM, Yixun Lan wrote:
>> > 
>> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> > index ec6d8e72d980f604ded2bfa2143420e0e0095920..cb874ab5e54a4763d601122becd63b6d759e55d2 100644
>> > --- a/kernel/irq/irqdomain.c
>> > +++ b/kernel/irq/irqdomain.c
>> > @@ -1208,10 +1208,17 @@ int irq_domain_translate_twocell(struct irq_domain *d,
>> >   				 unsigned long *out_hwirq,
>> >   				 unsigned int *out_type)
>> >   {
>> 
>> This function is meant for "twocell".  There is also another function
>> irq_domain_translate_onecell().  Why don't you just create
>> irq_domain_translate_threecell" instead?
>> 
> good question!
>
> it's too many changes for adding "threecell" which I thought not worth
> the effort, or maybe we can rename the function to *twothreecell()?
>
> I'm not sure which way to go is the best, ideas from maintainer are
> welcome

We really want to have explicit functions for two and three cells.
 
>> > +	u32 irq, type;
>> > +
>> >   	if (WARN_ON(fwspec->param_count < 2))
>> >   		return -EINVAL;
>> > -	*out_hwirq = fwspec->param[0];
>> > -	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> > +
>> > +	irq = fwspec->param_count - 2;
>> > +	type = fwspec->param_count - 1;
> no matter two or three cell, it's always parse the last two cells,
> virtually they are same syntax, which can reuse the *_translate_twocell()
> function perfectly..

Yes, that works but the code is completely non-obvious. So what you
really want is something like this:

int irq_domain_translate_cells(struct irq_domain *d, unsigned long *hwirq,
			       unsigned int *type)
{
        unsigned int cells = fwspec->param_count;

        switch (cells) {
       	case 1:
                *hwirq = fwspec->param[0];
                *type = IRQ_TYPE_NONE;
                return 0;
       	case 2..3:
	        /*
        	 * For multi cell translations the hardware interrupt number and type
	         * are in the last two cells.
	         */
		*hwirq = fwspec->param[cells - 2];
        	*type = fwspec->param[cells - 1] & IRQ_TYPE_SENSE_MASK;
                return 0;
        default:
   		return -EINVAL;
        }
}

Then have inline helpers:

static inline int irq_domain_translate_XXXcell(struct irq_domain *d, unsigned long *hwirq,
					       unsigned int *type)
{
        return irq_domain_translate_cells(d, hwirq, type);
}

That avoids changing all call sites at once and merges the one cell
translation into it.

You get the idea....

Thanks,

        tglx