linux-kernel - Re: [PATCH v2] irq domain: drop IRQ_DOMAIN

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: <24da75fa-b0f9-b9ff-5b34-42ae6fc2ba24@infradead.org>
Date:   Thu, 23 Mar 2023 23:11:02 -0700
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Cc:     Marc Zyngier <maz@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        linux-arch@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2] irq domain: drop IRQ_DOMAIN_HIERARCHY option, make it
 always on

Hi Thomas,

On 3/23/23 13:53, Thomas Gleixner wrote:
> On Sun, Mar 12 2023 at 19:39, Randy Dunlap wrote:
>> In preparation for dropping the IRQ_DOMAIN Kconfig option (effectively
>> making it always set/on), first drop IRQ_DOMAIN_HIERARCHY as an option,
>> making its code always set/on.
>>
>> This has been built successfully on all ARCHes except hexagon,
>> both 32-bit and 64-bit where applicable.
> 
> I really like where this is going, but reviewing this is a pain. I tried
> to split it up into more digestable pieces:
> 
>    https://tglx.de/~tglx/patches.tar
> 
> That's not completely equivalent to your patch as I did some of the
> changes below. It builds on various oddball architectures with
> IRQ_DOMAIN=n, but is otherwise completely untested.
> 
> It should be actually trivial after that to make IRQ_DOMAIN def_bool y
> and then gradually remove the IRQ_DOMAIN selects and ifdeffery.

Yeah, that may be the best & simplest approach.

Or just use your patches.tar.

>> v2: add stubs in include/linux/irqdomain.h for the config case of
>> IRQ_DOMAIN is not set. If these are not added, there will be plenty
>> of build errors (not so much for modern arches as for older ones).
> 
> I'm not really convinced that all of these stubs are required. Why would
> there suddenly be a requirement to expose stubs for functions which
> depend on CONFIG_IRQ_DOMAIN=y already today just by removing the
> hierarchy config?

All of those stubs were added because I had configs/builds that were
failing due to them. I didn't just add them for fun or "completeness."

I just checked and I didn't save all of those failing configs or
output files that contain the errors.


> Even exposing stubs for functions which have been only available via
> CONFIG_IRQ_DOMAIN_HIERARCHY is questionable simply because there cannot
> be any code which invokes them unconditionally if
> CONFIG_IRQ_DOMAIN_HIERARCHY=n today.
> 
> IOW, the sum of required stubs cannot be larger than number of stubs
> required today.
> 
> If there is code which has a #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY then
> this needs to be changed to CONFIG_IRQ_DOMAIN or the required functions
> have to be exposed unconditionally, right?

Yes, s/CONFIG_IRQ_DOMAIN_HIERARCHY/CONFIG_IRQ_DOMAIN/ in code.


>> diff -- a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -361,7 +361,7 @@ config GPIO_IXP4XX
>>  	depends on OF
>>  	select GPIO_GENERIC
>>  	select GPIOLIB_IRQCHIP
>> -	select IRQ_DOMAIN_HIERARCHY
>> +	select IRQ_DOMAIN
> 
> IRQ_DOMAIN is already selected by GPIOLIB_IRQCHIP, so this select is
> redundant for all GPIO configs which select GPIOLIB_IRQCHIP.

Ack.

I'm perfectly happy to use either Marc's patch that he posted on
2023-FEB-14 (https://lore.kernel.org/all/86y1p0xbqd.wl-maz@kernel.org/)
or your patches. Both of you know your way around this better than I do.

Thanks for your review and feedback.

-- 
~Randy