linux-kernel - Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

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]

Message-ID: <87a5ho4hb6.ffs@tglx>
Date: Wed, 07 Aug 2024 15:02:21 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Matti Vaittinen <mazziesaccount@...il.com>, Matti Vaittinen
 <matti.vaittinen@...rohmeurope.com>
Cc: Mark Brown <broonie@...nel.org>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

Matti!

On Tue, Aug 06 2024 at 14:51, Matti Vaittinen wrote:
> On 7/13/24 15:22, Thomas Gleixner wrote:
>> Something like the untested below should work. That would make your
>> info:
>> 
>> 	struct irq_domain_info info = {
>> 		.fwnode		= fwnode,
>> 		.size		= chip->num_irqs,
>
> Based on my code reading, the .size is used for allocating the "revmap". 
> Looking at the info struct for existing implementation of the 
> irq_domain_create_legacy(), the .size is set as:
>
> .size		= first_hwirq + size,
>
>> 		.hwirq_max	= chip->num_irqs,
>
> Also, the irq_domain_create_legacy() sets hwirq_max as:
>
> .hwirq_max	= first_hwirq + size.
>
> see:
>
>  > @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
>  >   		.fwnode		= fwnode,
>  >   		.size		= first_hwirq + size,
>  >   		.hwirq_max	= first_hwirq + size,
>  > +		.hwirq_base	= first_hwirq,
>  > +		.virq_base	= first_irq,
>  >   		.ops		= ops,
>  >   		.host_data	= host_data,
>  >   	};
>  > -	struct irq_domain *domain;
>  > +	struct irq_domain *domain = irq_domain_instantiate(&info);
>  >
>  > -	domain = irq_domain_instantiate(&info);
>  > -	if (IS_ERR(domain))
>  > -		return NULL;
>  > -
>  > -	irq_domain_associate_many(domain, first_irq, first_hwirq, size);
>
> Lookin at this, the existing code calls irq_domain_associate_many() with 
> the given size parameter (without the + first_hwirq which is assigned to 
> .size).

Indeed.

> I think this is not aligned with what the patch below results (and yes, 
> I know Thomas told it's untested).

:)

> I'd better admit I am not 100% sure how the legacy domains work and that 
> I don't (any more) fully trust on my ability to flawlessly interpret the 
> code ;)

You definitely did better than me :)

> Hence I'd rather learn from a small explanation (what is the 
> expected .size) than by fixing this after I see regression reports from 
> real users of the irq_domain_create_legacy() :)

So the size of the domain is sum of the parameters @size and
@first_hwirq. That's so that the hardware interrupt is zero indexed for
an array based lookup.

The association obviously wants only the @size parameter because that's
what the caller wants interrupts for as it obviously can't provide
interrupts below @first_hwirq.

> So, any guidance as to what the revmap allocation size should be (the 
> info->size), and what should be the size for the 
> irq_domain_associate_many()?

So that associate should be:

   irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
   			     info->size - info->hwirq_base);

Thanks,

        tglx