[PATCH v2 0/3] regmap IRQ support for devices with multiple IRQs

[PATCH v2 0/3] regmap IRQ support for devices with multiple IRQs

[Matti Vaittinen]

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

Devices can provide multiple interrupt lines. One reason for this is that
a device has multiple subfunctions, each providing its own interrupt line.
Another reason is that a device can be designed to be used (also) on a
system where some of the interrupts can be routed to another processor.

A line often further acts as a demultiplex for specific interrupts
and has it's respective set of interrupt (status, mask, ack, ...)
registers.

Regmap supports the handling of these registers and demultiplexing
interrupts, but interrupt domain code ends up assigning the same name for
the per interrupt line domains

This series adds possibility for giving a name suffix for an interrupt
domain when regmap is used for devices with multiple interrupt lines.

Series is created on top of the:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/core

which contains the changes by Herve Codina adding both the
struct irq_domain_info and the irq_domain_instantiate().

Previous discussion can be found from:
https://lore.kernel.org/all/87plst28yk.ffs@tglx/
https://lore.kernel.org/all/15685ef6-92a5-41df-9148-1a67ceaec47b@gmail.com/

The domain suffix support added in this series will be used by the
ROHM BD96801 ERRB IRQ support code. The BD96801 ERRB support will need
the initial BD96801 driver code, which is not yet in irq/core or regmap
trees. Thus the user for this new support is not included in the series,
but will be sent once the name suffix support gets merged.

Revision history:
	v1 => v2:
	  - Fixes as suggested by Thomas Gleixner. Mainly fixes to
	    legacy and simple domain.
	  - Added new patch, patch 1.

---

Matti Vaittinen (3):
  irqdomain: simplify simple and legacy domain creation
  irqdomain: Allow giving name suffix for domain
  regmap: Allow setting IRQ domain name suffix

 drivers/base/regmap/regmap-irq.c |  37 +++++++----
 include/linux/irqdomain.h        |   8 +++
 include/linux/regmap.h           |   4 ++
 kernel/irq/irqdomain.c           | 109 +++++++++++++++++++------------
 4 files changed, 105 insertions(+), 53 deletions(-)


base-commit: bb4531976523c6e394188c4f4a7eeaf5e9efdd48
-- 
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 =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

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

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 =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/3] irqdomain: Allow giving name suffix for domain

[Matti Vaittinen]

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

Devices can provide multiple interrupt lines. One reason for this is that
a device has multiple subfunctions, each providing its own interrupt line.
Another reason is that a device can be designed to be used (also) on a
system where some of the interrupts can be routed to another processor.

A line often further acts as a demultiplex for specific interrupts
and has it's respective set of interrupt (status, mask, ack, ...)
registers.

Regmap supports the handling of these registers and demultiplexing
interrupts, but interrupt domain code ends up assigning the same name for
the per interrupt line domains. This will cause a naming collision in the
debugFS code and can also lead to confusion, as /proc/interrupts would
show two separate interrupts with the same domain name and hardware
interrupt number.

Instead of adding a workaround in regmap or driver code, allow giving a
name suffix for the domain name when the domain is created.

Add a name_suffix field in the irq_domain_info structure and make the
irq_domain_instantiate() to use this suffix if it is given when a domain
is created.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
v1 => v2:
 - typofix in comment. 'collison' to 'collision'.
---
 include/linux/irqdomain.h |  3 +++
 kernel/irq/irqdomain.c    | 36 +++++++++++++++++++++++++++---------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index bfcffa2c7047..e432b6a12a32 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -295,6 +295,8 @@ struct irq_domain_chip_generic_info;
  * @virq_base:		The first Linux interrupt number for legacy domains to
  *			immediately associate the interrupts after domain creation
  * @bus_token:		Domain bus token
+ * @name_suffix:	Optional name suffix to avoid collisions when multiple
+ *			domains are added using same fwnode
  * @ops:		Domain operation callbacks
  * @host_data:		Controller private data pointer
  * @dgc_info:		Geneneric chip information structure pointer used to
@@ -313,6 +315,7 @@ struct irq_domain_info {
 	unsigned int				hwirq_base;
 	unsigned int				virq_base;
 	enum irq_domain_bus_token		bus_token;
+	const char				*name_suffix;
 	const struct irq_domain_ops		*ops;
 	void					*host_data;
 #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 5af5e4028de2..376bcfb45aff 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -129,13 +129,25 @@ void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
 
 static int irq_domain_set_name(struct irq_domain *domain,
-			       const struct fwnode_handle *fwnode,
-			       enum irq_domain_bus_token bus_token)
+			       const struct irq_domain_info *info)
 {
+	const struct fwnode_handle *fwnode = info->fwnode;
+	enum irq_domain_bus_token bus_token = info->bus_token;
 	static atomic_t unknown_domains;
 	struct irqchip_fwid *fwid;
 
 	if (is_fwnode_irqchip(fwnode)) {
+		/*
+		 * The name_suffix is only intended to be used to avoid a name
+		 * collision, when multiple domains are created for a single
+		 * device and the name is picked using a real device node.
+		 * (Typical use-case is regmap-IRQ controllers for devices
+		 * providing more than one physical IRQ.) There should be no
+		 * need to use name_suffix with irqchip-fwnode.
+		 */
+		if (info->name_suffix)
+			return NULL;
+
 		fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
 
 		switch (fwid->type) {
@@ -164,17 +176,23 @@ static int irq_domain_set_name(struct irq_domain *domain,
 		   is_software_node(fwnode)) {
 		char *name;
 
+		if (info->name_suffix)
+			name = bus_token ?
+				kasprintf(GFP_KERNEL, "%pfw-%s-%d", fwnode,
+					  info->name_suffix, bus_token) :
+				kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, info->name_suffix);
+		else
+			name = bus_token ?
+				kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
+				kasprintf(GFP_KERNEL, "%pfw", fwnode);
+		if (!name)
+			return -ENOMEM;
+
 		/*
 		 * fwnode paths contain '/', which debugfs is legitimately
 		 * unhappy about. Replace them with ':', which does
 		 * the trick and is not as offensive as '\'...
 		 */
-		name = bus_token ?
-			kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
-			kasprintf(GFP_KERNEL, "%pfw", fwnode);
-		if (!name)
-			return -ENOMEM;
-
 		domain->name = strreplace(name, '/', ':');
 		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
 	}
@@ -211,7 +229,7 @@ static struct irq_domain *__irq_domain_create(const struct irq_domain_info *info
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	err = irq_domain_set_name(domain, info->fwnode, info->bus_token);
+	err = irq_domain_set_name(domain, info);
 	if (err) {
 		kfree(domain);
 		return ERR_PTR(err);
-- 
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 =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 3/3] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

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

When multiple IRQ domains are created from the same device-tree node they
will get the same name based on the device-tree path. This will cause a
naming collision in debugFS when IRQ domain specific entries are created.

The regmap-IRQ creates per instance IRQ domains. This will lead to a
domain name conflict when a device which provides more than one
interrupt line uses the regmap-IRQ.

Add support for specifying an IRQ domain name suffix when creating a
regmap-IRQ controller.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
A change worth mentioning is that this patch changes the error code
returned by IRQ domain generation code to be propagated to the caller.
Earlier all IRQ domain creation failutes were returning the -ENOMEM.
Please let me know if you assume this will cause problems.

This patch was originally part of the series adding support for the
ROHM BD96801 PMIC. Basic support was already merged while this one was
postponed until the name-suffix support was added to IRQ-domain code.
Hence the non linear version history.

Finally, there is a comment:
"Should really dispose of the domain but..." in the regmap-IRQ creation
code. Any insight what the "but..." refers to would be appreciated as
there would be an option to for example use the devm_ variant of the
irq_domain_instantiate().

Revision history:
v1 => v2 (new series)
 - fix the legacy domain instantiation as was pointed out by Thomas
   Gleixner.
 - drop irq_domain_associate_many() which was moved to be called during
   the irq_domain_instantiate() as was suggested by Thomas.
v1 of the new series:
 - use the new irq_domain_instantiate().
v2 => v3 (old series):
 - Drop name suffix support for the legacy domains
---
 drivers/base/regmap/regmap-irq.c | 37 ++++++++++++++++++++++----------
 include/linux/regmap.h           |  4 ++++
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index d3ec1345b5b5..a750e48a26b8 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -608,6 +608,30 @@ int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
 }
 EXPORT_SYMBOL_GPL(regmap_irq_set_type_config_simple);
 
+static int regmap_irq_create_domain(struct fwnode_handle *fwnode, int irq_base,
+				    const struct regmap_irq_chip *chip,
+				    struct regmap_irq_chip_data *d)
+{
+	struct irq_domain_info info = {
+		.fwnode = fwnode,
+		.size = chip->num_irqs,
+		.hwirq_max = chip->num_irqs,
+		.virq_base = irq_base,
+		.ops = &regmap_domain_ops,
+		.host_data = d,
+		.name_suffix = chip->domain_suffix,
+	};
+
+	d->domain = irq_domain_instantiate(&info);
+	if (IS_ERR(d->domain)) {
+		dev_err(d->map->dev, "Failed to create IRQ domain\n");
+		return PTR_ERR(d->domain);
+	}
+
+	return 0;
+}
+
+
 /**
  * regmap_add_irq_chip_fwnode() - Use standard regmap IRQ controller handling
  *
@@ -856,18 +880,9 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 	}
 
-	if (irq_base)
-		d->domain = irq_domain_create_legacy(fwnode, chip->num_irqs,
-						     irq_base, 0,
-						     &regmap_domain_ops, d);
-	else
-		d->domain = irq_domain_create_linear(fwnode, chip->num_irqs,
-						     &regmap_domain_ops, d);
-	if (!d->domain) {
-		dev_err(map->dev, "Failed to create IRQ domain\n");
-		ret = -ENOMEM;
+	ret = regmap_irq_create_domain(fwnode, irq_base, chip, d);
+	if (ret)
 		goto err_alloc;
-	}
 
 	ret = request_threaded_irq(irq, NULL, regmap_irq_thread,
 				   irq_flags | IRQF_ONESHOT,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 122e38161acb..f9ccad32fc5c 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1521,6 +1521,9 @@ struct regmap_irq_chip_data;
  * struct regmap_irq_chip - Description of a generic regmap irq_chip.
  *
  * @name:        Descriptive name for IRQ controller.
+ * @domain_suffix: Name suffix to be appended to end of IRQ domain name. Needed
+ *		   when multiple regmap-IRQ controllers are created from same
+ *		   device.
  *
  * @main_status: Base main status register address. For chips which have
  *		 interrupts arranged in separate sub-irq blocks with own IRQ
@@ -1606,6 +1609,7 @@ struct regmap_irq_chip_data;
  */
 struct regmap_irq_chip {
 	const char *name;
+	const char *domain_suffix;
 
 	unsigned int main_status;
 	unsigned int num_main_status_bits;
-- 
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 =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] irqdomain: Allow giving name suffix for domain

[Thomas Gleixner]

On Thu, Aug 08 2024 at 15:35, Matti Vaittinen wrote:
>  
> +		if (info->name_suffix)
> +			name = bus_token ?
> +				kasprintf(GFP_KERNEL, "%pfw-%s-%d", fwnode,
> +					  info->name_suffix, bus_token) :
> +				kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, info->name_suffix);
> +		else
> +			name = bus_token ?
> +				kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
> +				kasprintf(GFP_KERNEL, "%pfw", fwnode);

This really makes my eyes bleed. I know you copied the existing mess,
but that does not make it less tasteless.

I'll send out a cleanup for this horrorshow in a few.

Thanks,

        tglx

[PATCH] irqdomain: Cleanup domain name allocation

[Thomas Gleixner]

irq_domain_set_name() is truly unreadable gunk. Clean it up before adding
more.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/irqdomain.c |  104 +++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 49 deletions(-)

--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -128,11 +128,53 @@ void irq_domain_free_fwnode(struct fwnod
 }
 EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
 
-static int irq_domain_set_name(struct irq_domain *domain,
-			       const struct fwnode_handle *fwnode,
-			       enum irq_domain_bus_token bus_token)
+static int alloc_name(struct irq_domain *domain, char *base, enum irq_domain_bus_token bus_token)
+{
+	domain->name = bus_token ? kasprintf(GFP_KERNEL, "%s-%d", base, bus_token) :
+				   kasprintf(GFP_KERNEL, "%s", base);
+	if (!domain->name)
+		return -ENOMEM;
+
+	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	return 0;
+}
+
+static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
+			     enum irq_domain_bus_token bus_token)
+{
+	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
+				 kasprintf(GFP_KERNEL, "%pfw", fwnode);
+
+	if (!name)
+		return -ENOMEM;
+
+	/*
+	 * fwnode paths contain '/', which debugfs is legitimately unhappy
+	 * about. Replace them with ':', which does the trick and is not as
+	 * offensive as '\'...
+	 */
+	domain->name = strreplace(name, '/', ':');
+	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	return 0;
+}
+
+static int alloc_unknown_name(struct irq_domain *domain, enum irq_domain_bus_token bus_token)
 {
 	static atomic_t unknown_domains;
+	int id = atomic_inc_return(&unknown_domains);
+
+	domain->name = bus_token ? kasprintf(GFP_KERNEL, "unknown-%d-%d", id, bus_token) :
+				   kasprintf(GFP_KERNEL, "unknown-%d", id);
+
+	if (!domain->name)
+		return -ENOMEM;
+	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	return 0;
+}
+
+static int irq_domain_set_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
+			       enum irq_domain_bus_token bus_token)
+{
 	struct irqchip_fwid *fwid;
 
 	if (is_fwnode_irqchip(fwnode)) {
@@ -141,59 +183,23 @@ static int irq_domain_set_name(struct ir
 		switch (fwid->type) {
 		case IRQCHIP_FWNODE_NAMED:
 		case IRQCHIP_FWNODE_NAMED_ID:
-			domain->name = bus_token ?
-					kasprintf(GFP_KERNEL, "%s-%d",
-						  fwid->name, bus_token) :
-					kstrdup(fwid->name, GFP_KERNEL);
-			if (!domain->name)
-				return -ENOMEM;
-			domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
-			break;
+			return alloc_name(domain, fwid->name, bus_token);
 		default:
 			domain->name = fwid->name;
-			if (bus_token) {
-				domain->name = kasprintf(GFP_KERNEL, "%s-%d",
-							 fwid->name, bus_token);
-				if (!domain->name)
-					return -ENOMEM;
-				domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
-			}
-			break;
+			if (bus_token)
+				return alloc_name(domain, fwid->name, bus_token);
 		}
-	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) ||
-		   is_software_node(fwnode)) {
-		char *name;
-
-		/*
-		 * fwnode paths contain '/', which debugfs is legitimately
-		 * unhappy about. Replace them with ':', which does
-		 * the trick and is not as offensive as '\'...
-		 */
-		name = bus_token ?
-			kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
-			kasprintf(GFP_KERNEL, "%pfw", fwnode);
-		if (!name)
-			return -ENOMEM;
 
-		domain->name = strreplace(name, '/', ':');
-		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) || is_software_node(fwnode)) {
+		return alloc_fwnode_name(domain, fwnode, bus_token);
 	}
 
-	if (!domain->name) {
-		if (fwnode)
-			pr_err("Invalid fwnode type for irqdomain\n");
-		domain->name = bus_token ?
-				kasprintf(GFP_KERNEL, "unknown-%d-%d",
-					  atomic_inc_return(&unknown_domains),
-					  bus_token) :
-				kasprintf(GFP_KERNEL, "unknown-%d",
-					  atomic_inc_return(&unknown_domains));
-		if (!domain->name)
-			return -ENOMEM;
-		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
-	}
+	if (domain->name)
+		return 0;
 
-	return 0;
+	if (fwnode)
+		pr_err("Invalid fwnode type for irqdomain\n");
+	return alloc_unknown_name(domain, bus_token);
 }
 
 static struct irq_domain *__irq_domain_create(const struct irq_domain_info *info)

[PATCH v2a 2/3] irqdomain: Allow giving name suffix for domain

[Thomas Gleixner]

From: Matti Vaittinen <mazziesaccount@gmail.com>

Devices can provide multiple interrupt lines. One reason for this is that
a device has multiple subfunctions, each providing its own interrupt line.
Another reason is that a device can be designed to be used (also) on a
system where some of the interrupts can be routed to another processor.

A line often further acts as a demultiplex for specific interrupts
and has it's respective set of interrupt (status, mask, ack, ...)
registers.

Regmap supports the handling of these registers and demultiplexing
interrupts, but the interrupt domain code ends up assigning the same name
for the per interrupt line domains. This causes a naming collision in the
debugFS code and leads to confusion, as /proc/interrupts shows two separate
interrupts with the same domain name and hardware interrupt number.

Instead of adding a workaround in regmap or driver code, allow giving a
name suffix for the domain name when the domain is created.

Add a name_suffix field in the irq_domain_info structure and make
irq_domain_instantiate() use this suffix if it is given when a domain is
created.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
Revision history:
v2 => v2a:
   Update to name allocation cleanup patch. Fix the invalid NULL return.
v1 => v2:
 - typofix in comment. 'collison' to 'collision'.
---
 include/linux/irqdomain.h |    3 +++
 kernel/irq/irqdomain.c    |   32 +++++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -295,6 +295,8 @@ struct irq_domain_chip_generic_info;
  * @virq_base:		The first Linux interrupt number for legacy domains to
  *			immediately associate the interrupts after domain creation
  * @bus_token:		Domain bus token
+ * @name_suffix:	Optional name suffix to avoid collisions when multiple
+ *			domains are added using same fwnode
  * @ops:		Domain operation callbacks
  * @host_data:		Controller private data pointer
  * @dgc_info:		Geneneric chip information structure pointer used to
@@ -313,6 +315,7 @@ struct irq_domain_info {
 	unsigned int				hwirq_base;
 	unsigned int				virq_base;
 	enum irq_domain_bus_token		bus_token;
+	const char				*name_suffix;
 	const struct irq_domain_ops		*ops;
 	void					*host_data;
 #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -140,11 +140,14 @@ static int alloc_name(struct irq_domain
 }
 
 static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
-			     enum irq_domain_bus_token bus_token)
+			     enum irq_domain_bus_token bus_token, const char *suffix)
 {
-	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
-				 kasprintf(GFP_KERNEL, "%pfw", fwnode);
+	const char *sep = suffix ? "-" : "";
+	const char *suf = suffix ? : "";
+	char *name;
 
+	name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%s%s%d", fwnode, suf, sep, bus_token) :
+			   kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, suf);
 	if (!name)
 		return -ENOMEM;
 
@@ -172,13 +175,24 @@ static int alloc_unknown_name(struct irq
 	return 0;
 }
 
-static int irq_domain_set_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
-			       enum irq_domain_bus_token bus_token)
+static int irq_domain_set_name(struct irq_domain *domain, const struct irq_domain_info *info)
 {
-	struct irqchip_fwid *fwid;
+	enum irq_domain_bus_token bus_token = info->bus_token;
+	const struct fwnode_handle *fwnode = info->fwnode;
 
 	if (is_fwnode_irqchip(fwnode)) {
-		fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
+		struct irqchip_fwid *fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
+
+		/*
+		 * The name_suffix is only intended to be used to avoid a name
+		 * collision, when multiple domains are created for a single
+		 * device and the name is picked using a real device node.
+		 * (Typical use-case is regmap-IRQ controllers for devices
+		 * providing more than one physical IRQ.) There should be no
+		 * need to use name_suffix with irqchip-fwnode.
+		 */
+		if (info->name_suffix)
+			return -EINVAL;
 
 		switch (fwid->type) {
 		case IRQCHIP_FWNODE_NAMED:
@@ -191,7 +205,7 @@ static int irq_domain_set_name(struct ir
 		}
 
 	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) || is_software_node(fwnode)) {
-		return alloc_fwnode_name(domain, fwnode, bus_token);
+		return alloc_fwnode_name(domain, fwnode, bus_token, info->name_suffix);
 	}
 
 	if (domain->name)
@@ -217,7 +231,7 @@ static struct irq_domain *__irq_domain_c
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	err = irq_domain_set_name(domain, info->fwnode, info->bus_token);
+	err = irq_domain_set_name(domain, info);
 	if (err) {
 		kfree(domain);
 		return ERR_PTR(err);

Re: [PATCH v2a 2/3] irqdomain: Allow giving name suffix for domain

[Matti Vaittinen]

On 8/8/24 23:23, Thomas Gleixner wrote:
> 
> From: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Devices can provide multiple interrupt lines. One reason for this is that
> a device has multiple subfunctions, each providing its own interrupt line.
> Another reason is that a device can be designed to be used (also) on a
> system where some of the interrupts can be routed to another processor.
> 
> A line often further acts as a demultiplex for specific interrupts
> and has it's respective set of interrupt (status, mask, ack, ...)
> registers.
> 
> Regmap supports the handling of these registers and demultiplexing
> interrupts, but the interrupt domain code ends up assigning the same name
> for the per interrupt line domains. This causes a naming collision in the
> debugFS code and leads to confusion, as /proc/interrupts shows two separate
> interrupts with the same domain name and hardware interrupt number.
> 
> Instead of adding a workaround in regmap or driver code, allow giving a
> name suffix for the domain name when the domain is created.
> 
> Add a name_suffix field in the irq_domain_info structure and make
> irq_domain_instantiate() use this suffix if it is given when a domain is
> created.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> Revision history:
> v2 => v2a:
>     Update to name allocation cleanup patch. Fix the invalid NULL return.
> v1 => v2:
>   - typofix in comment. 'collison' to 'collision'.
> ---
>   include/linux/irqdomain.h |    3 +++
>   kernel/irq/irqdomain.c    |   32 +++++++++++++++++++++++---------
>   2 files changed, 26 insertions(+), 9 deletions(-)
> 
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -295,6 +295,8 @@ struct irq_domain_chip_generic_info;
>    * @virq_base:		The first Linux interrupt number for legacy domains to
>    *			immediately associate the interrupts after domain creation
>    * @bus_token:		Domain bus token
> + * @name_suffix:	Optional name suffix to avoid collisions when multiple
> + *			domains are added using same fwnode
>    * @ops:		Domain operation callbacks
>    * @host_data:		Controller private data pointer
>    * @dgc_info:		Geneneric chip information structure pointer used to
> @@ -313,6 +315,7 @@ struct irq_domain_info {
>   	unsigned int				hwirq_base;
>   	unsigned int				virq_base;
>   	enum irq_domain_bus_token		bus_token;
> +	const char				*name_suffix;
>   	const struct irq_domain_ops		*ops;
>   	void					*host_data;
>   #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -140,11 +140,14 @@ static int alloc_name(struct irq_domain
>   }
>   
>   static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
> -			     enum irq_domain_bus_token bus_token)
> +			     enum irq_domain_bus_token bus_token, const char *suffix)
>   {
> -	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
> -				 kasprintf(GFP_KERNEL, "%pfw", fwnode);
> +	const char *sep = suffix ? "-" : "";
> +	const char *suf = suffix ? : "";
> +	char *name;
>   
> +	name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%s%s%d", fwnode, suf, sep, bus_token) :
> +			   kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, suf);
>   	if (!name)
>   		return -ENOMEM;
>   

Thanks Thomas!
This looks much, much cleaner to me compared to my original version :)

> @@ -172,13 +175,24 @@ static int alloc_unknown_name(struct irq
>   	return 0;
>   }
>

Yours,
	-- Matti

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

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

Re: [PATCH] irqdomain: Cleanup domain name allocation

[Matti Vaittinen]

On 8/8/24 23:19, Thomas Gleixner wrote:
> irq_domain_set_name() is truly unreadable gunk. Clean it up before adding
> more.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   kernel/irq/irqdomain.c |  104 +++++++++++++++++++++++++------------------------
>   1 file changed, 55 insertions(+), 49 deletions(-)

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

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

Re: [PATCH v2 3/3] regmap: Allow setting IRQ domain name suffix

[Thomas Gleixner]

On Thu, Aug 08 2024 at 15:36, Matti Vaittinen wrote:
> When multiple IRQ domains are created from the same device-tree node they
> will get the same name based on the device-tree path. This will cause a
> naming collision in debugFS when IRQ domain specific entries are created.
>
> The regmap-IRQ creates per instance IRQ domains. This will lead to a
> domain name conflict when a device which provides more than one
> interrupt line uses the regmap-IRQ.
>
> Add support for specifying an IRQ domain name suffix when creating a
> regmap-IRQ controller.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

The change looks good to me.

Mark, I assume you want to take the regmap change. If so then feel free
to pull the prerequisites from

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-domain-24-08-09

into your tree. It's based on 6.11-rc1 and contains only the three
irqdomain core changes.

If you want me to carry the regmap change, please let me know.

Thanks,

        tglx

[tip: irq/core] irqdomain: Allow giving name suffix for domain

[tip-bot2 for Matti Vaittinen]

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

Commit-ID:     1e7c05292531e5b6bebe409cd531ed4ec0b2ff56
Gitweb:        https://git.kernel.org/tip/1e7c05292531e5b6bebe409cd531ed4ec0b2ff56
Author:        Matti Vaittinen <mazziesaccount@gmail.com>
AuthorDate:    Thu, 08 Aug 2024 22:23:06 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 09 Aug 2024 22:37:54 +02:00

irqdomain: Allow giving name suffix for domain

Devices can provide multiple interrupt lines. One reason for this is that
a device has multiple subfunctions, each providing its own interrupt line.
Another reason is that a device can be designed to be used (also) on a
system where some of the interrupts can be routed to another processor.

A line often further acts as a demultiplex for specific interrupts
and has it's respective set of interrupt (status, mask, ack, ...)
registers.

Regmap supports the handling of these registers and demultiplexing
interrupts, but the interrupt domain code ends up assigning the same name
for the per interrupt line domains. This causes a naming collision in the
debugFS code and leads to confusion, as /proc/interrupts shows two separate
interrupts with the same domain name and hardware interrupt number.

Instead of adding a workaround in regmap or driver code, allow giving a
name suffix for the domain name when the domain is created.

Add a name_suffix field in the irq_domain_info structure and make
irq_domain_instantiate() use this suffix if it is given when a domain is
created.

[ tglx: Adopt it to the cleanup patch and fixup the invalid NULL return ]

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/871q2yvk5x.ffs@tglx

---
 include/linux/irqdomain.h |  3 +++
 kernel/irq/irqdomain.c    | 30 +++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index bfcffa2..e432b6a 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -295,6 +295,8 @@ struct irq_domain_chip_generic_info;
  * @virq_base:		The first Linux interrupt number for legacy domains to
  *			immediately associate the interrupts after domain creation
  * @bus_token:		Domain bus token
+ * @name_suffix:	Optional name suffix to avoid collisions when multiple
+ *			domains are added using same fwnode
  * @ops:		Domain operation callbacks
  * @host_data:		Controller private data pointer
  * @dgc_info:		Geneneric chip information structure pointer used to
@@ -313,6 +315,7 @@ struct irq_domain_info {
 	unsigned int				hwirq_base;
 	unsigned int				virq_base;
 	enum irq_domain_bus_token		bus_token;
+	const char				*name_suffix;
 	const struct irq_domain_ops		*ops;
 	void					*host_data;
 #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 72ab601..01001eb 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -140,11 +140,14 @@ static int alloc_name(struct irq_domain *domain, char *base, enum irq_domain_bus
 }
 
 static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
-			     enum irq_domain_bus_token bus_token)
+			     enum irq_domain_bus_token bus_token, const char *suffix)
 {
-	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
-				 kasprintf(GFP_KERNEL, "%pfw", fwnode);
+	const char *sep = suffix ? "-" : "";
+	const char *suf = suffix ? : "";
+	char *name;
 
+	name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%s%s%d", fwnode, suf, sep, bus_token) :
+			   kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, suf);
 	if (!name)
 		return -ENOMEM;
 
@@ -172,12 +175,25 @@ static int alloc_unknown_name(struct irq_domain *domain, enum irq_domain_bus_tok
 	return 0;
 }
 
-static int irq_domain_set_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
-			       enum irq_domain_bus_token bus_token)
+static int irq_domain_set_name(struct irq_domain *domain, const struct irq_domain_info *info)
 {
+	enum irq_domain_bus_token bus_token = info->bus_token;
+	const struct fwnode_handle *fwnode = info->fwnode;
+
 	if (is_fwnode_irqchip(fwnode)) {
 		struct irqchip_fwid *fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
 
+		/*
+		 * The name_suffix is only intended to be used to avoid a name
+		 * collision when multiple domains are created for a single
+		 * device and the name is picked using a real device node.
+		 * (Typical use-case is regmap-IRQ controllers for devices
+		 * providing more than one physical IRQ.) There should be no
+		 * need to use name_suffix with irqchip-fwnode.
+		 */
+		if (info->name_suffix)
+			return -EINVAL;
+
 		switch (fwid->type) {
 		case IRQCHIP_FWNODE_NAMED:
 		case IRQCHIP_FWNODE_NAMED_ID:
@@ -189,7 +205,7 @@ static int irq_domain_set_name(struct irq_domain *domain, const struct fwnode_ha
 		}
 
 	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) || is_software_node(fwnode)) {
-		return alloc_fwnode_name(domain, fwnode, bus_token);
+		return alloc_fwnode_name(domain, fwnode, bus_token, info->name_suffix);
 	}
 
 	if (domain->name)
@@ -215,7 +231,7 @@ static struct irq_domain *__irq_domain_create(const struct irq_domain_info *info
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	err = irq_domain_set_name(domain, info->fwnode, info->bus_token);
+	err = irq_domain_set_name(domain, info);
 	if (err) {
 		kfree(domain);
 		return ERR_PTR(err);

[tip: irq/core] irqdomain: Cleanup domain name allocation

[tip-bot2 for Thomas Gleixner]

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

Commit-ID:     1bf2c92829274e7c815d06d7b3196a967ff70917
Gitweb:        https://git.kernel.org/tip/1bf2c92829274e7c815d06d7b3196a967ff70917
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Thu, 08 Aug 2024 22:19:41 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 09 Aug 2024 22:37:54 +02:00

irqdomain: Cleanup domain name allocation

irq_domain_set_name() is truly unreadable gunk. Clean it up before adding
more.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Link: https://lore.kernel.org/all/874j7uvkbm.ffs@tglx

---
 kernel/irq/irqdomain.c | 106 ++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 51 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 7625e42..72ab601 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -128,72 +128,76 @@ void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
 
-static int irq_domain_set_name(struct irq_domain *domain,
-			       const struct fwnode_handle *fwnode,
-			       enum irq_domain_bus_token bus_token)
+static int alloc_name(struct irq_domain *domain, char *base, enum irq_domain_bus_token bus_token)
+{
+	domain->name = bus_token ? kasprintf(GFP_KERNEL, "%s-%d", base, bus_token) :
+				   kasprintf(GFP_KERNEL, "%s", base);
+	if (!domain->name)
+		return -ENOMEM;
+
+	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	return 0;
+}
+
+static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
+			     enum irq_domain_bus_token bus_token)
+{
+	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
+				 kasprintf(GFP_KERNEL, "%pfw", fwnode);
+
+	if (!name)
+		return -ENOMEM;
+
+	/*
+	 * fwnode paths contain '/', which debugfs is legitimately unhappy
+	 * about. Replace them with ':', which does the trick and is not as
+	 * offensive as '\'...
+	 */
+	domain->name = strreplace(name, '/', ':');
+	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	return 0;
+}
+
+static int alloc_unknown_name(struct irq_domain *domain, enum irq_domain_bus_token bus_token)
 {
 	static atomic_t unknown_domains;
-	struct irqchip_fwid *fwid;
+	int id = atomic_inc_return(&unknown_domains);
+
+	domain->name = bus_token ? kasprintf(GFP_KERNEL, "unknown-%d-%d", id, bus_token) :
+				   kasprintf(GFP_KERNEL, "unknown-%d", id);
 
+	if (!domain->name)
+		return -ENOMEM;
+	domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	return 0;
+}
+
+static int irq_domain_set_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
+			       enum irq_domain_bus_token bus_token)
+{
 	if (is_fwnode_irqchip(fwnode)) {
-		fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
+		struct irqchip_fwid *fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
 
 		switch (fwid->type) {
 		case IRQCHIP_FWNODE_NAMED:
 		case IRQCHIP_FWNODE_NAMED_ID:
-			domain->name = bus_token ?
-					kasprintf(GFP_KERNEL, "%s-%d",
-						  fwid->name, bus_token) :
-					kstrdup(fwid->name, GFP_KERNEL);
-			if (!domain->name)
-				return -ENOMEM;
-			domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
-			break;
+			return alloc_name(domain, fwid->name, bus_token);
 		default:
 			domain->name = fwid->name;
-			if (bus_token) {
-				domain->name = kasprintf(GFP_KERNEL, "%s-%d",
-							 fwid->name, bus_token);
-				if (!domain->name)
-					return -ENOMEM;
-				domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
-			}
-			break;
+			if (bus_token)
+				return alloc_name(domain, fwid->name, bus_token);
 		}
-	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) ||
-		   is_software_node(fwnode)) {
-		char *name;
 
-		/*
-		 * fwnode paths contain '/', which debugfs is legitimately
-		 * unhappy about. Replace them with ':', which does
-		 * the trick and is not as offensive as '\'...
-		 */
-		name = bus_token ?
-			kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
-			kasprintf(GFP_KERNEL, "%pfw", fwnode);
-		if (!name)
-			return -ENOMEM;
-
-		domain->name = strreplace(name, '/', ':');
-		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
+	} else if (is_of_node(fwnode) || is_acpi_device_node(fwnode) || is_software_node(fwnode)) {
+		return alloc_fwnode_name(domain, fwnode, bus_token);
 	}
 
-	if (!domain->name) {
-		if (fwnode)
-			pr_err("Invalid fwnode type for irqdomain\n");
-		domain->name = bus_token ?
-				kasprintf(GFP_KERNEL, "unknown-%d-%d",
-					  atomic_inc_return(&unknown_domains),
-					  bus_token) :
-				kasprintf(GFP_KERNEL, "unknown-%d",
-					  atomic_inc_return(&unknown_domains));
-		if (!domain->name)
-			return -ENOMEM;
-		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
-	}
+	if (domain->name)
+		return 0;
 
-	return 0;
+	if (fwnode)
+		pr_err("Invalid fwnode type for irqdomain\n");
+	return alloc_unknown_name(domain, bus_token);
 }
 
 static struct irq_domain *__irq_domain_create(const struct irq_domain_info *info)

Re: [PATCH v2 3/3] regmap: Allow setting IRQ domain name suffix

[Andy Shevchenko]

On Thu, Aug 08, 2024 at 03:36:28PM +0300, Matti Vaittinen wrote:
> When multiple IRQ domains are created from the same device-tree node they
> will get the same name based on the device-tree path. This will cause a
> naming collision in debugFS when IRQ domain specific entries are created.
> 
> The regmap-IRQ creates per instance IRQ domains. This will lead to a
> domain name conflict when a device which provides more than one
> interrupt line uses the regmap-IRQ.
> 
> Add support for specifying an IRQ domain name suffix when creating a
> regmap-IRQ controller.

...

> +static int regmap_irq_create_domain(struct fwnode_handle *fwnode, int irq_base,
> +				    const struct regmap_irq_chip *chip,
> +				    struct regmap_irq_chip_data *d)
> +{
> +	struct irq_domain_info info = {
> +		.fwnode = fwnode,
> +		.size = chip->num_irqs,
> +		.hwirq_max = chip->num_irqs,
> +		.virq_base = irq_base,
> +		.ops = &regmap_domain_ops,
> +		.host_data = d,
> +		.name_suffix = chip->domain_suffix,
> +	};
> +
> +	d->domain = irq_domain_instantiate(&info);
> +	if (IS_ERR(d->domain)) {
> +		dev_err(d->map->dev, "Failed to create IRQ domain\n");
> +		return PTR_ERR(d->domain);
> +	}
> +
> +	return 0;
> +}
> +
> +

One blank line is enough?

>  /**

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

On 8/12/24 22:05, Andy Shevchenko wrote:
> On Thu, Aug 08, 2024 at 03:36:28PM +0300, Matti Vaittinen wrote:
>> When multiple IRQ domains are created from the same device-tree node they
>> will get the same name based on the device-tree path. This will cause a
>> naming collision in debugFS when IRQ domain specific entries are created.
>>
>> The regmap-IRQ creates per instance IRQ domains. This will lead to a
>> domain name conflict when a device which provides more than one
>> interrupt line uses the regmap-IRQ.
>>
>> Add support for specifying an IRQ domain name suffix when creating a
>> regmap-IRQ controller.
> 
> ...
> 
>> +static int regmap_irq_create_domain(struct fwnode_handle *fwnode, int irq_base,
>> +				    const struct regmap_irq_chip *chip,
>> +				    struct regmap_irq_chip_data *d)
>> +{
>> +	struct irq_domain_info info = {
>> +		.fwnode = fwnode,
>> +		.size = chip->num_irqs,
>> +		.hwirq_max = chip->num_irqs,
>> +		.virq_base = irq_base,
>> +		.ops = &regmap_domain_ops,
>> +		.host_data = d,
>> +		.name_suffix = chip->domain_suffix,
>> +	};
>> +
>> +	d->domain = irq_domain_instantiate(&info);
>> +	if (IS_ERR(d->domain)) {
>> +		dev_err(d->map->dev, "Failed to create IRQ domain\n");
>> +		return PTR_ERR(d->domain);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +
> 
> One blank line is enough?

Sure. Thanks Andy!

Yours,
	--Matti

> 
>>   /**
> 

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

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

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

[Jiaxun Yang]

[-- Attachment #1.1.1: Type: text/plain, Size: 8908 bytes --]



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);
>   


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 17207 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Matti Vaittinen]

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

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

[Matti Vaittinen]

[-- 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

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

[Jiaxun Yang]

在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

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

[Jiaxun Yang]

在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

[tip: irq/core] irqdomain: Always associate interrupts for legacy domains

[tip-bot2 for Matti Vaittinen]

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;
 }

Re: (subset) [PATCH v2 0/3] regmap IRQ support for devices with multiple IRQs

[Mark Brown]

On Thu, 08 Aug 2024 15:32:51 +0300, Matti Vaittinen wrote:
> Devices can provide multiple interrupt lines. One reason for this is that
> a device has multiple subfunctions, each providing its own interrupt line.
> Another reason is that a device can be designed to be used (also) on a
> system where some of the interrupts can be routed to another processor.
> 
> A line often further acts as a demultiplex for specific interrupts
> and has it's respective set of interrupt (status, mask, ack, ...)
> registers.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[3/3] regmap: Allow setting IRQ domain name suffix
      commit: dde286ee57704226b500cb9eb59547fec07aad3d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

[tip: irq/core] irqdomain: Always associate interrupts for legacy domains

[tip-bot2 for Matti Vaittinen]

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;
 }