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

[Matti Vaittinen <mazziesaccount@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4142 bytes --]

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



[-- Attachment #2: 0001-irqdomain-Fix-irq_domain_create_legacy-when-first_ir.patch --]
[-- Type: text/x-patch, Size: 2903 bytes --]

From ebd574885910a4a3a4fd1bfe63542f9465bf6dad Mon Sep 17 00:00:00 2001
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