Re: [PATCH v2 1/3] irqdomain: simplify simple and legacy domain creation

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 8/13/24 13:19, Jiaxun Yang wrote:
> 
> 
> On 2024/8/8 13:34, Matti Vaittinen wrote:
>> Move a bit more logic in the generic __irq_domain_instantiate() from the
>> irq_domain_create_simple() and the irq_domain_create_legacy(). This does
>> simplify the irq_domain_create_simple() and irq_domain_create_legacy().
>> It will also ease the use of irq_domain_instantiate() instead of the
>> irq_domain_create_simple() or irq_domain_create_legacy() by allowing the
>> callers of irq_domain_instantiate() to omit the IRQ association and
>> irq_desc allocation code.
>>
>> Reduce code duplication by introducing the hwirq_base and virq_base
>> members in the irq_domain_info structure, creating helper function
>> for allocating irq_descs, and moving logic from the .._legacy() and
>> the .._simple() to the more generic irq_domain_instantiate().
> 
> Hi all,
> 
> This patch currently in next had caused regression on MIPS systems.
> 
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/irq/chip.c:1008 
> __irq_set_handler+0x64/0xa8
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 
> 6.11.0-rc3-next-20240812 #10
> [    0.000000] Hardware name: img,boston
> [    0.000000] Stack : 0000000000000034 0000000000000000 
> 0000000000000018 ffffffff80cbba58
> [    0.000000]         ffffffff80cbba58 ffffffff80cbbb88 
> 0000000000000000 0000000000000000
> [    0.000000]         0000000000000000 0000000000000001 
> 0000000000000000 0000000000000000
> [    0.000000]         0000000000000000 0000000000000000 
> ffffffff80a7ed04 0000000000002000
> [    0.000000]         ffffffff80cbb824 0000000000000000 
> 0000000000000000 ffffffff80c35aa0
> [    0.000000]         ffffffff80ce0000 ffffffff80ce0000 
> 0000000000000000 0000000000000100
> [    0.000000]         ffffffff80ce0000 0000000000000000 
> ffffffffffffffff 0000000016200001
> [    0.000000]         ffffffffb6100088 ffffffff80cb8000 
> ffffffff80cbba50 ffffffff80c90000
> [    0.000000]         ffffffff801096d4 0000000000000000 
> 0000000000000000 0000000000000000
> [    0.000000]         0000000000000000 0000000000000000 
> ffffffff801096ec 0000000000000000
> [    0.000000]         ...
> [    0.000000] Call Trace:
> [    0.000000] [<ffffffff801096ec>] show_stack+0x5c/0x150
> [    0.000000] [<ffffffff80a8e2e4>] dump_stack_lvl+0x6c/0xb4
> [    0.000000] [<ffffffff80a80e20>] __warn+0x9c/0xcc
> [    0.000000] [<ffffffff80135bd8>] warn_slowpath_fmt+0x158/0x1c0
> [    0.000000] [<ffffffff801a4d04>] __irq_set_handler+0x64/0xa8
> [    0.000000] [<ffffffff80dba50c>] gic_of_init+0x2a8/0x55c
> [    0.000000] [<ffffffff80dc4430>] of_irq_init+0x1c8/0x324
> [    0.000000] [<ffffffff80da092c>] init_IRQ+0x60/0x120
> [    0.000000] [<ffffffff80d9dbd4>] start_kernel+0x4dc/0x658
> [    0.000000]
> [    0.000000] ---[ end trace 0000000000000000 ]---
> 
> This might be caused by drivers/irqchip/irq-mips-cpu.c has 
> irq_domain_add_legacy() called with
> first_irq set to 0.
> 

Right. Thanks for the report! I do appreciate this testing!

first_irq gets assigned to the info->virq_base making it 0.

Later, in the __irq_domain_instantiate() we use the info->virq_base as a 
condition for
	if (info->virq_base > 0)
                 irq_domain_associate_many()
which was previously unconditionally called in the irq_domain_add_legacy().

> Do you have any idea on how should we fix it?
Maybe we could add a flag for domain association similar to the "bool 
cond_alloc_descs", but maybe Thomas has some better ideas...?


Yours,
	-- Matti
-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~