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().

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>

---
Revision history:
v1 => v2:
 - New patch

Please note that this patch has received only limited testing. Any and
all testing with devices using the legacy domains is greatly appreciated
:)
---
 include/linux/irqdomain.h |  5 +++
 kernel/irq/irqdomain.c    | 73 +++++++++++++++++++++------------------
 2 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index de6105f68fec..bfcffa2c7047 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -291,6 +291,9 @@ struct irq_domain_chip_generic_info;
  * @hwirq_max:		Maximum number of interrupts supported by controller
  * @direct_max:		Maximum value of direct maps;
  *			Use ~0 for no limit; 0 for no direct mapping
+ * @hwirq_base:		The first hardware interrupt number (legacy domains only)
+ * @virq_base:		The first Linux interrupt number for legacy domains to
+ *			immediately associate the interrupts after domain creation
  * @bus_token:		Domain bus token
  * @ops:		Domain operation callbacks
  * @host_data:		Controller private data pointer
@@ -307,6 +310,8 @@ struct irq_domain_info {
 	unsigned int				size;
 	irq_hw_number_t				hwirq_max;
 	int					direct_max;
+	unsigned int				hwirq_base;
+	unsigned int				virq_base;
 	enum irq_domain_bus_token		bus_token;
 	const struct irq_domain_ops		*ops;
 	void					*host_data;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index cea8f6874b1f..5af5e4028de2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_domain *domain)
 	kfree(domain);
 }
 
-/**
- * irq_domain_instantiate() - Instantiate a new irq domain data structure
- * @info: Domain information pointer pointing to the information for this domain
- *
- * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
- */
-struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
+static void irq_domain_instantiate_descs(const struct irq_domain_info *info)
+{
+	if (!IS_ENABLED(CONFIG_SPARSE_IRQ))
+		return;
+
+	if (irq_alloc_descs(info->virq_base, info->virq_base, info->size,
+			    of_node_to_nid(to_of_node(info->fwnode))) < 0) {
+		pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
+			info->virq_base);
+	}
+}
+
+static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info *info,
+						   bool cond_alloc_descs)
 {
 	struct irq_domain *domain;
 	int err;
@@ -306,6 +313,14 @@ struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
 
 	__irq_domain_publish(domain);
 
+	if (cond_alloc_descs && info->virq_base > 0)
+		irq_domain_instantiate_descs(info);
+
+	/* Legacy interrupt domains have a fixed Linux interrupt number */
+	if (info->virq_base > 0)
+		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
+					  info->size - info->hwirq_base);
+
 	return domain;
 
 err_domain_gc_remove:
@@ -315,6 +330,17 @@ struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
 	irq_domain_free(domain);
 	return ERR_PTR(err);
 }
+
+/**
+ * irq_domain_instantiate() - Instantiate a new irq domain data structure
+ * @info: Domain information pointer pointing to the information for this domain
+ *
+ * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
+ */
+struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
+{
+	return __irq_domain_instantiate(info, false);
+}
 EXPORT_SYMBOL_GPL(irq_domain_instantiate);
 
 /**
@@ -413,28 +439,13 @@ struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
 		.fwnode		= fwnode,
 		.size		= size,
 		.hwirq_max	= size,
+		.virq_base	= first_irq,
 		.ops		= ops,
 		.host_data	= host_data,
 	};
-	struct irq_domain *domain;
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
 
-	domain = irq_domain_instantiate(&info);
-	if (IS_ERR(domain))
-		return NULL;
-
-	if (first_irq > 0) {
-		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
-			/* attempt to allocated irq_descs */
-			int rc = irq_alloc_descs(first_irq, first_irq, size,
-						 of_node_to_nid(to_of_node(fwnode)));
-			if (rc < 0)
-				pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
-					first_irq);
-		}
-		irq_domain_associate_many(domain, first_irq, 0, size);
-	}
-
-	return domain;
+	return IS_ERR(domain) ? NULL : domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_simple);
 
@@ -476,18 +487,14 @@ struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
 		.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);
-
-	return domain;
+	return IS_ERR(domain) ? NULL : domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_legacy);
 
-- 
2.45.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

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.

Do you have any idea on how should we fix it?

Thanks
- Jiaxun

>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>
> ---
> Revision history:
> v1 => v2:
>   - New patch
>
> Please note that this patch has received only limited testing. Any and
> all testing with devices using the legacy domains is greatly appreciated
> :)
> ---
>   include/linux/irqdomain.h |  5 +++
>   kernel/irq/irqdomain.c    | 73 +++++++++++++++++++++------------------
>   2 files changed, 45 insertions(+), 33 deletions(-)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index de6105f68fec..bfcffa2c7047 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -291,6 +291,9 @@ struct irq_domain_chip_generic_info;
>    * @hwirq_max:		Maximum number of interrupts supported by controller
>    * @direct_max:		Maximum value of direct maps;
>    *			Use ~0 for no limit; 0 for no direct mapping
> + * @hwirq_base:		The first hardware interrupt number (legacy domains only)
> + * @virq_base:		The first Linux interrupt number for legacy domains to
> + *			immediately associate the interrupts after domain creation
>    * @bus_token:		Domain bus token
>    * @ops:		Domain operation callbacks
>    * @host_data:		Controller private data pointer
> @@ -307,6 +310,8 @@ struct irq_domain_info {
>   	unsigned int				size;
>   	irq_hw_number_t				hwirq_max;
>   	int					direct_max;
> +	unsigned int				hwirq_base;
> +	unsigned int				virq_base;
>   	enum irq_domain_bus_token		bus_token;
>   	const struct irq_domain_ops		*ops;
>   	void					*host_data;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index cea8f6874b1f..5af5e4028de2 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_domain *domain)
>   	kfree(domain);
>   }
>   
> -/**
> - * irq_domain_instantiate() - Instantiate a new irq domain data structure
> - * @info: Domain information pointer pointing to the information for this domain
> - *
> - * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
> - */
> -struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
> +static void irq_domain_instantiate_descs(const struct irq_domain_info *info)
> +{
> +	if (!IS_ENABLED(CONFIG_SPARSE_IRQ))
> +		return;
> +
> +	if (irq_alloc_descs(info->virq_base, info->virq_base, info->size,
> +			    of_node_to_nid(to_of_node(info->fwnode))) < 0) {
> +		pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> +			info->virq_base);
> +	}
> +}
> +
> +static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info *info,
> +						   bool cond_alloc_descs)
>   {
>   	struct irq_domain *domain;
>   	int err;
> @@ -306,6 +313,14 @@ struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
>   
>   	__irq_domain_publish(domain);
>   
> +	if (cond_alloc_descs && info->virq_base > 0)
> +		irq_domain_instantiate_descs(info);
> +
> +	/* Legacy interrupt domains have a fixed Linux interrupt number */
> +	if (info->virq_base > 0)
> +		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
> +					  info->size - info->hwirq_base);
> +
>   	return domain;
>   
>   err_domain_gc_remove:
> @@ -315,6 +330,17 @@ struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
>   	irq_domain_free(domain);
>   	return ERR_PTR(err);
>   }
> +
> +/**
> + * irq_domain_instantiate() - Instantiate a new irq domain data structure
> + * @info: Domain information pointer pointing to the information for this domain
> + *
> + * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
> + */
> +struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
> +{
> +	return __irq_domain_instantiate(info, false);
> +}
>   EXPORT_SYMBOL_GPL(irq_domain_instantiate);
>   
>   /**
> @@ -413,28 +439,13 @@ struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
>   		.fwnode		= fwnode,
>   		.size		= size,
>   		.hwirq_max	= size,
> +		.virq_base	= first_irq,
>   		.ops		= ops,
>   		.host_data	= host_data,
>   	};
> -	struct irq_domain *domain;
> +	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
>   
> -	domain = irq_domain_instantiate(&info);
> -	if (IS_ERR(domain))
> -		return NULL;
> -
> -	if (first_irq > 0) {
> -		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
> -			/* attempt to allocated irq_descs */
> -			int rc = irq_alloc_descs(first_irq, first_irq, size,
> -						 of_node_to_nid(to_of_node(fwnode)));
> -			if (rc < 0)
> -				pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> -					first_irq);
> -		}
> -		irq_domain_associate_many(domain, first_irq, 0, size);
> -	}
> -
> -	return domain;
> +	return IS_ERR(domain) ? NULL : domain;
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_create_simple);
>   
> @@ -476,18 +487,14 @@ struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
>   		.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);
> -
> -	return domain;
> +	return IS_ERR(domain) ? NULL : domain;
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_create_legacy);
>   

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! ~~

在2024年8月13日八月 上午11:54，Matti Vaittinen写道：
[...]
>> 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().

Thanks for troubleshooting!

For those who want an easy reproducer on QEMU (9.0.2):

make ARCH=mips CROSS_COMPILE=mips64-linux- 64r6el_defconfig
make ARCH=mips CROSS_COMPILE=mips64-linux- -j8
qemu-system-mips64el -M boston -cpu I6500 -kernel ./vmlinux -append "console=ttyS0,115200" -nographic

Thanks

>
>> 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! ~~

-- 
- Jiaxun

Hi Jiaxun,

On 8/13/24 13:54, Matti Vaittinen wrote:
> 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.

...

>> Do you have any idea on how should we fix it?

This is quick'n dirty but do you think you could try following? (I have 
only compile-tested it). I'll also attach the patch as I have no idea if 
this mail client mutilates patches. I can send in proper format if it helps.


From: Matti Vaittinen <mazziesaccount@gmail.com>
Date: Tue, 13 Aug 2024 14:34:27 +0300
Subject: [PATCH] irqdomain: Fix irq_domain_create_legacy() when 
first_irq is 0

The
70114e7f7585 ("irqdomain: Simplify simple and legacy domain creation")
changed logic of calling the irq_domain_associate_many() from the
irq_domain_create_legacy() when first_irq is set to 0. Before the change,
the irq_domain_associate_many() is unconditionally called inside the
irq_domain_create_legacy(). After the change, the call is omitted when
first_irq is set to 0. This breaks MIPS systemns where
drivers/irqchip/irq-mips-cpu.c has irq_domain_add_legacy() called with
first_irq set to 0.

Fixes: 70114e7f7585 ("irqdomain: Simplify simple and legacy domain 
creation")
Signed-off-by Matti Vaittinen <mazziesaccount@gmail.com>
---
  kernel/irq/irqdomain.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 01001eb615ec..5be165399a96 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -300,7 +300,8 @@ static void irq_domain_instantiate_descs(const 
struct irq_domain_info *info)
  }

  static struct irq_domain *__irq_domain_instantiate(const struct 
irq_domain_info *info,
-						   bool cond_alloc_descs)
+						   bool cond_alloc_descs,
+						   bool cond_force_associate)
  {
  	struct irq_domain *domain;
  	int err;
@@ -337,10 +338,9 @@ static struct irq_domain 
*__irq_domain_instantiate(const struct irq_domain_info
  		irq_domain_instantiate_descs(info);

  	/* Legacy interrupt domains have a fixed Linux interrupt number */
-	if (info->virq_base > 0) {
+	if (cond_force_associate || info->virq_base > 0)
  		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
  					  info->size - info->hwirq_base);
-	}

  	return domain;

@@ -360,7 +360,7 @@ static struct irq_domain 
*__irq_domain_instantiate(const struct irq_domain_info
   */
  struct irq_domain *irq_domain_instantiate(const struct irq_domain_info 
*info)
  {
-	return __irq_domain_instantiate(info, false);
+	return __irq_domain_instantiate(info, false, false);
  }
  EXPORT_SYMBOL_GPL(irq_domain_instantiate);

@@ -464,7 +464,7 @@ struct irq_domain *irq_domain_create_simple(struct 
fwnode_handle *fwnode,
  		.ops		= ops,
  		.host_data	= host_data,
  	};
-	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true, false);

  	return IS_ERR(domain) ? NULL : domain;
  }
@@ -513,7 +513,7 @@ struct irq_domain *irq_domain_create_legacy(struct 
fwnode_handle *fwnode,
  		.ops		= ops,
  		.host_data	= host_data,
  	};
-	struct irq_domain *domain = irq_domain_instantiate(&info);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, false, true);

  	return IS_ERR(domain) ? NULL : domain;
  }
-- 
2.45.2

在2024年8月13日八月 下午1:02，Matti Vaittinen写道：
> Hi Jiaxun,
>
> On 8/13/24 13:54, Matti Vaittinen wrote:
>> 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.
>
> ...
>
>>> Do you have any idea on how should we fix it?
>
> This is quick'n dirty but do you think you could try following? (I have 
> only compile-tested it). I'll also attach the patch as I have no idea if 
> this mail client mutilates patches. I can send in proper format if it helps.

Can confirm it fixed booting!

Thanks for quick fix.

- Jiaxun

>
>
> From: Matti Vaittinen <mazziesaccount@gmail.com>
> Date: Tue, 13 Aug 2024 14:34:27 +0300
> Subject: [PATCH] irqdomain: Fix irq_domain_create_legacy() when 
> first_irq is 0
>
> The
> 70114e7f7585 ("irqdomain: Simplify simple and legacy domain creation")
> changed logic of calling the irq_domain_associate_many() from the
> irq_domain_create_legacy() when first_irq is set to 0. Before the change,
> the irq_domain_associate_many() is unconditionally called inside the
> irq_domain_create_legacy(). After the change, the call is omitted when
> first_irq is set to 0. This breaks MIPS systemns where
> drivers/irqchip/irq-mips-cpu.c has irq_domain_add_legacy() called with
> first_irq set to 0.
>
> Fixes: 70114e7f7585 ("irqdomain: Simplify simple and legacy domain 
> creation")
> Signed-off-by Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>   kernel/irq/irqdomain.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 01001eb615ec..5be165399a96 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -300,7 +300,8 @@ static void irq_domain_instantiate_descs(const 
> struct irq_domain_info *info)
>   }
>
>   static struct irq_domain *__irq_domain_instantiate(const struct 
> irq_domain_info *info,
> -						   bool cond_alloc_descs)
> +						   bool cond_alloc_descs,
> +						   bool cond_force_associate)
>   {
>   	struct irq_domain *domain;
>   	int err;
> @@ -337,10 +338,9 @@ static struct irq_domain 
> *__irq_domain_instantiate(const struct irq_domain_info
>   		irq_domain_instantiate_descs(info);
>
>   	/* Legacy interrupt domains have a fixed Linux interrupt number */
> -	if (info->virq_base > 0) {
> +	if (cond_force_associate || info->virq_base > 0)
>   		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
>   					  info->size - info->hwirq_base);
> -	}
>
>   	return domain;
>
> @@ -360,7 +360,7 @@ static struct irq_domain 
> *__irq_domain_instantiate(const struct irq_domain_info
>    */
>   struct irq_domain *irq_domain_instantiate(const struct irq_domain_info 
> *info)
>   {
> -	return __irq_domain_instantiate(info, false);
> +	return __irq_domain_instantiate(info, false, false);
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_instantiate);
>
> @@ -464,7 +464,7 @@ struct irq_domain *irq_domain_create_simple(struct 
> fwnode_handle *fwnode,
>   		.ops		= ops,
>   		.host_data	= host_data,
>   	};
> -	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
> +	struct irq_domain *domain = __irq_domain_instantiate(&info, true, false);
>
>   	return IS_ERR(domain) ? NULL : domain;
>   }
> @@ -513,7 +513,7 @@ struct irq_domain *irq_domain_create_legacy(struct 
> fwnode_handle *fwnode,
>   		.ops		= ops,
>   		.host_data	= host_data,
>   	};
> -	struct irq_domain *domain = irq_domain_instantiate(&info);
> +	struct irq_domain *domain = __irq_domain_instantiate(&info, false, true);
>
>   	return IS_ERR(domain) ? NULL : domain;
>   }
> -- 
> 2.45.2
>
>
>
> 附件:
> * 0001-irqdomain-Fix-irq_domain_create_legacy-when-first_ir.patch

-- 
- Jiaxun

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     24d02c4e53e2f02da16b2ae8a1bc92553110ca25
Gitweb:        https://git.kernel.org/tip/24d02c4e53e2f02da16b2ae8a1bc92553110ca25
Author:        Matti Vaittinen <mazziesaccount@gmail.com>
AuthorDate:    Tue, 13 Aug 2024 14:34:27 +03:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 20 Aug 2024 17:12:43 +02:00

irqdomain: Always associate interrupts for legacy domains

The unification of irq_domain_create_legacy() missed the fact that
interrupts must be associated even when the Linux interrupt number provided
in the first_irq argument is 0.

This breaks all call sites of irq_domain_create_legacy() which supply 0 as
the first_irq argument.

Enforce the association for legacy domains in __irq_domain_instantiate() to
cure this.

[ tglx: Massaged it slightly. ]

Fixes: 70114e7f7585 ("irqdomain: Simplify simple and legacy domain creation")
Reported-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Link: https://lore.kernel.org/all/c3379142-10bc-4f14-b8ac-a46927aeac38@gmail.com
---
 kernel/irq/irqdomain.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 1acc530..5df8780 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -306,7 +306,7 @@ static void irq_domain_instantiate_descs(const struct irq_domain_info *info)
 }
 
 static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info *info,
-						   bool cond_alloc_descs)
+						   bool cond_alloc_descs, bool force_associate)
 {
 	struct irq_domain *domain;
 	int err;
@@ -342,8 +342,12 @@ static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info 
 	if (cond_alloc_descs && info->virq_base > 0)
 		irq_domain_instantiate_descs(info);
 
-	/* Legacy interrupt domains have a fixed Linux interrupt number */
-	if (info->virq_base > 0) {
+	/*
+	 * Legacy interrupt domains have a fixed Linux interrupt number
+	 * associated. Other interrupt domains can request association by
+	 * providing a Linux interrupt number > 0.
+	 */
+	if (force_associate || info->virq_base > 0) {
 		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
 					  info->size - info->hwirq_base);
 	}
@@ -366,7 +370,7 @@ err_domain_free:
  */
 struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
 {
-	return __irq_domain_instantiate(info, false);
+	return __irq_domain_instantiate(info, false, false);
 }
 EXPORT_SYMBOL_GPL(irq_domain_instantiate);
 
@@ -470,7 +474,7 @@ struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
 		.ops		= ops,
 		.host_data	= host_data,
 	};
-	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true, false);
 
 	return IS_ERR(domain) ? NULL : domain;
 }
@@ -519,7 +523,7 @@ struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
 		.ops		= ops,
 		.host_data	= host_data,
 	};
-	struct irq_domain *domain = irq_domain_instantiate(&info);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, false, true);
 
 	return IS_ERR(domain) ? NULL : domain;
 }

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     50059ccaa3c98badeae197b918e2ae06bb6f5162
Gitweb:        https://git.kernel.org/tip/50059ccaa3c98badeae197b918e2ae06bb6f5162
Author:        Matti Vaittinen <mazziesaccount@gmail.com>
AuthorDate:    Tue, 13 Aug 2024 14:34:27 +03:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Aug 2024 16:09:47 +02:00

irqdomain: Always associate interrupts for legacy domains

The unification of irq_domain_create_legacy() missed the fact that
interrupts must be associated even when the Linux interrupt number provided
in the first_irq argument is 0.

This breaks all call sites of irq_domain_create_legacy() which supply 0 as
the first_irq argument.

Enforce the association for legacy domains in __irq_domain_instantiate() to
cure this.

[ tglx: Massaged it slightly. ]

Fixes: 70114e7f7585 ("irqdomain: Simplify simple and legacy domain creation")
Reported-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by Matti Vaittinen <mazziesaccount@gmail.com>
Tested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Link: https://lore.kernel.org/all/c3379142-10bc-4f14-b8ac-a46927aeac38@gmail.com
---
 kernel/irq/irqdomain.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 1acc530..5df8780 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -306,7 +306,7 @@ static void irq_domain_instantiate_descs(const struct irq_domain_info *info)
 }
 
 static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info *info,
-						   bool cond_alloc_descs)
+						   bool cond_alloc_descs, bool force_associate)
 {
 	struct irq_domain *domain;
 	int err;
@@ -342,8 +342,12 @@ static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info 
 	if (cond_alloc_descs && info->virq_base > 0)
 		irq_domain_instantiate_descs(info);
 
-	/* Legacy interrupt domains have a fixed Linux interrupt number */
-	if (info->virq_base > 0) {
+	/*
+	 * Legacy interrupt domains have a fixed Linux interrupt number
+	 * associated. Other interrupt domains can request association by
+	 * providing a Linux interrupt number > 0.
+	 */
+	if (force_associate || info->virq_base > 0) {
 		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
 					  info->size - info->hwirq_base);
 	}
@@ -366,7 +370,7 @@ err_domain_free:
  */
 struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
 {
-	return __irq_domain_instantiate(info, false);
+	return __irq_domain_instantiate(info, false, false);
 }
 EXPORT_SYMBOL_GPL(irq_domain_instantiate);
 
@@ -470,7 +474,7 @@ struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
 		.ops		= ops,
 		.host_data	= host_data,
 	};
-	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true, false);
 
 	return IS_ERR(domain) ? NULL : domain;
 }
@@ -519,7 +523,7 @@ struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
 		.ops		= ops,
 		.host_data	= host_data,
 	};
-	struct irq_domain *domain = irq_domain_instantiate(&info);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, false, true);
 
 	return IS_ERR(domain) ? NULL : domain;
 }