[PATCH v4 00/19] irqdomain: fix mapping race and clean up locking

[PATCH v4 00/19] irqdomain: fix mapping race and clean up locking

[Johan Hovold]

Parallel probing (e.g. due to asynchronous probing) of devices that
share interrupts can currently result in two mappings for the same
hardware interrupt to be created.

This series fixes this mapping race and clean up the irqdomain locking
so that in the end the global irq_domain_mutex is only used for managing
the likewise global irq_domain_list, while domain operations (e.g.
IRQ allocations) use per-domain (hierarchy) locking.

Johan


Changes in v4
 - add a comment to __irq_domain_add() as further documentation of the
   domain lock and root pointer (19/19)
 - add a comment documenting that the lockdep assertion in
   irq_domain_set_mapping() also verifies that the domains in a
   hierarchy point to the same root (19/19)

Changes in v3
 - drop dead and bogus code (1--3/19, new)
 - fix racy mapcount accesses (5/19, new)
 - drop revmap mutex (6/19, new)
 - use irq_domain_mutex to address mapping race (9/19)
 - clean up irq_domain_push/pop_irq() (10/19, new)
 - use irq_domain_create_hierarchy() to construct hierarchies
   (11--18/19, new)
 - switch to per-domain locking (19/19, new)

Changes in v2
 - split out redundant-lookup cleanup (1/4)
 - use a per-domain mutex to address mapping race (2/4)
 - move kernel-doc to exported function (2/4)
 - fix association race (3/4, new)
 - use per-domain mutex for associations (4/4, new)


Johan Hovold (19):
  irqdomain: Drop bogus fwspec-mapping error handling
  irqdomain: Drop dead domain-name assignment
  irqdomain: Drop leftover brackets
  irqdomain: Fix association race
  irqdomain: Fix disassociation race
  irqdomain: Drop revmap mutex
  irqdomain: Look for existing mapping only once
  irqdomain: Refactor __irq_domain_alloc_irqs()
  irqdomain: Fix mapping-creation race
  irqdomain: Clean up irq_domain_push/pop_irq()
  x86/ioapic: Use irq_domain_create_hierarchy()
  x86/apic: Use irq_domain_create_hierarchy()
  irqchip/alpine-msi: Use irq_domain_add_hierarchy()
  irqchip/gic-v2m: Use irq_domain_create_hierarchy()
  irqchip/gic-v3-its: Use irq_domain_create_hierarchy()
  irqchip/gic-v3-mbi: Use irq_domain_create_hierarchy()
  irqchip/loongson-pch-msi: Use irq_domain_create_hierarchy()
  irqchip/mvebu-odmi: Use irq_domain_create_hierarchy()
  irqdomain: Switch to per-domain locking

 arch/x86/kernel/apic/io_apic.c         |   8 +-
 arch/x86/platform/uv/uv_irq.c          |   7 +-
 drivers/irqchip/irq-alpine-msi.c       |   8 +-
 drivers/irqchip/irq-gic-v2m.c          |   5 +-
 drivers/irqchip/irq-gic-v3-its.c       |  13 +-
 drivers/irqchip/irq-gic-v3-mbi.c       |   5 +-
 drivers/irqchip/irq-loongson-pch-msi.c |   9 +-
 drivers/irqchip/irq-mvebu-odmi.c       |  13 +-
 include/linux/irqdomain.h              |   6 +-
 kernel/irq/irqdomain.c                 | 341 ++++++++++++++-----------
 10 files changed, 233 insertions(+), 182 deletions(-)

-- 
2.38.2

[PATCH v4 00/19] irqdomain: fix mapping race and clean up locking

[Johan Hovold]

Parallel probing (e.g. due to asynchronous probing) of devices that
share interrupts can currently result in two mappings for the same
hardware interrupt to be created.

This series fixes this mapping race and clean up the irqdomain locking
so that in the end the global irq_domain_mutex is only used for managing
the likewise global irq_domain_list, while domain operations (e.g.
IRQ allocations) use per-domain (hierarchy) locking.

Johan


Changes in v4
 - add a comment to __irq_domain_add() as further documentation of the
   domain lock and root pointer (19/19)
 - add a comment documenting that the lockdep assertion in
   irq_domain_set_mapping() also verifies that the domains in a
   hierarchy point to the same root (19/19)

Changes in v3
 - drop dead and bogus code (1--3/19, new)
 - fix racy mapcount accesses (5/19, new)
 - drop revmap mutex (6/19, new)
 - use irq_domain_mutex to address mapping race (9/19)
 - clean up irq_domain_push/pop_irq() (10/19, new)
 - use irq_domain_create_hierarchy() to construct hierarchies
   (11--18/19, new)
 - switch to per-domain locking (19/19, new)

Changes in v2
 - split out redundant-lookup cleanup (1/4)
 - use a per-domain mutex to address mapping race (2/4)
 - move kernel-doc to exported function (2/4)
 - fix association race (3/4, new)
 - use per-domain mutex for associations (4/4, new)


Johan Hovold (19):
  irqdomain: Drop bogus fwspec-mapping error handling
  irqdomain: Drop dead domain-name assignment
  irqdomain: Drop leftover brackets
  irqdomain: Fix association race
  irqdomain: Fix disassociation race
  irqdomain: Drop revmap mutex
  irqdomain: Look for existing mapping only once
  irqdomain: Refactor __irq_domain_alloc_irqs()
  irqdomain: Fix mapping-creation race
  irqdomain: Clean up irq_domain_push/pop_irq()
  x86/ioapic: Use irq_domain_create_hierarchy()
  x86/apic: Use irq_domain_create_hierarchy()
  irqchip/alpine-msi: Use irq_domain_add_hierarchy()
  irqchip/gic-v2m: Use irq_domain_create_hierarchy()
  irqchip/gic-v3-its: Use irq_domain_create_hierarchy()
  irqchip/gic-v3-mbi: Use irq_domain_create_hierarchy()
  irqchip/loongson-pch-msi: Use irq_domain_create_hierarchy()
  irqchip/mvebu-odmi: Use irq_domain_create_hierarchy()
  irqdomain: Switch to per-domain locking

 arch/x86/kernel/apic/io_apic.c         |   8 +-
 arch/x86/platform/uv/uv_irq.c          |   7 +-
 drivers/irqchip/irq-alpine-msi.c       |   8 +-
 drivers/irqchip/irq-gic-v2m.c          |   5 +-
 drivers/irqchip/irq-gic-v3-its.c       |  13 +-
 drivers/irqchip/irq-gic-v3-mbi.c       |   5 +-
 drivers/irqchip/irq-loongson-pch-msi.c |   9 +-
 drivers/irqchip/irq-mvebu-odmi.c       |  13 +-
 include/linux/irqdomain.h              |   6 +-
 kernel/irq/irqdomain.c                 | 341 ++++++++++++++-----------
 10 files changed, 233 insertions(+), 182 deletions(-)

-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 01/19] irqdomain: Drop bogus fwspec-mapping error handling

[Johan Hovold]

In case a newly allocated IRQ ever ends up not having any associated
struct irq_data it would not even be possible to dispose the mapping.

Replace the bogus disposal with a WARN_ON().

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8fe1da9614ee..bf67de1733ee 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -833,13 +833,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	}
 
 	irq_data = irq_get_irq_data(virq);
-	if (!irq_data) {
-		if (irq_domain_is_hierarchy(domain))
-			irq_domain_free_irqs(virq, 1);
-		else
-			irq_dispose_mapping(virq);
+	if (WARN_ON(!irq_data))
 		return 0;
-	}
 
 	/* Store trigger type */
 	irqd_set_trigger_type(irq_data, type);
-- 
2.38.2

[PATCH v4 01/19] irqdomain: Drop bogus fwspec-mapping error handling

[Johan Hovold]

In case a newly allocated IRQ ever ends up not having any associated
struct irq_data it would not even be possible to dispose the mapping.

Replace the bogus disposal with a WARN_ON().

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8fe1da9614ee..bf67de1733ee 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -833,13 +833,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	}
 
 	irq_data = irq_get_irq_data(virq);
-	if (!irq_data) {
-		if (irq_domain_is_hierarchy(domain))
-			irq_domain_free_irqs(virq, 1);
-		else
-			irq_dispose_mapping(virq);
+	if (WARN_ON(!irq_data))
 		return 0;
-	}
 
 	/* Store trigger type */
 	irqd_set_trigger_type(irq_data, type);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 01/19] irqdomain: Drop bogus fwspec-mapping error handling

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> In case a newly allocated IRQ ever ends up not having any associated
> struct irq_data it would not even be possible to dispose the mapping.
>
> Replace the bogus disposal with a WARN_ON().
>

This lacks a Fixes tag especially because this ends up in the dependency
chain of the race fixes.

Fixes: 1e2a7d78499e ("irqdomain: Don't set type when mapping an IRQ")

is not completely precise but close enough.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 01/19] irqdomain: Drop bogus fwspec-mapping error handling

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> In case a newly allocated IRQ ever ends up not having any associated
> struct irq_data it would not even be possible to dispose the mapping.
>
> Replace the bogus disposal with a WARN_ON().
>

This lacks a Fixes tag especially because this ends up in the dependency
chain of the race fixes.

Fixes: 1e2a7d78499e ("irqdomain: Don't set type when mapping an IRQ")

is not completely precise but close enough.

Thanks,

        tglx

[PATCH v4 02/19] irqdomain: Drop dead domain-name assignment

[Johan Hovold]

Since commit d59f6617eef0 ("genirq: Allow fwnode to carry name
information only") an IRQ domain is always given a name during
allocation (e.g. used for the debugfs entry).

Drop the leftover name assignment when allocating the first IRQ.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index bf67de1733ee..fe9ec53fe7aa 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -593,10 +593,6 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 			mutex_unlock(&irq_domain_mutex);
 			return ret;
 		}
-
-		/* If not already assigned, give the domain the chip's name */
-		if (!domain->name && irq_data->chip)
-			domain->name = irq_data->chip->name;
 	}
 
 	domain->mapcount++;
@@ -1118,10 +1114,6 @@ static void irq_domain_insert_irq(int virq)
 
 		domain->mapcount++;
 		irq_domain_set_mapping(domain, data->hwirq, data);
-
-		/* If not already assigned, give the domain the chip's name */
-		if (!domain->name && data->chip)
-			domain->name = data->chip->name;
 	}
 
 	irq_clear_status_flags(virq, IRQ_NOREQUEST);
-- 
2.38.2

[PATCH v4 02/19] irqdomain: Drop dead domain-name assignment

[Johan Hovold]

Since commit d59f6617eef0 ("genirq: Allow fwnode to carry name
information only") an IRQ domain is always given a name during
allocation (e.g. used for the debugfs entry).

Drop the leftover name assignment when allocating the first IRQ.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index bf67de1733ee..fe9ec53fe7aa 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -593,10 +593,6 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 			mutex_unlock(&irq_domain_mutex);
 			return ret;
 		}
-
-		/* If not already assigned, give the domain the chip's name */
-		if (!domain->name && irq_data->chip)
-			domain->name = irq_data->chip->name;
 	}
 
 	domain->mapcount++;
@@ -1118,10 +1114,6 @@ static void irq_domain_insert_irq(int virq)
 
 		domain->mapcount++;
 		irq_domain_set_mapping(domain, data->hwirq, data);
-
-		/* If not already assigned, give the domain the chip's name */
-		if (!domain->name && data->chip)
-			domain->name = data->chip->name;
 	}
 
 	irq_clear_status_flags(virq, IRQ_NOREQUEST);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/19] irqdomain: Drop dead domain-name assignment

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Since commit d59f6617eef0 ("genirq: Allow fwnode to carry name
> information only") an IRQ domain is always given a name during
> allocation (e.g. used for the debugfs entry).
>
> Drop the leftover name assignment when allocating the first IRQ.

Wants a Fixes tag too for the same reason as before.

Fixes: d59f6617eef0 ("genirq: Allow fwnode to carry name information only")

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/19] irqdomain: Drop dead domain-name assignment

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Since commit d59f6617eef0 ("genirq: Allow fwnode to carry name
> information only") an IRQ domain is always given a name during
> allocation (e.g. used for the debugfs entry).
>
> Drop the leftover name assignment when allocating the first IRQ.

Wants a Fixes tag too for the same reason as before.

Fixes: d59f6617eef0 ("genirq: Allow fwnode to carry name information only")

Thanks,

        tglx

[PATCH v4 03/19] irqdomain: Drop leftover brackets

[Johan Hovold]

Drop some unnecessary brackets that were left in place when the
corresponding code was updated.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index fe9ec53fe7aa..dfd60bd49109 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -219,9 +219,8 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 	domain->host_data = host_data;
 	domain->hwirq_max = hwirq_max;
 
-	if (direct_max) {
+	if (direct_max)
 		domain->flags |= IRQ_DOMAIN_FLAG_NO_MAP;
-	}
 
 	domain->revmap_size = size;
 
@@ -615,9 +614,8 @@ void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
 	pr_debug("%s(%s, irqbase=%i, hwbase=%i, count=%i)\n", __func__,
 		of_node_full_name(of_node), irq_base, (int)hwirq_base, count);
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < count; i++)
 		irq_domain_associate(domain, irq_base + i, hwirq_base + i);
-	}
 }
 EXPORT_SYMBOL_GPL(irq_domain_associate_many);
 
-- 
2.38.2

[PATCH v4 03/19] irqdomain: Drop leftover brackets

[Johan Hovold]

Drop some unnecessary brackets that were left in place when the
corresponding code was updated.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index fe9ec53fe7aa..dfd60bd49109 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -219,9 +219,8 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 	domain->host_data = host_data;
 	domain->hwirq_max = hwirq_max;
 
-	if (direct_max) {
+	if (direct_max)
 		domain->flags |= IRQ_DOMAIN_FLAG_NO_MAP;
-	}
 
 	domain->revmap_size = size;
 
@@ -615,9 +614,8 @@ void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
 	pr_debug("%s(%s, irqbase=%i, hwbase=%i, count=%i)\n", __func__,
 		of_node_full_name(of_node), irq_base, (int)hwirq_base, count);
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < count; i++)
 		irq_domain_associate(domain, irq_base + i, hwirq_base + i);
-	}
 }
 EXPORT_SYMBOL_GPL(irq_domain_associate_many);
 
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/19] irqdomain: Drop leftover brackets

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Drop some unnecessary brackets that were left in place when the
> corresponding code was updated.

This is really the wrong patch order. Real fixes first, then the
cleanups and cosmetics.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/19] irqdomain: Drop leftover brackets

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Drop some unnecessary brackets that were left in place when the
> corresponding code was updated.

This is really the wrong patch order. Real fixes first, then the
cleanups and cosmetics.

Re: [PATCH v4 03/19] irqdomain: Drop leftover brackets

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:19:02PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > Drop some unnecessary brackets that were left in place when the
> > corresponding code was updated.
> 
> This is really the wrong patch order. Real fixes first, then the
> cleanups and cosmetics.

Yeah, that's what I'd normally do. Perhaps I took Marc's:

	No, let's put the code in shape *first*, then add work on the
	locking, as it should make the patch simpler. Backports aren't
	my concern, really.

	https://lore.kernel.org/lkml/87v8rhwf9r.wl-maz@kernel.org/

too literally.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/19] irqdomain: Drop leftover brackets

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:19:02PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > Drop some unnecessary brackets that were left in place when the
> > corresponding code was updated.
> 
> This is really the wrong patch order. Real fixes first, then the
> cleanups and cosmetics.

Yeah, that's what I'd normally do. Perhaps I took Marc's:

	No, let's put the code in shape *first*, then add work on the
	locking, as it should make the patch simpler. Backports aren't
	my concern, really.

	https://lore.kernel.org/lkml/87v8rhwf9r.wl-maz@kernel.org/

too literally.

Johan

[PATCH v4 04/19] irqdomain: Fix association race

[Johan Hovold]

The sanity check for an already mapped virq was done outside of the
irq_domain_mutex-protected section which meant that an (unlikely) racing
association may not be detected.

Fix this by factoring out the association implementation, which will
also be used in follow-on changes to rework the locking.

Fixes: ddaf144c61da ("irqdomain: Refactor irq_domain_associate_many()")
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index dfd60bd49109..b2087f55a1ac 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -558,8 +558,8 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 	irq_domain_clear_mapping(domain, hwirq);
 }
 
-int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
-			 irq_hw_number_t hwirq)
+static int __irq_domain_associate(struct irq_domain *domain, unsigned int virq,
+				  irq_hw_number_t hwirq)
 {
 	struct irq_data *irq_data = irq_get_irq_data(virq);
 	int ret;
@@ -572,7 +572,6 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 	if (WARN(irq_data->domain, "error: virq%i is already associated", virq))
 		return -EINVAL;
 
-	mutex_lock(&irq_domain_mutex);
 	irq_data->hwirq = hwirq;
 	irq_data->domain = domain;
 	if (domain->ops->map) {
@@ -589,19 +588,29 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 			}
 			irq_data->domain = NULL;
 			irq_data->hwirq = 0;
-			mutex_unlock(&irq_domain_mutex);
 			return ret;
 		}
 	}
 
 	domain->mapcount++;
 	irq_domain_set_mapping(domain, hwirq, irq_data);
-	mutex_unlock(&irq_domain_mutex);
 
 	irq_clear_status_flags(virq, IRQ_NOREQUEST);
 
 	return 0;
 }
+
+int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq)
+{
+	int ret;
+
+	mutex_lock(&irq_domain_mutex);
+	ret = __irq_domain_associate(domain, virq, hwirq);
+	mutex_unlock(&irq_domain_mutex);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(irq_domain_associate);
 
 void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
-- 
2.38.2

[PATCH v4 04/19] irqdomain: Fix association race

[Johan Hovold]

The sanity check for an already mapped virq was done outside of the
irq_domain_mutex-protected section which meant that an (unlikely) racing
association may not be detected.

Fix this by factoring out the association implementation, which will
also be used in follow-on changes to rework the locking.

Fixes: ddaf144c61da ("irqdomain: Refactor irq_domain_associate_many()")
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index dfd60bd49109..b2087f55a1ac 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -558,8 +558,8 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 	irq_domain_clear_mapping(domain, hwirq);
 }
 
-int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
-			 irq_hw_number_t hwirq)
+static int __irq_domain_associate(struct irq_domain *domain, unsigned int virq,
+				  irq_hw_number_t hwirq)
 {
 	struct irq_data *irq_data = irq_get_irq_data(virq);
 	int ret;
@@ -572,7 +572,6 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 	if (WARN(irq_data->domain, "error: virq%i is already associated", virq))
 		return -EINVAL;
 
-	mutex_lock(&irq_domain_mutex);
 	irq_data->hwirq = hwirq;
 	irq_data->domain = domain;
 	if (domain->ops->map) {
@@ -589,19 +588,29 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 			}
 			irq_data->domain = NULL;
 			irq_data->hwirq = 0;
-			mutex_unlock(&irq_domain_mutex);
 			return ret;
 		}
 	}
 
 	domain->mapcount++;
 	irq_domain_set_mapping(domain, hwirq, irq_data);
-	mutex_unlock(&irq_domain_mutex);
 
 	irq_clear_status_flags(virq, IRQ_NOREQUEST);
 
 	return 0;
 }
+
+int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
+			 irq_hw_number_t hwirq)
+{
+	int ret;
+
+	mutex_lock(&irq_domain_mutex);
+	ret = __irq_domain_associate(domain, virq, hwirq);
+	mutex_unlock(&irq_domain_mutex);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(irq_domain_associate);
 
 void irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 04/19] irqdomain: Fix association race

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> The sanity check for an already mapped virq was done outside of the

s/was/is/

> irq_domain_mutex-protected section which meant that an (unlikely) racing

s/meant/means/

> association may not be detected.

You describe the status quo and then correctly what the fix is:

> Fix this by factoring out the association implementation, which will
> also be used in follow-on changes to rework the locking.

Otherwise your fix fixes something which existed in the past, no?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 04/19] irqdomain: Fix association race

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> The sanity check for an already mapped virq was done outside of the

s/was/is/

> irq_domain_mutex-protected section which meant that an (unlikely) racing

s/meant/means/

> association may not be detected.

You describe the status quo and then correctly what the fix is:

> Fix this by factoring out the association implementation, which will
> also be used in follow-on changes to rework the locking.

Otherwise your fix fixes something which existed in the past, no?

Thanks,

        tglx

[PATCH v4 05/19] irqdomain: Fix disassociation race

[Johan Hovold]

The global irq_domain_mutex is held when mapping interrupts from
non-hierarchical domains but currently not when disposing them.

This specifically means that updates of the domain mapcount is racy
(currently only used for statistics in debugfs).

Make sure to hold the global irq_domain_mutex also when disposing
mappings from non-hierarchical domains.

Fixes: 9dc6be3d4193 ("genirq/irqdomain: Add map counter")
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index b2087f55a1ac..23f5919e58b7 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -537,6 +537,9 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 		return;
 
 	hwirq = irq_data->hwirq;
+
+	mutex_lock(&irq_domain_mutex);
+
 	irq_set_status_flags(irq, IRQ_NOREQUEST);
 
 	/* remove chip and handler */
@@ -556,6 +559,8 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 
 	/* Clear reverse map for this hwirq */
 	irq_domain_clear_mapping(domain, hwirq);
+
+	mutex_unlock(&irq_domain_mutex);
 }
 
 static int __irq_domain_associate(struct irq_domain *domain, unsigned int virq,
-- 
2.38.2

[PATCH v4 05/19] irqdomain: Fix disassociation race

[Johan Hovold]

The global irq_domain_mutex is held when mapping interrupts from
non-hierarchical domains but currently not when disposing them.

This specifically means that updates of the domain mapcount is racy
(currently only used for statistics in debugfs).

Make sure to hold the global irq_domain_mutex also when disposing
mappings from non-hierarchical domains.

Fixes: 9dc6be3d4193 ("genirq/irqdomain: Add map counter")
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index b2087f55a1ac..23f5919e58b7 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -537,6 +537,9 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 		return;
 
 	hwirq = irq_data->hwirq;
+
+	mutex_lock(&irq_domain_mutex);
+
 	irq_set_status_flags(irq, IRQ_NOREQUEST);
 
 	/* remove chip and handler */
@@ -556,6 +559,8 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 
 	/* Clear reverse map for this hwirq */
 	irq_domain_clear_mapping(domain, hwirq);
+
+	mutex_unlock(&irq_domain_mutex);
 }
 
 static int __irq_domain_associate(struct irq_domain *domain, unsigned int virq,
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

The global irq_domain_mutex is now held in all paths that update the
revmap structures so there is no longer any need for the revmap mutex.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 include/linux/irqdomain.h |  2 --
 kernel/irq/irqdomain.c    | 13 ++++++-------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 5b2718e1c6de..7fd3939328c2 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -143,7 +143,6 @@ struct irq_domain_chip_generic;
  * Revmap data, used internally by the irq domain code:
  * @revmap_size:	Size of the linear map table @revmap[]
  * @revmap_tree:	Radix map tree for hwirqs that don't fit in the linear map
- * @revmap_mutex:	Lock for the revmap
  * @revmap:		Linear table of irq_data pointers
  */
 struct irq_domain {
@@ -171,7 +170,6 @@ struct irq_domain {
 	irq_hw_number_t			hwirq_max;
 	unsigned int			revmap_size;
 	struct radix_tree_root		revmap_tree;
-	struct mutex			revmap_mutex;
 	struct irq_data __rcu		*revmap[];
 };
 
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 23f5919e58b7..248e6acfafbe 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -214,7 +214,6 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 
 	/* Fill structure */
 	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
-	mutex_init(&domain->revmap_mutex);
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->hwirq_max = hwirq_max;
@@ -501,30 +500,30 @@ static bool irq_domain_is_nomap(struct irq_domain *domain)
 static void irq_domain_clear_mapping(struct irq_domain *domain,
 				     irq_hw_number_t hwirq)
 {
+	lockdep_assert_held(&irq_domain_mutex);
+
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	mutex_lock(&domain->revmap_mutex);
 	if (hwirq < domain->revmap_size)
 		rcu_assign_pointer(domain->revmap[hwirq], NULL);
 	else
 		radix_tree_delete(&domain->revmap_tree, hwirq);
-	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_set_mapping(struct irq_domain *domain,
 				   irq_hw_number_t hwirq,
 				   struct irq_data *irq_data)
 {
+	lockdep_assert_held(&irq_domain_mutex);
+
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	mutex_lock(&domain->revmap_mutex);
 	if (hwirq < domain->revmap_size)
 		rcu_assign_pointer(domain->revmap[hwirq], irq_data);
 	else
 		radix_tree_insert(&domain->revmap_tree, hwirq, irq_data);
-	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
@@ -1511,11 +1510,12 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 {
 	void __rcu **slot;
 
+	lockdep_assert_held(&irq_domain_mutex);
+
 	if (irq_domain_is_nomap(d->domain))
 		return;
 
 	/* Fix up the revmap. */
-	mutex_lock(&d->domain->revmap_mutex);
 	if (d->hwirq < d->domain->revmap_size) {
 		/* Not using radix tree */
 		rcu_assign_pointer(d->domain->revmap[d->hwirq], d);
@@ -1524,7 +1524,6 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 		if (slot)
 			radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
 	}
-	mutex_unlock(&d->domain->revmap_mutex);
 }
 
 /**
-- 
2.38.2

[PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

The global irq_domain_mutex is now held in all paths that update the
revmap structures so there is no longer any need for the revmap mutex.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 include/linux/irqdomain.h |  2 --
 kernel/irq/irqdomain.c    | 13 ++++++-------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 5b2718e1c6de..7fd3939328c2 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -143,7 +143,6 @@ struct irq_domain_chip_generic;
  * Revmap data, used internally by the irq domain code:
  * @revmap_size:	Size of the linear map table @revmap[]
  * @revmap_tree:	Radix map tree for hwirqs that don't fit in the linear map
- * @revmap_mutex:	Lock for the revmap
  * @revmap:		Linear table of irq_data pointers
  */
 struct irq_domain {
@@ -171,7 +170,6 @@ struct irq_domain {
 	irq_hw_number_t			hwirq_max;
 	unsigned int			revmap_size;
 	struct radix_tree_root		revmap_tree;
-	struct mutex			revmap_mutex;
 	struct irq_data __rcu		*revmap[];
 };
 
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 23f5919e58b7..248e6acfafbe 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -214,7 +214,6 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 
 	/* Fill structure */
 	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
-	mutex_init(&domain->revmap_mutex);
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->hwirq_max = hwirq_max;
@@ -501,30 +500,30 @@ static bool irq_domain_is_nomap(struct irq_domain *domain)
 static void irq_domain_clear_mapping(struct irq_domain *domain,
 				     irq_hw_number_t hwirq)
 {
+	lockdep_assert_held(&irq_domain_mutex);
+
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	mutex_lock(&domain->revmap_mutex);
 	if (hwirq < domain->revmap_size)
 		rcu_assign_pointer(domain->revmap[hwirq], NULL);
 	else
 		radix_tree_delete(&domain->revmap_tree, hwirq);
-	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_set_mapping(struct irq_domain *domain,
 				   irq_hw_number_t hwirq,
 				   struct irq_data *irq_data)
 {
+	lockdep_assert_held(&irq_domain_mutex);
+
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	mutex_lock(&domain->revmap_mutex);
 	if (hwirq < domain->revmap_size)
 		rcu_assign_pointer(domain->revmap[hwirq], irq_data);
 	else
 		radix_tree_insert(&domain->revmap_tree, hwirq, irq_data);
-	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
@@ -1511,11 +1510,12 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 {
 	void __rcu **slot;
 
+	lockdep_assert_held(&irq_domain_mutex);
+
 	if (irq_domain_is_nomap(d->domain))
 		return;
 
 	/* Fix up the revmap. */
-	mutex_lock(&d->domain->revmap_mutex);
 	if (d->hwirq < d->domain->revmap_size) {
 		/* Not using radix tree */
 		rcu_assign_pointer(d->domain->revmap[d->hwirq], d);
@@ -1524,7 +1524,6 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 		if (slot)
 			radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
 	}
-	mutex_unlock(&d->domain->revmap_mutex);
 }
 
 /**
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> The global irq_domain_mutex is now held in all paths that update the
> revmap structures so there is no longer any need for the revmap mutex.

This can also go after the 3rd race fix, but ...

>  static void irq_domain_clear_mapping(struct irq_domain *domain,
>  				     irq_hw_number_t hwirq)
>  {
> +	lockdep_assert_held(&irq_domain_mutex);

these lockdep asserts want to be part of the [dis]association race
fixes. They are completely unrelated to the removal of the revmap_mutex.

Your race fixes change the locking and you want to ensure that all
callers comply right there, no?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> The global irq_domain_mutex is now held in all paths that update the
> revmap structures so there is no longer any need for the revmap mutex.

This can also go after the 3rd race fix, but ...

>  static void irq_domain_clear_mapping(struct irq_domain *domain,
>  				     irq_hw_number_t hwirq)
>  {
> +	lockdep_assert_held(&irq_domain_mutex);

these lockdep asserts want to be part of the [dis]association race
fixes. They are completely unrelated to the removal of the revmap_mutex.

Your race fixes change the locking and you want to ensure that all
callers comply right there, no?

Thanks,

        tglx

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:23:20PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > The global irq_domain_mutex is now held in all paths that update the
> > revmap structures so there is no longer any need for the revmap mutex.
> 
> This can also go after the 3rd race fix, but ...
> 
> >  static void irq_domain_clear_mapping(struct irq_domain *domain,
> >  				     irq_hw_number_t hwirq)
> >  {
> > +	lockdep_assert_held(&irq_domain_mutex);
> 
> these lockdep asserts want to be part of the [dis]association race
> fixes. They are completely unrelated to the removal of the revmap_mutex.

No, they are very much related to the removal of the revmap_mutex. These
functions deal with the revmap structures which before this patch were
clearly only modified with the revmap_mutex held.

The lockdep assert is here to guarantee that my claim that all current
(and future) paths that end up modifying these structures do so under
the irq_domain_mutex instead.

> Your race fixes change the locking and you want to ensure that all
> callers comply right there, no?

I want to make sure that all callers of these function comply, yes.
That's why the asserts belong in this patch.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:23:20PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > The global irq_domain_mutex is now held in all paths that update the
> > revmap structures so there is no longer any need for the revmap mutex.
> 
> This can also go after the 3rd race fix, but ...
> 
> >  static void irq_domain_clear_mapping(struct irq_domain *domain,
> >  				     irq_hw_number_t hwirq)
> >  {
> > +	lockdep_assert_held(&irq_domain_mutex);
> 
> these lockdep asserts want to be part of the [dis]association race
> fixes. They are completely unrelated to the removal of the revmap_mutex.

No, they are very much related to the removal of the revmap_mutex. These
functions deal with the revmap structures which before this patch were
clearly only modified with the revmap_mutex held.

The lockdep assert is here to guarantee that my claim that all current
(and future) paths that end up modifying these structures do so under
the irq_domain_mutex instead.

> Your race fixes change the locking and you want to ensure that all
> callers comply right there, no?

I want to make sure that all callers of these function comply, yes.
That's why the asserts belong in this patch.

Johan

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Thomas Gleixner]

On Wed, Jan 18 2023 at 10:22, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:23:20PM +0100, Thomas Gleixner wrote:
>> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>> > The global irq_domain_mutex is now held in all paths that update the
>> > revmap structures so there is no longer any need for the revmap mutex.
>> 
>> This can also go after the 3rd race fix, but ...
>> 
>> >  static void irq_domain_clear_mapping(struct irq_domain *domain,
>> >  				     irq_hw_number_t hwirq)
>> >  {
>> > +	lockdep_assert_held(&irq_domain_mutex);
>> 
>> these lockdep asserts want to be part of the [dis]association race
>> fixes. They are completely unrelated to the removal of the revmap_mutex.
>
> No, they are very much related to the removal of the revmap_mutex. These
> functions deal with the revmap structures which before this patch were
> clearly only modified with the revmap_mutex held.
>
> The lockdep assert is here to guarantee that my claim that all current
> (and future) paths that end up modifying these structures do so under
> the irq_domain_mutex instead.
>
>> Your race fixes change the locking and you want to ensure that all
>> callers comply right there, no?
>
> I want to make sure that all callers of these function comply, yes.
> That's why the asserts belong in this patch.

You can do this in a two step approach.

    1) Add the new locking and ensure that the lock is held when
       the functions are called

    2) Safely remove the revmap_mutex because you already established
       that revmap is protected by some other means

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Thomas Gleixner]

On Wed, Jan 18 2023 at 10:22, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:23:20PM +0100, Thomas Gleixner wrote:
>> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>> > The global irq_domain_mutex is now held in all paths that update the
>> > revmap structures so there is no longer any need for the revmap mutex.
>> 
>> This can also go after the 3rd race fix, but ...
>> 
>> >  static void irq_domain_clear_mapping(struct irq_domain *domain,
>> >  				     irq_hw_number_t hwirq)
>> >  {
>> > +	lockdep_assert_held(&irq_domain_mutex);
>> 
>> these lockdep asserts want to be part of the [dis]association race
>> fixes. They are completely unrelated to the removal of the revmap_mutex.
>
> No, they are very much related to the removal of the revmap_mutex. These
> functions deal with the revmap structures which before this patch were
> clearly only modified with the revmap_mutex held.
>
> The lockdep assert is here to guarantee that my claim that all current
> (and future) paths that end up modifying these structures do so under
> the irq_domain_mutex instead.
>
>> Your race fixes change the locking and you want to ensure that all
>> callers comply right there, no?
>
> I want to make sure that all callers of these function comply, yes.
> That's why the asserts belong in this patch.

You can do this in a two step approach.

    1) Add the new locking and ensure that the lock is held when
       the functions are called

    2) Safely remove the revmap_mutex because you already established
       that revmap is protected by some other means

Thanks,

        tglx

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

On Wed, Jan 18, 2023 at 02:05:29PM +0100, Thomas Gleixner wrote:
> On Wed, Jan 18 2023 at 10:22, Johan Hovold wrote:
> > On Tue, Jan 17, 2023 at 10:23:20PM +0100, Thomas Gleixner wrote:
> >> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> >> > The global irq_domain_mutex is now held in all paths that update the
> >> > revmap structures so there is no longer any need for the revmap mutex.
> >> 
> >> This can also go after the 3rd race fix, but ...
> >> 
> >> >  static void irq_domain_clear_mapping(struct irq_domain *domain,
> >> >  				     irq_hw_number_t hwirq)
> >> >  {
> >> > +	lockdep_assert_held(&irq_domain_mutex);
> >> 
> >> these lockdep asserts want to be part of the [dis]association race
> >> fixes. They are completely unrelated to the removal of the revmap_mutex.
> >
> > No, they are very much related to the removal of the revmap_mutex. These
> > functions deal with the revmap structures which before this patch were
> > clearly only modified with the revmap_mutex held.
> >
> > The lockdep assert is here to guarantee that my claim that all current
> > (and future) paths that end up modifying these structures do so under
> > the irq_domain_mutex instead.
> >
> >> Your race fixes change the locking and you want to ensure that all
> >> callers comply right there, no?
> >
> > I want to make sure that all callers of these function comply, yes.
> > That's why the asserts belong in this patch.
> 
> You can do this in a two step approach.
> 
>     1) Add the new locking and ensure that the lock is held when
>        the functions are called

But the new locking has nothing to do with these functions. They are
added because they fix various races elsewhere. Adding lockdep
assertions in unrelated function as part of those fixes doesn't really
make much sense.

>     2) Safely remove the revmap_mutex because you already established
>        that revmap is protected by some other means

I still think it belongs in this patch.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

On Wed, Jan 18, 2023 at 02:05:29PM +0100, Thomas Gleixner wrote:
> On Wed, Jan 18 2023 at 10:22, Johan Hovold wrote:
> > On Tue, Jan 17, 2023 at 10:23:20PM +0100, Thomas Gleixner wrote:
> >> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> >> > The global irq_domain_mutex is now held in all paths that update the
> >> > revmap structures so there is no longer any need for the revmap mutex.
> >> 
> >> This can also go after the 3rd race fix, but ...
> >> 
> >> >  static void irq_domain_clear_mapping(struct irq_domain *domain,
> >> >  				     irq_hw_number_t hwirq)
> >> >  {
> >> > +	lockdep_assert_held(&irq_domain_mutex);
> >> 
> >> these lockdep asserts want to be part of the [dis]association race
> >> fixes. They are completely unrelated to the removal of the revmap_mutex.
> >
> > No, they are very much related to the removal of the revmap_mutex. These
> > functions deal with the revmap structures which before this patch were
> > clearly only modified with the revmap_mutex held.
> >
> > The lockdep assert is here to guarantee that my claim that all current
> > (and future) paths that end up modifying these structures do so under
> > the irq_domain_mutex instead.
> >
> >> Your race fixes change the locking and you want to ensure that all
> >> callers comply right there, no?
> >
> > I want to make sure that all callers of these function comply, yes.
> > That's why the asserts belong in this patch.
> 
> You can do this in a two step approach.
> 
>     1) Add the new locking and ensure that the lock is held when
>        the functions are called

But the new locking has nothing to do with these functions. They are
added because they fix various races elsewhere. Adding lockdep
assertions in unrelated function as part of those fixes doesn't really
make much sense.

>     2) Safely remove the revmap_mutex because you already established
>        that revmap is protected by some other means

I still think it belongs in this patch.

Johan

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Thomas Gleixner]

On Wed, Jan 18 2023 at 14:10, Johan Hovold wrote:
> On Wed, Jan 18, 2023 at 02:05:29PM +0100, Thomas Gleixner wrote:
>> You can do this in a two step approach.
>> 
>>     1) Add the new locking and ensure that the lock is held when
>>        the functions are called
>
> But the new locking has nothing to do with these functions. They are
> added because they fix various races elsewhere. Adding lockdep
> assertions in unrelated function as part of those fixes doesn't really
> make much sense.

Seriously? The point is that revmap_mutex is not protecting against any
of the races which you are trying to protect against. Ergo, any callsite
which does not hold the irqdomain mutex is part of the problem you are
trying to solve, no?

The removal of the revmap_mutex becomes possible due to the overall
protection scheme rework _after_ you established that all callers hold
the irqdomain mutex.

Thanks,

        tglx

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Thomas Gleixner]

On Wed, Jan 18 2023 at 14:10, Johan Hovold wrote:
> On Wed, Jan 18, 2023 at 02:05:29PM +0100, Thomas Gleixner wrote:
>> You can do this in a two step approach.
>> 
>>     1) Add the new locking and ensure that the lock is held when
>>        the functions are called
>
> But the new locking has nothing to do with these functions. They are
> added because they fix various races elsewhere. Adding lockdep
> assertions in unrelated function as part of those fixes doesn't really
> make much sense.

Seriously? The point is that revmap_mutex is not protecting against any
of the races which you are trying to protect against. Ergo, any callsite
which does not hold the irqdomain mutex is part of the problem you are
trying to solve, no?

The removal of the revmap_mutex becomes possible due to the overall
protection scheme rework _after_ you established that all callers hold
the irqdomain mutex.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

On Mon, Feb 06, 2023 at 02:09:02PM +0100, Thomas Gleixner wrote:
> On Wed, Jan 18 2023 at 14:10, Johan Hovold wrote:
> > On Wed, Jan 18, 2023 at 02:05:29PM +0100, Thomas Gleixner wrote:
> >> You can do this in a two step approach.
> >> 
> >>     1) Add the new locking and ensure that the lock is held when
> >>        the functions are called
> >
> > But the new locking has nothing to do with these functions. They are
> > added because they fix various races elsewhere. Adding lockdep
> > assertions in unrelated function as part of those fixes doesn't really
> > make much sense.
> 
> Seriously? The point is that revmap_mutex is not protecting against any
> of the races which you are trying to protect against. Ergo, any callsite
> which does not hold the irqdomain mutex is part of the problem you are
> trying to solve, no?

The current locking using the revmap_mutex is indeed incomplete as it
only serialises updates of the revmap structures themselves and
specifically does not prevent two mappings for the same interrupt to be
created. It basically just protects the integrity of the revmap tree.

The association and disassocation fixes are not sufficient to address the
mapping race, but those two changes do guarantee that all current paths
that access the revmap structures hold the irq_domain_mutex.

Once that has been established, there is no point in keeping the
revmap_mutex around for the update path (lookups are still protected by
RCU).

But this change is separate from the race that the disassociation fix
addressed (e.g. the racy updates of the domain mapcount) and it is also
independent of the fix for the mapping race (which still exists after
removing the revmap mutex with the current order of the patches).

Therefore the dropping the revmap mutex belongs in its own patch and I
placed it here after the disassociation fix to highlight that it is
indeed independent of the later fixes for the mapping race.

It can be moved after, but I still think the lockdep asserts belong in
the patch that removes the revmap tree lock.

Johan

Re: [PATCH v4 06/19] irqdomain: Drop revmap mutex

[Johan Hovold]

On Mon, Feb 06, 2023 at 02:09:02PM +0100, Thomas Gleixner wrote:
> On Wed, Jan 18 2023 at 14:10, Johan Hovold wrote:
> > On Wed, Jan 18, 2023 at 02:05:29PM +0100, Thomas Gleixner wrote:
> >> You can do this in a two step approach.
> >> 
> >>     1) Add the new locking and ensure that the lock is held when
> >>        the functions are called
> >
> > But the new locking has nothing to do with these functions. They are
> > added because they fix various races elsewhere. Adding lockdep
> > assertions in unrelated function as part of those fixes doesn't really
> > make much sense.
> 
> Seriously? The point is that revmap_mutex is not protecting against any
> of the races which you are trying to protect against. Ergo, any callsite
> which does not hold the irqdomain mutex is part of the problem you are
> trying to solve, no?

The current locking using the revmap_mutex is indeed incomplete as it
only serialises updates of the revmap structures themselves and
specifically does not prevent two mappings for the same interrupt to be
created. It basically just protects the integrity of the revmap tree.

The association and disassocation fixes are not sufficient to address the
mapping race, but those two changes do guarantee that all current paths
that access the revmap structures hold the irq_domain_mutex.

Once that has been established, there is no point in keeping the
revmap_mutex around for the update path (lookups are still protected by
RCU).

But this change is separate from the race that the disassociation fix
addressed (e.g. the racy updates of the domain mapcount) and it is also
independent of the fix for the mapping race (which still exists after
removing the revmap mutex with the current order of the patches).

Therefore the dropping the revmap mutex belongs in its own patch and I
placed it here after the disassociation fix to highlight that it is
indeed independent of the later fixes for the mapping race.

It can be moved after, but I still think the lockdep asserts belong in
the patch that removes the revmap tree lock.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Johan Hovold]

Avoid looking for an existing mapping twice when creating a new mapping
using irq_create_fwspec_mapping() by factoring out the actual allocation
which is shared with irq_create_mapping_affinity().

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 60 +++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 248e6acfafbe..894bc6ee6348 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -675,6 +675,34 @@ unsigned int irq_create_direct_mapping(struct irq_domain *domain)
 EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
 #endif
 
+static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
+						  irq_hw_number_t hwirq,
+						  const struct irq_affinity_desc *affinity)
+{
+	struct device_node *of_node = irq_domain_get_of_node(domain);
+	int virq;
+
+	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
+
+	/* Allocate a virtual interrupt number */
+	virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
+				      affinity);
+	if (virq <= 0) {
+		pr_debug("-> virq allocation failed\n");
+		return 0;
+	}
+
+	if (irq_domain_associate(domain, virq, hwirq)) {
+		irq_free_desc(virq);
+		return 0;
+	}
+
+	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
+		hwirq, of_node_full_name(of_node), virq);
+
+	return virq;
+}
+
 /**
  * irq_create_mapping_affinity() - Map a hardware interrupt into linux irq space
  * @domain: domain owning this hardware interrupt or NULL for default domain
@@ -687,14 +715,11 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
  * on the number returned from that call.
  */
 unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
-				       irq_hw_number_t hwirq,
-				       const struct irq_affinity_desc *affinity)
+					 irq_hw_number_t hwirq,
+					 const struct irq_affinity_desc *affinity)
 {
-	struct device_node *of_node;
 	int virq;
 
-	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
-
 	/* Look for default domain if necessary */
 	if (domain == NULL)
 		domain = irq_default_domain;
@@ -702,34 +727,15 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 		WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
 		return 0;
 	}
-	pr_debug("-> using domain @%p\n", domain);
-
-	of_node = irq_domain_get_of_node(domain);
 
 	/* Check if mapping already exists */
 	virq = irq_find_mapping(domain, hwirq);
 	if (virq) {
-		pr_debug("-> existing mapping on virq %d\n", virq);
+		pr_debug("existing mapping on virq %d\n", virq);
 		return virq;
 	}
 
-	/* Allocate a virtual interrupt number */
-	virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
-				      affinity);
-	if (virq <= 0) {
-		pr_debug("-> virq allocation failed\n");
-		return 0;
-	}
-
-	if (irq_domain_associate(domain, virq, hwirq)) {
-		irq_free_desc(virq);
-		return 0;
-	}
-
-	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
-		hwirq, of_node_full_name(of_node), virq);
-
-	return virq;
+	return __irq_create_mapping_affinity(domain, hwirq, affinity);
 }
 EXPORT_SYMBOL_GPL(irq_create_mapping_affinity);
 
@@ -834,7 +840,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 			return 0;
 	} else {
 		/* Create mapping */
-		virq = irq_create_mapping(domain, hwirq);
+		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
 		if (!virq)
 			return virq;
 	}
-- 
2.38.2

[PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Johan Hovold]

Avoid looking for an existing mapping twice when creating a new mapping
using irq_create_fwspec_mapping() by factoring out the actual allocation
which is shared with irq_create_mapping_affinity().

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 60 +++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 248e6acfafbe..894bc6ee6348 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -675,6 +675,34 @@ unsigned int irq_create_direct_mapping(struct irq_domain *domain)
 EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
 #endif
 
+static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
+						  irq_hw_number_t hwirq,
+						  const struct irq_affinity_desc *affinity)
+{
+	struct device_node *of_node = irq_domain_get_of_node(domain);
+	int virq;
+
+	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
+
+	/* Allocate a virtual interrupt number */
+	virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
+				      affinity);
+	if (virq <= 0) {
+		pr_debug("-> virq allocation failed\n");
+		return 0;
+	}
+
+	if (irq_domain_associate(domain, virq, hwirq)) {
+		irq_free_desc(virq);
+		return 0;
+	}
+
+	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
+		hwirq, of_node_full_name(of_node), virq);
+
+	return virq;
+}
+
 /**
  * irq_create_mapping_affinity() - Map a hardware interrupt into linux irq space
  * @domain: domain owning this hardware interrupt or NULL for default domain
@@ -687,14 +715,11 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapping);
  * on the number returned from that call.
  */
 unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
-				       irq_hw_number_t hwirq,
-				       const struct irq_affinity_desc *affinity)
+					 irq_hw_number_t hwirq,
+					 const struct irq_affinity_desc *affinity)
 {
-	struct device_node *of_node;
 	int virq;
 
-	pr_debug("irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
-
 	/* Look for default domain if necessary */
 	if (domain == NULL)
 		domain = irq_default_domain;
@@ -702,34 +727,15 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 		WARN(1, "%s(, %lx) called with NULL domain\n", __func__, hwirq);
 		return 0;
 	}
-	pr_debug("-> using domain @%p\n", domain);
-
-	of_node = irq_domain_get_of_node(domain);
 
 	/* Check if mapping already exists */
 	virq = irq_find_mapping(domain, hwirq);
 	if (virq) {
-		pr_debug("-> existing mapping on virq %d\n", virq);
+		pr_debug("existing mapping on virq %d\n", virq);
 		return virq;
 	}
 
-	/* Allocate a virtual interrupt number */
-	virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
-				      affinity);
-	if (virq <= 0) {
-		pr_debug("-> virq allocation failed\n");
-		return 0;
-	}
-
-	if (irq_domain_associate(domain, virq, hwirq)) {
-		irq_free_desc(virq);
-		return 0;
-	}
-
-	pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
-		hwirq, of_node_full_name(of_node), virq);
-
-	return virq;
+	return __irq_create_mapping_affinity(domain, hwirq, affinity);
 }
 EXPORT_SYMBOL_GPL(irq_create_mapping_affinity);
 
@@ -834,7 +840,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 			return 0;
 	} else {
 		/* Create mapping */
-		virq = irq_create_mapping(domain, hwirq);
+		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
 		if (!virq)
 			return virq;
 	}
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Avoid looking for an existing mapping twice when creating a new mapping
> using irq_create_fwspec_mapping() by factoring out the actual allocation
> which is shared with irq_create_mapping_affinity().

This changelog is incomplete and it took me a while to figure out why
this is before the race fix.

The point is that you need __irq_create_mapping_affinity() later to fix
the shared mapping race. The double check avoidance is just a nice side
effect.

So please spell it out and make it clear that this needs to be
backported too, e.g. by adding:

The split out internal function will be used to fix a shared interrupt
mapping race. This change is therefore tagged with the same fixes tag.

Fixes: ....

>  
> +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
> +						  irq_hw_number_t hwirq,
> +						  const struct irq_affinity_desc *affinity)

Please rename to irq_create_mapping_affinity_locked() so it's clear what
this is about and what the calling convention is. A lockdep assert to
that effect would be nice too.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> Avoid looking for an existing mapping twice when creating a new mapping
> using irq_create_fwspec_mapping() by factoring out the actual allocation
> which is shared with irq_create_mapping_affinity().

This changelog is incomplete and it took me a while to figure out why
this is before the race fix.

The point is that you need __irq_create_mapping_affinity() later to fix
the shared mapping race. The double check avoidance is just a nice side
effect.

So please spell it out and make it clear that this needs to be
backported too, e.g. by adding:

The split out internal function will be used to fix a shared interrupt
mapping race. This change is therefore tagged with the same fixes tag.

Fixes: ....

>  
> +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
> +						  irq_hw_number_t hwirq,
> +						  const struct irq_affinity_desc *affinity)

Please rename to irq_create_mapping_affinity_locked() so it's clear what
this is about and what the calling convention is. A lockdep assert to
that effect would be nice too.

Thanks,

        tglx

Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:34:07PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > Avoid looking for an existing mapping twice when creating a new mapping
> > using irq_create_fwspec_mapping() by factoring out the actual allocation
> > which is shared with irq_create_mapping_affinity().
> 
> This changelog is incomplete and it took me a while to figure out why
> this is before the race fix.
> 
> The point is that you need __irq_create_mapping_affinity() later to fix
> the shared mapping race. The double check avoidance is just a nice side
> effect.
> 
> So please spell it out and make it clear that this needs to be
> backported too, e.g. by adding:
> 
> The split out internal function will be used to fix a shared interrupt
> mapping race. This change is therefore tagged with the same fixes tag.
> 
> Fixes: ....

Sure. It was originally part of the fix of the race, but I was told to
clean up the code first (and not worry about backporting).

I'll see what I can do about reordering these again with the aim of
making things easier to backport.

> > +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
> > +						  irq_hw_number_t hwirq,
> > +						  const struct irq_affinity_desc *affinity)
> 
> Please rename to irq_create_mapping_affinity_locked() so it's clear what
> this is about and what the calling convention is. A lockdep assert to
> that effect would be nice too.

Will do.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:34:07PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > Avoid looking for an existing mapping twice when creating a new mapping
> > using irq_create_fwspec_mapping() by factoring out the actual allocation
> > which is shared with irq_create_mapping_affinity().
> 
> This changelog is incomplete and it took me a while to figure out why
> this is before the race fix.
> 
> The point is that you need __irq_create_mapping_affinity() later to fix
> the shared mapping race. The double check avoidance is just a nice side
> effect.
> 
> So please spell it out and make it clear that this needs to be
> backported too, e.g. by adding:
> 
> The split out internal function will be used to fix a shared interrupt
> mapping race. This change is therefore tagged with the same fixes tag.
> 
> Fixes: ....

Sure. It was originally part of the fix of the race, but I was told to
clean up the code first (and not worry about backporting).

I'll see what I can do about reordering these again with the aim of
making things easier to backport.

> > +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
> > +						  irq_hw_number_t hwirq,
> > +						  const struct irq_affinity_desc *affinity)
> 
> Please rename to irq_create_mapping_affinity_locked() so it's clear what
> this is about and what the calling convention is. A lockdep assert to
> that effect would be nice too.

Will do.

Johan

Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Johan Hovold]

On Wed, Jan 18, 2023 at 10:26:16AM +0100, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:34:07PM +0100, Thomas Gleixner wrote:
> > On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > > Avoid looking for an existing mapping twice when creating a new mapping
> > > using irq_create_fwspec_mapping() by factoring out the actual allocation
> > > which is shared with irq_create_mapping_affinity().
> > 
> > This changelog is incomplete and it took me a while to figure out why
> > this is before the race fix.
> > 
> > The point is that you need __irq_create_mapping_affinity() later to fix
> > the shared mapping race. The double check avoidance is just a nice side
> > effect.
> > 
> > So please spell it out and make it clear that this needs to be
> > backported too, e.g. by adding:
> > 
> > The split out internal function will be used to fix a shared interrupt
> > mapping race. This change is therefore tagged with the same fixes tag.
> > 
> > Fixes: ....
> 
> Sure. It was originally part of the fix of the race, but I was told to
> clean up the code first (and not worry about backporting).
> 
> I'll see what I can do about reordering these again with the aim of
> making things easier to backport.
> 
> > > +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
> > > +						  irq_hw_number_t hwirq,
> > > +						  const struct irq_affinity_desc *affinity)
> > 
> > Please rename to irq_create_mapping_affinity_locked() so it's clear what
> > this is about and what the calling convention is. A lockdep assert to
> > that effect would be nice too.
> 
> Will do.

Actually this cannot be done as part of this patch as the function is
still being called without the lock held until the actual
shared-interrupt mapping fix. I have a vague recollection that this was
part of the reason I went with the double underscore prefix.

I'll rename the function using a __locked suffix as part of the race
fix, but a lockdep assert feels like overkill here as this static
function is only in two places where the lock has just been taken (and
the asserts in the revmap helper will eventually catch any future
hypothetical offenders).

Johan

Re: [PATCH v4 07/19] irqdomain: Look for existing mapping only once

[Johan Hovold]

On Wed, Jan 18, 2023 at 10:26:16AM +0100, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:34:07PM +0100, Thomas Gleixner wrote:
> > On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > > Avoid looking for an existing mapping twice when creating a new mapping
> > > using irq_create_fwspec_mapping() by factoring out the actual allocation
> > > which is shared with irq_create_mapping_affinity().
> > 
> > This changelog is incomplete and it took me a while to figure out why
> > this is before the race fix.
> > 
> > The point is that you need __irq_create_mapping_affinity() later to fix
> > the shared mapping race. The double check avoidance is just a nice side
> > effect.
> > 
> > So please spell it out and make it clear that this needs to be
> > backported too, e.g. by adding:
> > 
> > The split out internal function will be used to fix a shared interrupt
> > mapping race. This change is therefore tagged with the same fixes tag.
> > 
> > Fixes: ....
> 
> Sure. It was originally part of the fix of the race, but I was told to
> clean up the code first (and not worry about backporting).
> 
> I'll see what I can do about reordering these again with the aim of
> making things easier to backport.
> 
> > > +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
> > > +						  irq_hw_number_t hwirq,
> > > +						  const struct irq_affinity_desc *affinity)
> > 
> > Please rename to irq_create_mapping_affinity_locked() so it's clear what
> > this is about and what the calling convention is. A lockdep assert to
> > that effect would be nice too.
> 
> Will do.

Actually this cannot be done as part of this patch as the function is
still being called without the lock held until the actual
shared-interrupt mapping fix. I have a vague recollection that this was
part of the reason I went with the double underscore prefix.

I'll rename the function using a __locked suffix as part of the race
fix, but a lockdep assert feels like overkill here as this static
function is only in two places where the lock has just been taken (and
the asserts in the revmap helper will eventually catch any future
hypothetical offenders).

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 08/19] irqdomain: Refactor __irq_domain_alloc_irqs()

[Johan Hovold]

Refactor __irq_domain_alloc_irqs() so that it can be called internally
while holding the irq_domain_mutex.

This will be used to fix a shared-interrupt mapping race.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 88 +++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 40 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 894bc6ee6348..d6139b0218d4 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1430,40 +1430,12 @@ int irq_domain_alloc_irqs_hierarchy(struct irq_domain *domain,
 	return domain->ops->alloc(domain, irq_base, nr_irqs, arg);
 }
 
-/**
- * __irq_domain_alloc_irqs - Allocate IRQs from domain
- * @domain:	domain to allocate from
- * @irq_base:	allocate specified IRQ number if irq_base >= 0
- * @nr_irqs:	number of IRQs to allocate
- * @node:	NUMA node id for memory allocation
- * @arg:	domain specific argument
- * @realloc:	IRQ descriptors have already been allocated if true
- * @affinity:	Optional irq affinity mask for multiqueue devices
- *
- * Allocate IRQ numbers and initialized all data structures to support
- * hierarchy IRQ domains.
- * Parameter @realloc is mainly to support legacy IRQs.
- * Returns error code or allocated IRQ number
- *
- * The whole process to setup an IRQ has been split into two steps.
- * The first step, __irq_domain_alloc_irqs(), is to allocate IRQ
- * descriptor and required hardware resources. The second step,
- * irq_domain_activate_irq(), is to program the hardware with preallocated
- * resources. In this way, it's easier to rollback when failing to
- * allocate resources.
- */
-int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
-			    unsigned int nr_irqs, int node, void *arg,
-			    bool realloc, const struct irq_affinity_desc *affinity)
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				    unsigned int nr_irqs, int node, void *arg,
+				    bool realloc, const struct irq_affinity_desc *affinity)
 {
 	int i, ret, virq;
 
-	if (domain == NULL) {
-		domain = irq_default_domain;
-		if (WARN(!domain, "domain is NULL; cannot allocate IRQ\n"))
-			return -EINVAL;
-	}
-
 	if (realloc && irq_base >= 0) {
 		virq = irq_base;
 	} else {
@@ -1482,24 +1454,18 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 		goto out_free_desc;
 	}
 
-	mutex_lock(&irq_domain_mutex);
 	ret = irq_domain_alloc_irqs_hierarchy(domain, virq, nr_irqs, arg);
-	if (ret < 0) {
-		mutex_unlock(&irq_domain_mutex);
+	if (ret < 0)
 		goto out_free_irq_data;
-	}
 
 	for (i = 0; i < nr_irqs; i++) {
 		ret = irq_domain_trim_hierarchy(virq + i);
-		if (ret) {
-			mutex_unlock(&irq_domain_mutex);
+		if (ret)
 			goto out_free_irq_data;
-		}
 	}
-	
+
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_insert_irq(virq + i);
-	mutex_unlock(&irq_domain_mutex);
 
 	return virq;
 
@@ -1509,6 +1475,48 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 	irq_free_descs(virq, nr_irqs);
 	return ret;
 }
+
+/**
+ * __irq_domain_alloc_irqs - Allocate IRQs from domain
+ * @domain:	domain to allocate from
+ * @irq_base:	allocate specified IRQ number if irq_base >= 0
+ * @nr_irqs:	number of IRQs to allocate
+ * @node:	NUMA node id for memory allocation
+ * @arg:	domain specific argument
+ * @realloc:	IRQ descriptors have already been allocated if true
+ * @affinity:	Optional irq affinity mask for multiqueue devices
+ *
+ * Allocate IRQ numbers and initialized all data structures to support
+ * hierarchy IRQ domains.
+ * Parameter @realloc is mainly to support legacy IRQs.
+ * Returns error code or allocated IRQ number
+ *
+ * The whole process to setup an IRQ has been split into two steps.
+ * The first step, __irq_domain_alloc_irqs(), is to allocate IRQ
+ * descriptor and required hardware resources. The second step,
+ * irq_domain_activate_irq(), is to program the hardware with preallocated
+ * resources. In this way, it's easier to rollback when failing to
+ * allocate resources.
+ */
+int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+			    unsigned int nr_irqs, int node, void *arg,
+			    bool realloc, const struct irq_affinity_desc *affinity)
+{
+	int ret;
+
+	if (domain == NULL) {
+		domain = irq_default_domain;
+		if (WARN(!domain, "domain is NULL; cannot allocate IRQ\n"))
+			return -EINVAL;
+	}
+
+	mutex_lock(&irq_domain_mutex);
+	ret = ___irq_domain_alloc_irqs(domain, irq_base, nr_irqs, node, arg,
+				       realloc, affinity);
+	mutex_unlock(&irq_domain_mutex);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(__irq_domain_alloc_irqs);
 
 /* The irq_data was moved, fix the revmap to refer to the new location */
-- 
2.38.2

[PATCH v4 08/19] irqdomain: Refactor __irq_domain_alloc_irqs()

[Johan Hovold]

Refactor __irq_domain_alloc_irqs() so that it can be called internally
while holding the irq_domain_mutex.

This will be used to fix a shared-interrupt mapping race.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 88 +++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 40 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 894bc6ee6348..d6139b0218d4 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1430,40 +1430,12 @@ int irq_domain_alloc_irqs_hierarchy(struct irq_domain *domain,
 	return domain->ops->alloc(domain, irq_base, nr_irqs, arg);
 }
 
-/**
- * __irq_domain_alloc_irqs - Allocate IRQs from domain
- * @domain:	domain to allocate from
- * @irq_base:	allocate specified IRQ number if irq_base >= 0
- * @nr_irqs:	number of IRQs to allocate
- * @node:	NUMA node id for memory allocation
- * @arg:	domain specific argument
- * @realloc:	IRQ descriptors have already been allocated if true
- * @affinity:	Optional irq affinity mask for multiqueue devices
- *
- * Allocate IRQ numbers and initialized all data structures to support
- * hierarchy IRQ domains.
- * Parameter @realloc is mainly to support legacy IRQs.
- * Returns error code or allocated IRQ number
- *
- * The whole process to setup an IRQ has been split into two steps.
- * The first step, __irq_domain_alloc_irqs(), is to allocate IRQ
- * descriptor and required hardware resources. The second step,
- * irq_domain_activate_irq(), is to program the hardware with preallocated
- * resources. In this way, it's easier to rollback when failing to
- * allocate resources.
- */
-int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
-			    unsigned int nr_irqs, int node, void *arg,
-			    bool realloc, const struct irq_affinity_desc *affinity)
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				    unsigned int nr_irqs, int node, void *arg,
+				    bool realloc, const struct irq_affinity_desc *affinity)
 {
 	int i, ret, virq;
 
-	if (domain == NULL) {
-		domain = irq_default_domain;
-		if (WARN(!domain, "domain is NULL; cannot allocate IRQ\n"))
-			return -EINVAL;
-	}
-
 	if (realloc && irq_base >= 0) {
 		virq = irq_base;
 	} else {
@@ -1482,24 +1454,18 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 		goto out_free_desc;
 	}
 
-	mutex_lock(&irq_domain_mutex);
 	ret = irq_domain_alloc_irqs_hierarchy(domain, virq, nr_irqs, arg);
-	if (ret < 0) {
-		mutex_unlock(&irq_domain_mutex);
+	if (ret < 0)
 		goto out_free_irq_data;
-	}
 
 	for (i = 0; i < nr_irqs; i++) {
 		ret = irq_domain_trim_hierarchy(virq + i);
-		if (ret) {
-			mutex_unlock(&irq_domain_mutex);
+		if (ret)
 			goto out_free_irq_data;
-		}
 	}
-	
+
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_insert_irq(virq + i);
-	mutex_unlock(&irq_domain_mutex);
 
 	return virq;
 
@@ -1509,6 +1475,48 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 	irq_free_descs(virq, nr_irqs);
 	return ret;
 }
+
+/**
+ * __irq_domain_alloc_irqs - Allocate IRQs from domain
+ * @domain:	domain to allocate from
+ * @irq_base:	allocate specified IRQ number if irq_base >= 0
+ * @nr_irqs:	number of IRQs to allocate
+ * @node:	NUMA node id for memory allocation
+ * @arg:	domain specific argument
+ * @realloc:	IRQ descriptors have already been allocated if true
+ * @affinity:	Optional irq affinity mask for multiqueue devices
+ *
+ * Allocate IRQ numbers and initialized all data structures to support
+ * hierarchy IRQ domains.
+ * Parameter @realloc is mainly to support legacy IRQs.
+ * Returns error code or allocated IRQ number
+ *
+ * The whole process to setup an IRQ has been split into two steps.
+ * The first step, __irq_domain_alloc_irqs(), is to allocate IRQ
+ * descriptor and required hardware resources. The second step,
+ * irq_domain_activate_irq(), is to program the hardware with preallocated
+ * resources. In this way, it's easier to rollback when failing to
+ * allocate resources.
+ */
+int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+			    unsigned int nr_irqs, int node, void *arg,
+			    bool realloc, const struct irq_affinity_desc *affinity)
+{
+	int ret;
+
+	if (domain == NULL) {
+		domain = irq_default_domain;
+		if (WARN(!domain, "domain is NULL; cannot allocate IRQ\n"))
+			return -EINVAL;
+	}
+
+	mutex_lock(&irq_domain_mutex);
+	ret = ___irq_domain_alloc_irqs(domain, irq_base, nr_irqs, node, arg,
+				       realloc, affinity);
+	mutex_unlock(&irq_domain_mutex);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(__irq_domain_alloc_irqs);
 
 /* The irq_data was moved, fix the revmap to refer to the new location */
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/19] irqdomain: Refactor __irq_domain_alloc_irqs()

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

> Refactor __irq_domain_alloc_irqs() so that it can be called internally
> while holding the irq_domain_mutex.
>
> This will be used to fix a shared-interrupt mapping race.

No functional change. The split out internal function will be used to
fix a shared interrupt mapping race. This change is therefore tagged
with the same fixes tag.

Fixes: ....

> -int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> -			    unsigned int nr_irqs, int node, void *arg,
> -			    bool realloc, const struct irq_affinity_desc *affinity)
> +static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> +				    unsigned int nr_irqs, int node, void *arg,
> +				    bool realloc, const struct irq_affinity_desc *affinity)

__ vs. ___ is almost undistinguishable.

irq_domain_alloc_irqs_locked() nicely explains what this is about, no?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/19] irqdomain: Refactor __irq_domain_alloc_irqs()

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

> Refactor __irq_domain_alloc_irqs() so that it can be called internally
> while holding the irq_domain_mutex.
>
> This will be used to fix a shared-interrupt mapping race.

No functional change. The split out internal function will be used to
fix a shared interrupt mapping race. This change is therefore tagged
with the same fixes tag.

Fixes: ....

> -int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> -			    unsigned int nr_irqs, int node, void *arg,
> -			    bool realloc, const struct irq_affinity_desc *affinity)
> +static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> +				    unsigned int nr_irqs, int node, void *arg,
> +				    bool realloc, const struct irq_affinity_desc *affinity)

__ vs. ___ is almost undistinguishable.

irq_domain_alloc_irqs_locked() nicely explains what this is about, no?

Thanks,

        tglx

Re: [PATCH v4 08/19] irqdomain: Refactor __irq_domain_alloc_irqs()

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:34:40PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

> > -int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> > -			    unsigned int nr_irqs, int node, void *arg,
> > -			    bool realloc, const struct irq_affinity_desc *affinity)
> > +static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> > +				    unsigned int nr_irqs, int node, void *arg,
> > +				    bool realloc, const struct irq_affinity_desc *affinity)
> 
> __ vs. ___ is almost undistinguishable.
> 
> irq_domain_alloc_irqs_locked() nicely explains what this is about, no?

Yeah, wasn't too happy about those three underscores either, but with
the exported function unfortunately already having a double underscore
prefix... I'll try switching to a 'locked' suffix instead.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/19] irqdomain: Refactor __irq_domain_alloc_irqs()

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:34:40PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

> > -int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> > -			    unsigned int nr_irqs, int node, void *arg,
> > -			    bool realloc, const struct irq_affinity_desc *affinity)
> > +static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
> > +				    unsigned int nr_irqs, int node, void *arg,
> > +				    bool realloc, const struct irq_affinity_desc *affinity)
> 
> __ vs. ___ is almost undistinguishable.
> 
> irq_domain_alloc_irqs_locked() nicely explains what this is about, no?

Yeah, wasn't too happy about those three underscores either, but with
the exported function unfortunately already having a double underscore
prefix... I'll try switching to a 'locked' suffix instead.

Johan

[PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Johan Hovold]

Parallel probing (e.g. due to asynchronous probing) of devices that share
interrupts can currently result in two mappings for the same hardware
interrupt to be created.

Make sure to hold the irq_domain_mutex when creating mappings so that
looking for an existing mapping before creating a new one is done
atomically.

Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for drivers")
Fixes: b62b2cf5759b ("irqdomain: Fix handling of type settings for existing mappings")
Link: https://lore.kernel.org/r/YuJXMHoT4ijUxnRb@hovoldconsulting.com
Cc: Dmitry Torokhov <dtor@chromium.org>
Cc: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 47 ++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index d6139b0218d4..7232947eee3e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -25,6 +25,9 @@ static DEFINE_MUTEX(irq_domain_mutex);
 
 static struct irq_domain *irq_default_domain;
 
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				    unsigned int nr_irqs, int node, void *arg,
+				    bool realloc, const struct irq_affinity_desc *affinity);
 static void irq_domain_check_hierarchy(struct irq_domain *domain);
 
 struct irqchip_fwid {
@@ -692,7 +695,7 @@ static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
 		return 0;
 	}
 
-	if (irq_domain_associate(domain, virq, hwirq)) {
+	if (__irq_domain_associate(domain, virq, hwirq)) {
 		irq_free_desc(virq);
 		return 0;
 	}
@@ -728,14 +731,20 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 		return 0;
 	}
 
+	mutex_lock(&irq_domain_mutex);
+
 	/* Check if mapping already exists */
 	virq = irq_find_mapping(domain, hwirq);
 	if (virq) {
 		pr_debug("existing mapping on virq %d\n", virq);
-		return virq;
+		goto out;
 	}
 
-	return __irq_create_mapping_affinity(domain, hwirq, affinity);
+	virq = __irq_create_mapping_affinity(domain, hwirq, affinity);
+out:
+	mutex_unlock(&irq_domain_mutex);
+
+	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_mapping_affinity);
 
@@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
 		type &= IRQ_TYPE_SENSE_MASK;
 
+	mutex_lock(&irq_domain_mutex);
+
 	/*
 	 * If we've already configured this interrupt,
 	 * don't do it again, or hell will break loose.
@@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		 * interrupt number.
 		 */
 		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
-			return virq;
+			goto out;
 
 		/*
 		 * If the trigger type has not been set yet, then set
@@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
 			irq_data = irq_get_irq_data(virq);
 			if (!irq_data)
-				return 0;
+				goto err;
 
 			irqd_set_trigger_type(irq_data, type);
-			return virq;
+			goto out;
 		}
 
 		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
 			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
-		return 0;
+		goto err;
 	}
 
 	if (irq_domain_is_hierarchy(domain)) {
-		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
+		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
+						fwspec, false, NULL);
 		if (virq <= 0)
-			return 0;
+			goto err;
 	} else {
 		/* Create mapping */
 		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
 		if (!virq)
-			return virq;
+			goto err;
 	}
 
 	irq_data = irq_get_irq_data(virq);
 	if (WARN_ON(!irq_data))
-		return 0;
+		goto err;
 
 	/* Store trigger type */
 	irqd_set_trigger_type(irq_data, type);
+out:
+	mutex_unlock(&irq_domain_mutex);
 
 	return virq;
+err:
+	mutex_unlock(&irq_domain_mutex);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
 
@@ -1877,6 +1895,13 @@ void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
 	irq_set_handler_data(virq, handler_data);
 }
 
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				    unsigned int nr_irqs, int node, void *arg,
+				    bool realloc, const struct irq_affinity_desc *affinity)
+{
+	return -EINVAL;
+}
+
 static void irq_domain_check_hierarchy(struct irq_domain *domain)
 {
 }
-- 
2.38.2

[PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Johan Hovold]

Parallel probing (e.g. due to asynchronous probing) of devices that share
interrupts can currently result in two mappings for the same hardware
interrupt to be created.

Make sure to hold the irq_domain_mutex when creating mappings so that
looking for an existing mapping before creating a new one is done
atomically.

Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for drivers")
Fixes: b62b2cf5759b ("irqdomain: Fix handling of type settings for existing mappings")
Link: https://lore.kernel.org/r/YuJXMHoT4ijUxnRb@hovoldconsulting.com
Cc: Dmitry Torokhov <dtor@chromium.org>
Cc: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 47 ++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index d6139b0218d4..7232947eee3e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -25,6 +25,9 @@ static DEFINE_MUTEX(irq_domain_mutex);
 
 static struct irq_domain *irq_default_domain;
 
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				    unsigned int nr_irqs, int node, void *arg,
+				    bool realloc, const struct irq_affinity_desc *affinity);
 static void irq_domain_check_hierarchy(struct irq_domain *domain);
 
 struct irqchip_fwid {
@@ -692,7 +695,7 @@ static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain,
 		return 0;
 	}
 
-	if (irq_domain_associate(domain, virq, hwirq)) {
+	if (__irq_domain_associate(domain, virq, hwirq)) {
 		irq_free_desc(virq);
 		return 0;
 	}
@@ -728,14 +731,20 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 		return 0;
 	}
 
+	mutex_lock(&irq_domain_mutex);
+
 	/* Check if mapping already exists */
 	virq = irq_find_mapping(domain, hwirq);
 	if (virq) {
 		pr_debug("existing mapping on virq %d\n", virq);
-		return virq;
+		goto out;
 	}
 
-	return __irq_create_mapping_affinity(domain, hwirq, affinity);
+	virq = __irq_create_mapping_affinity(domain, hwirq, affinity);
+out:
+	mutex_unlock(&irq_domain_mutex);
+
+	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_mapping_affinity);
 
@@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
 		type &= IRQ_TYPE_SENSE_MASK;
 
+	mutex_lock(&irq_domain_mutex);
+
 	/*
 	 * If we've already configured this interrupt,
 	 * don't do it again, or hell will break loose.
@@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		 * interrupt number.
 		 */
 		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
-			return virq;
+			goto out;
 
 		/*
 		 * If the trigger type has not been set yet, then set
@@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
 			irq_data = irq_get_irq_data(virq);
 			if (!irq_data)
-				return 0;
+				goto err;
 
 			irqd_set_trigger_type(irq_data, type);
-			return virq;
+			goto out;
 		}
 
 		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
 			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
-		return 0;
+		goto err;
 	}
 
 	if (irq_domain_is_hierarchy(domain)) {
-		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
+		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
+						fwspec, false, NULL);
 		if (virq <= 0)
-			return 0;
+			goto err;
 	} else {
 		/* Create mapping */
 		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
 		if (!virq)
-			return virq;
+			goto err;
 	}
 
 	irq_data = irq_get_irq_data(virq);
 	if (WARN_ON(!irq_data))
-		return 0;
+		goto err;
 
 	/* Store trigger type */
 	irqd_set_trigger_type(irq_data, type);
+out:
+	mutex_unlock(&irq_domain_mutex);
 
 	return virq;
+err:
+	mutex_unlock(&irq_domain_mutex);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
 
@@ -1877,6 +1895,13 @@ void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
 	irq_set_handler_data(virq, handler_data);
 }
 
+static int ___irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
+				    unsigned int nr_irqs, int node, void *arg,
+				    bool realloc, const struct irq_affinity_desc *affinity)
+{
+	return -EINVAL;
+}
+
 static void irq_domain_check_hierarchy(struct irq_domain *domain)
 {
 }
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

> Parallel probing (e.g. due to asynchronous probing) of devices that share
> interrupts can currently result in two mappings for the same hardware
> interrupt to be created.

This lacks an explanation why this can happen.

> @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
>  		type &= IRQ_TYPE_SENSE_MASK;
>  
> +	mutex_lock(&irq_domain_mutex);
> +
>  	/*
>  	 * If we've already configured this interrupt,
>  	 * don't do it again, or hell will break loose.
> @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		 * interrupt number.
>  		 */
>  		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> -			return virq;
> +			goto out;
>  
>  		/*
>  		 * If the trigger type has not been set yet, then set
> @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>  			irq_data = irq_get_irq_data(virq);
>  			if (!irq_data)
> -				return 0;
> +				goto err;
>  
>  			irqd_set_trigger_type(irq_data, type);
> -			return virq;
> +			goto out;
>  		}
>  
>  		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
>  			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> -		return 0;
> +		goto err;
>  	}
>  
>  	if (irq_domain_is_hierarchy(domain)) {
> -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> +		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> +						fwspec, false, NULL);
>  		if (virq <= 0)
> -			return 0;
> +			goto err;
>  	} else {
>  		/* Create mapping */
>  		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
>  		if (!virq)
> -			return virq;
> +			goto err;
>  	}
>  
>  	irq_data = irq_get_irq_data(virq);
>  	if (WARN_ON(!irq_data))
> -		return 0;
> +		goto err;
>  
>  	/* Store trigger type */
>  	irqd_set_trigger_type(irq_data, type);
> +out:
> +	mutex_unlock(&irq_domain_mutex);
>  
>  	return virq;
> +err:
> +	mutex_unlock(&irq_domain_mutex);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);

You can spare that goto churn by renaming the existing function to
irq_create_fwspec_mapping_locked() and invoked that guarded by the
mutex, no?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:

> Parallel probing (e.g. due to asynchronous probing) of devices that share
> interrupts can currently result in two mappings for the same hardware
> interrupt to be created.

This lacks an explanation why this can happen.

> @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
>  		type &= IRQ_TYPE_SENSE_MASK;
>  
> +	mutex_lock(&irq_domain_mutex);
> +
>  	/*
>  	 * If we've already configured this interrupt,
>  	 * don't do it again, or hell will break loose.
> @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		 * interrupt number.
>  		 */
>  		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> -			return virq;
> +			goto out;
>  
>  		/*
>  		 * If the trigger type has not been set yet, then set
> @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>  			irq_data = irq_get_irq_data(virq);
>  			if (!irq_data)
> -				return 0;
> +				goto err;
>  
>  			irqd_set_trigger_type(irq_data, type);
> -			return virq;
> +			goto out;
>  		}
>  
>  		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
>  			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> -		return 0;
> +		goto err;
>  	}
>  
>  	if (irq_domain_is_hierarchy(domain)) {
> -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> +		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> +						fwspec, false, NULL);
>  		if (virq <= 0)
> -			return 0;
> +			goto err;
>  	} else {
>  		/* Create mapping */
>  		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
>  		if (!virq)
> -			return virq;
> +			goto err;
>  	}
>  
>  	irq_data = irq_get_irq_data(virq);
>  	if (WARN_ON(!irq_data))
> -		return 0;
> +		goto err;
>  
>  	/* Store trigger type */
>  	irqd_set_trigger_type(irq_data, type);
> +out:
> +	mutex_unlock(&irq_domain_mutex);
>  
>  	return virq;
> +err:
> +	mutex_unlock(&irq_domain_mutex);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);

You can spare that goto churn by renaming the existing function to
irq_create_fwspec_mapping_locked() and invoked that guarded by the
mutex, no?

Thanks,

        tglx

Re: [PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:39:51PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> 
> > Parallel probing (e.g. due to asynchronous probing) of devices that share
> > interrupts can currently result in two mappings for the same hardware
> > interrupt to be created.
> 
> This lacks an explanation why this can happen.

The explanation is there (parallel probing), but I can amend it with the
concrete example from the input subsystem if that's what you're after?

Or do you mean that the above doesn't say that the current locking is
incomplete? I believe that's covered by the next paragraph:

    Make sure to hold the irq_domain_mutex when creating mappings so that
    looking for an existing mapping before creating a new one is done
    atomically.
 
> > @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> >  	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> >  		type &= IRQ_TYPE_SENSE_MASK;
> >  
> > +	mutex_lock(&irq_domain_mutex);
> > +
> >  	/*
> >  	 * If we've already configured this interrupt,
> >  	 * don't do it again, or hell will break loose.
> > @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> >  		 * interrupt number.
> >  		 */
> >  		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> > -			return virq;
> > +			goto out;
> >  
> >  		/*
> >  		 * If the trigger type has not been set yet, then set
> > @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> >  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> >  			irq_data = irq_get_irq_data(virq);
> >  			if (!irq_data)
> > -				return 0;
> > +				goto err;
> >  
> >  			irqd_set_trigger_type(irq_data, type);
> > -			return virq;
> > +			goto out;
> >  		}
> >  
> >  		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> >  			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> > -		return 0;
> > +		goto err;
> >  	}
> >  
> >  	if (irq_domain_is_hierarchy(domain)) {
> > -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> > +		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> > +						fwspec, false, NULL);
> >  		if (virq <= 0)
> > -			return 0;
> > +			goto err;
> >  	} else {
> >  		/* Create mapping */
> >  		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> >  		if (!virq)
> > -			return virq;
> > +			goto err;
> >  	}
> >  
> >  	irq_data = irq_get_irq_data(virq);
> >  	if (WARN_ON(!irq_data))
> > -		return 0;
> > +		goto err;
> >  
> >  	/* Store trigger type */
> >  	irqd_set_trigger_type(irq_data, type);
> > +out:
> > +	mutex_unlock(&irq_domain_mutex);
> >  
> >  	return virq;
> > +err:
> > +	mutex_unlock(&irq_domain_mutex);
> > +
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> 
> You can spare that goto churn by renaming the existing function to
> irq_create_fwspec_mapping_locked() and invoked that guarded by the
> mutex, no?

That may be possible, but I'm not sure it's better. It wasn't really
obvious which of the above returns where error paths and which were
success paths before this change.

I'll try and see what it would look like.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:39:51PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> 
> > Parallel probing (e.g. due to asynchronous probing) of devices that share
> > interrupts can currently result in two mappings for the same hardware
> > interrupt to be created.
> 
> This lacks an explanation why this can happen.

The explanation is there (parallel probing), but I can amend it with the
concrete example from the input subsystem if that's what you're after?

Or do you mean that the above doesn't say that the current locking is
incomplete? I believe that's covered by the next paragraph:

    Make sure to hold the irq_domain_mutex when creating mappings so that
    looking for an existing mapping before creating a new one is done
    atomically.
 
> > @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> >  	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> >  		type &= IRQ_TYPE_SENSE_MASK;
> >  
> > +	mutex_lock(&irq_domain_mutex);
> > +
> >  	/*
> >  	 * If we've already configured this interrupt,
> >  	 * don't do it again, or hell will break loose.
> > @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> >  		 * interrupt number.
> >  		 */
> >  		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> > -			return virq;
> > +			goto out;
> >  
> >  		/*
> >  		 * If the trigger type has not been set yet, then set
> > @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> >  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> >  			irq_data = irq_get_irq_data(virq);
> >  			if (!irq_data)
> > -				return 0;
> > +				goto err;
> >  
> >  			irqd_set_trigger_type(irq_data, type);
> > -			return virq;
> > +			goto out;
> >  		}
> >  
> >  		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> >  			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> > -		return 0;
> > +		goto err;
> >  	}
> >  
> >  	if (irq_domain_is_hierarchy(domain)) {
> > -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> > +		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> > +						fwspec, false, NULL);
> >  		if (virq <= 0)
> > -			return 0;
> > +			goto err;
> >  	} else {
> >  		/* Create mapping */
> >  		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> >  		if (!virq)
> > -			return virq;
> > +			goto err;
> >  	}
> >  
> >  	irq_data = irq_get_irq_data(virq);
> >  	if (WARN_ON(!irq_data))
> > -		return 0;
> > +		goto err;
> >  
> >  	/* Store trigger type */
> >  	irqd_set_trigger_type(irq_data, type);
> > +out:
> > +	mutex_unlock(&irq_domain_mutex);
> >  
> >  	return virq;
> > +err:
> > +	mutex_unlock(&irq_domain_mutex);
> > +
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> 
> You can spare that goto churn by renaming the existing function to
> irq_create_fwspec_mapping_locked() and invoked that guarded by the
> mutex, no?

That may be possible, but I'm not sure it's better. It wasn't really
obvious which of the above returns where error paths and which were
success paths before this change.

I'll try and see what it would look like.

Johan

Re: [PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Johan Hovold]

On Wed, Jan 18, 2023 at 10:40:57AM +0100, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:39:51PM +0100, Thomas Gleixner wrote:
> > On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
 
> > > @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > >  	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> > >  		type &= IRQ_TYPE_SENSE_MASK;
> > >  
> > > +	mutex_lock(&irq_domain_mutex);
> > > +
> > >  	/*
> > >  	 * If we've already configured this interrupt,
> > >  	 * don't do it again, or hell will break loose.
> > > @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > >  		 * interrupt number.
> > >  		 */
> > >  		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> > > -			return virq;
> > > +			goto out;
> > >  
> > >  		/*
> > >  		 * If the trigger type has not been set yet, then set
> > > @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > >  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> > >  			irq_data = irq_get_irq_data(virq);
> > >  			if (!irq_data)
> > > -				return 0;
> > > +				goto err;
> > >  
> > >  			irqd_set_trigger_type(irq_data, type);
> > > -			return virq;
> > > +			goto out;
> > >  		}
> > >  
> > >  		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> > >  			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> > > -		return 0;
> > > +		goto err;
> > >  	}
> > >  
> > >  	if (irq_domain_is_hierarchy(domain)) {
> > > -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> > > +		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> > > +						fwspec, false, NULL);
> > >  		if (virq <= 0)
> > > -			return 0;
> > > +			goto err;
> > >  	} else {
> > >  		/* Create mapping */
> > >  		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> > >  		if (!virq)
> > > -			return virq;
> > > +			goto err;
> > >  	}
> > >  
> > >  	irq_data = irq_get_irq_data(virq);
> > >  	if (WARN_ON(!irq_data))
> > > -		return 0;
> > > +		goto err;
> > >  
> > >  	/* Store trigger type */
> > >  	irqd_set_trigger_type(irq_data, type);
> > > +out:
> > > +	mutex_unlock(&irq_domain_mutex);
> > >  
> > >  	return virq;
> > > +err:
> > > +	mutex_unlock(&irq_domain_mutex);
> > > +
> > > +	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> > 
> > You can spare that goto churn by renaming the existing function to
> > irq_create_fwspec_mapping_locked() and invoked that guarded by the
> > mutex, no?
> 
> That may be possible, but I'm not sure it's better. It wasn't really
> obvious which of the above returns where error paths and which were
> success paths before this change.
> 
> I'll try and see what it would look like.

I gave it a try but the resulting diff is even bigger and also obscures
the reason for why the locking is there (i.e. that the lookup and and
creation needs to be done atomically).

Adding unlocked domain lookup helpers as a preparatory patch could
possible help somewhat with the diff size, but that doesn't really make
sense with the per-domain locking added by the final patch.

(I've made some experiments to quantify the parallel probe speed up that
using per-domain locks can result in so I'm including that for v5.)

Johan

Re: [PATCH v4 09/19] irqdomain: Fix mapping-creation race

[Johan Hovold]

On Wed, Jan 18, 2023 at 10:40:57AM +0100, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:39:51PM +0100, Thomas Gleixner wrote:
> > On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
 
> > > @@ -802,6 +811,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > >  	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> > >  		type &= IRQ_TYPE_SENSE_MASK;
> > >  
> > > +	mutex_lock(&irq_domain_mutex);
> > > +
> > >  	/*
> > >  	 * If we've already configured this interrupt,
> > >  	 * don't do it again, or hell will break loose.
> > > @@ -814,7 +825,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > >  		 * interrupt number.
> > >  		 */
> > >  		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
> > > -			return virq;
> > > +			goto out;
> > >  
> > >  		/*
> > >  		 * If the trigger type has not been set yet, then set
> > > @@ -823,36 +834,43 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> > >  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> > >  			irq_data = irq_get_irq_data(virq);
> > >  			if (!irq_data)
> > > -				return 0;
> > > +				goto err;
> > >  
> > >  			irqd_set_trigger_type(irq_data, type);
> > > -			return virq;
> > > +			goto out;
> > >  		}
> > >  
> > >  		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> > >  			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> > > -		return 0;
> > > +		goto err;
> > >  	}
> > >  
> > >  	if (irq_domain_is_hierarchy(domain)) {
> > > -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> > > +		virq = ___irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> > > +						fwspec, false, NULL);
> > >  		if (virq <= 0)
> > > -			return 0;
> > > +			goto err;
> > >  	} else {
> > >  		/* Create mapping */
> > >  		virq = __irq_create_mapping_affinity(domain, hwirq, NULL);
> > >  		if (!virq)
> > > -			return virq;
> > > +			goto err;
> > >  	}
> > >  
> > >  	irq_data = irq_get_irq_data(virq);
> > >  	if (WARN_ON(!irq_data))
> > > -		return 0;
> > > +		goto err;
> > >  
> > >  	/* Store trigger type */
> > >  	irqd_set_trigger_type(irq_data, type);
> > > +out:
> > > +	mutex_unlock(&irq_domain_mutex);
> > >  
> > >  	return virq;
> > > +err:
> > > +	mutex_unlock(&irq_domain_mutex);
> > > +
> > > +	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> > 
> > You can spare that goto churn by renaming the existing function to
> > irq_create_fwspec_mapping_locked() and invoked that guarded by the
> > mutex, no?
> 
> That may be possible, but I'm not sure it's better. It wasn't really
> obvious which of the above returns where error paths and which were
> success paths before this change.
> 
> I'll try and see what it would look like.

I gave it a try but the resulting diff is even bigger and also obscures
the reason for why the locking is there (i.e. that the lookup and and
creation needs to be done atomically).

Adding unlocked domain lookup helpers as a preparatory patch could
possible help somewhat with the diff size, but that doesn't really make
sense with the per-domain locking added by the final patch.

(I've made some experiments to quantify the parallel probe speed up that
using per-domain locks can result in so I'm including that for v5.)

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 10/19] irqdomain: Clean up irq_domain_push/pop_irq()

[Johan Hovold]

The irq_domain_push_irq() interface is used to add a new (outmost) level
to a hierarchical domain after IRQs have been allocated.

Possibly due to differing mental images of hierarchical domains, the
names used for the irq_data variables make these functions much harder
to understand than what they need to be.

Rename the struct irq_data pointer to the data embedded in the
descriptor as simply 'irq_data' and refer to the data allocated by this
interface as 'parent_irq_data' so that the names reflect how
hierarchical domains are implemented.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 65 +++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 7232947eee3e..6f2b8a1248e1 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1571,8 +1571,8 @@ static void irq_domain_fix_revmap(struct irq_data *d)
  */
 int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 {
-	struct irq_data *child_irq_data;
-	struct irq_data *root_irq_data = irq_get_irq_data(virq);
+	struct irq_data *irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
 	struct irq_desc *desc;
 	int rv = 0;
 
@@ -1597,45 +1597,44 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!irq_data)
 		return -EINVAL;
 
-	if (domain->parent != root_irq_data->domain)
+	if (domain->parent != irq_data->domain)
 		return -EINVAL;
 
-	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
-				      irq_data_get_node(root_irq_data));
-	if (!child_irq_data)
+	parent_irq_data = kzalloc_node(sizeof(*parent_irq_data), GFP_KERNEL,
+				       irq_data_get_node(irq_data));
+	if (!parent_irq_data)
 		return -ENOMEM;
 
 	mutex_lock(&irq_domain_mutex);
 
 	/* Copy the original irq_data. */
-	*child_irq_data = *root_irq_data;
+	*parent_irq_data = *irq_data;
 
 	/*
-	 * Overwrite the root_irq_data, which is embedded in struct
-	 * irq_desc, with values for this domain.
+	 * Overwrite the irq_data, which is embedded in struct irq_desc, with
+	 * values for this domain.
 	 */
-	root_irq_data->parent_data = child_irq_data;
-	root_irq_data->domain = domain;
-	root_irq_data->mask = 0;
-	root_irq_data->hwirq = 0;
-	root_irq_data->chip = NULL;
-	root_irq_data->chip_data = NULL;
+	irq_data->parent_data = parent_irq_data;
+	irq_data->domain = domain;
+	irq_data->mask = 0;
+	irq_data->hwirq = 0;
+	irq_data->chip = NULL;
+	irq_data->chip_data = NULL;
 
 	/* May (probably does) set hwirq, chip, etc. */
 	rv = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
 	if (rv) {
 		/* Restore the original irq_data. */
-		*root_irq_data = *child_irq_data;
-		kfree(child_irq_data);
+		*irq_data = *parent_irq_data;
+		kfree(parent_irq_data);
 		goto error;
 	}
 
-	irq_domain_fix_revmap(child_irq_data);
-	irq_domain_set_mapping(domain, root_irq_data->hwirq, root_irq_data);
-
+	irq_domain_fix_revmap(parent_irq_data);
+	irq_domain_set_mapping(domain, irq_data->hwirq, irq_data);
 error:
 	mutex_unlock(&irq_domain_mutex);
 
@@ -1653,8 +1652,8 @@ EXPORT_SYMBOL_GPL(irq_domain_push_irq);
  */
 int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 {
-	struct irq_data *root_irq_data = irq_get_irq_data(virq);
-	struct irq_data *child_irq_data;
+	struct irq_data *irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
 	struct irq_data *tmp_irq_data;
 	struct irq_desc *desc;
 
@@ -1676,37 +1675,37 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 	if (domain == NULL)
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!irq_data)
 		return -EINVAL;
 
 	tmp_irq_data = irq_domain_get_irq_data(domain, virq);
 
 	/* We can only "pop" if this domain is at the top of the list */
-	if (WARN_ON(root_irq_data != tmp_irq_data))
+	if (WARN_ON(irq_data != tmp_irq_data))
 		return -EINVAL;
 
-	if (WARN_ON(root_irq_data->domain != domain))
+	if (WARN_ON(irq_data->domain != domain))
 		return -EINVAL;
 
-	child_irq_data = root_irq_data->parent_data;
-	if (WARN_ON(!child_irq_data))
+	parent_irq_data = irq_data->parent_data;
+	if (WARN_ON(!parent_irq_data))
 		return -EINVAL;
 
 	mutex_lock(&irq_domain_mutex);
 
-	root_irq_data->parent_data = NULL;
+	irq_data->parent_data = NULL;
 
-	irq_domain_clear_mapping(domain, root_irq_data->hwirq);
+	irq_domain_clear_mapping(domain, irq_data->hwirq);
 	irq_domain_free_irqs_hierarchy(domain, virq, 1);
 
 	/* Restore the original irq_data. */
-	*root_irq_data = *child_irq_data;
+	*irq_data = *parent_irq_data;
 
-	irq_domain_fix_revmap(root_irq_data);
+	irq_domain_fix_revmap(irq_data);
 
 	mutex_unlock(&irq_domain_mutex);
 
-	kfree(child_irq_data);
+	kfree(parent_irq_data);
 
 	return 0;
 }
-- 
2.38.2

[PATCH v4 10/19] irqdomain: Clean up irq_domain_push/pop_irq()

[Johan Hovold]

The irq_domain_push_irq() interface is used to add a new (outmost) level
to a hierarchical domain after IRQs have been allocated.

Possibly due to differing mental images of hierarchical domains, the
names used for the irq_data variables make these functions much harder
to understand than what they need to be.

Rename the struct irq_data pointer to the data embedded in the
descriptor as simply 'irq_data' and refer to the data allocated by this
interface as 'parent_irq_data' so that the names reflect how
hierarchical domains are implemented.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 kernel/irq/irqdomain.c | 65 +++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 7232947eee3e..6f2b8a1248e1 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1571,8 +1571,8 @@ static void irq_domain_fix_revmap(struct irq_data *d)
  */
 int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 {
-	struct irq_data *child_irq_data;
-	struct irq_data *root_irq_data = irq_get_irq_data(virq);
+	struct irq_data *irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
 	struct irq_desc *desc;
 	int rv = 0;
 
@@ -1597,45 +1597,44 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!irq_data)
 		return -EINVAL;
 
-	if (domain->parent != root_irq_data->domain)
+	if (domain->parent != irq_data->domain)
 		return -EINVAL;
 
-	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
-				      irq_data_get_node(root_irq_data));
-	if (!child_irq_data)
+	parent_irq_data = kzalloc_node(sizeof(*parent_irq_data), GFP_KERNEL,
+				       irq_data_get_node(irq_data));
+	if (!parent_irq_data)
 		return -ENOMEM;
 
 	mutex_lock(&irq_domain_mutex);
 
 	/* Copy the original irq_data. */
-	*child_irq_data = *root_irq_data;
+	*parent_irq_data = *irq_data;
 
 	/*
-	 * Overwrite the root_irq_data, which is embedded in struct
-	 * irq_desc, with values for this domain.
+	 * Overwrite the irq_data, which is embedded in struct irq_desc, with
+	 * values for this domain.
 	 */
-	root_irq_data->parent_data = child_irq_data;
-	root_irq_data->domain = domain;
-	root_irq_data->mask = 0;
-	root_irq_data->hwirq = 0;
-	root_irq_data->chip = NULL;
-	root_irq_data->chip_data = NULL;
+	irq_data->parent_data = parent_irq_data;
+	irq_data->domain = domain;
+	irq_data->mask = 0;
+	irq_data->hwirq = 0;
+	irq_data->chip = NULL;
+	irq_data->chip_data = NULL;
 
 	/* May (probably does) set hwirq, chip, etc. */
 	rv = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
 	if (rv) {
 		/* Restore the original irq_data. */
-		*root_irq_data = *child_irq_data;
-		kfree(child_irq_data);
+		*irq_data = *parent_irq_data;
+		kfree(parent_irq_data);
 		goto error;
 	}
 
-	irq_domain_fix_revmap(child_irq_data);
-	irq_domain_set_mapping(domain, root_irq_data->hwirq, root_irq_data);
-
+	irq_domain_fix_revmap(parent_irq_data);
+	irq_domain_set_mapping(domain, irq_data->hwirq, irq_data);
 error:
 	mutex_unlock(&irq_domain_mutex);
 
@@ -1653,8 +1652,8 @@ EXPORT_SYMBOL_GPL(irq_domain_push_irq);
  */
 int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 {
-	struct irq_data *root_irq_data = irq_get_irq_data(virq);
-	struct irq_data *child_irq_data;
+	struct irq_data *irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
 	struct irq_data *tmp_irq_data;
 	struct irq_desc *desc;
 
@@ -1676,37 +1675,37 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 	if (domain == NULL)
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!irq_data)
 		return -EINVAL;
 
 	tmp_irq_data = irq_domain_get_irq_data(domain, virq);
 
 	/* We can only "pop" if this domain is at the top of the list */
-	if (WARN_ON(root_irq_data != tmp_irq_data))
+	if (WARN_ON(irq_data != tmp_irq_data))
 		return -EINVAL;
 
-	if (WARN_ON(root_irq_data->domain != domain))
+	if (WARN_ON(irq_data->domain != domain))
 		return -EINVAL;
 
-	child_irq_data = root_irq_data->parent_data;
-	if (WARN_ON(!child_irq_data))
+	parent_irq_data = irq_data->parent_data;
+	if (WARN_ON(!parent_irq_data))
 		return -EINVAL;
 
 	mutex_lock(&irq_domain_mutex);
 
-	root_irq_data->parent_data = NULL;
+	irq_data->parent_data = NULL;
 
-	irq_domain_clear_mapping(domain, root_irq_data->hwirq);
+	irq_domain_clear_mapping(domain, irq_data->hwirq);
 	irq_domain_free_irqs_hierarchy(domain, virq, 1);
 
 	/* Restore the original irq_data. */
-	*root_irq_data = *child_irq_data;
+	*irq_data = *parent_irq_data;
 
-	irq_domain_fix_revmap(root_irq_data);
+	irq_domain_fix_revmap(irq_data);
 
 	mutex_unlock(&irq_domain_mutex);
 
-	kfree(child_irq_data);
+	kfree(parent_irq_data);
 
 	return 0;
 }
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 11/19] x86/ioapic: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/x86/kernel/apic/io_apic.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index a868b76cd3d4..9cc4c8e0c3c4 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2364,9 +2364,9 @@ static int mp_irqdomain_create(int ioapic)
 		return -ENODEV;
 	}
 
-	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
-						 (void *)(long)ioapic);
-
+	ip->irqdomain = irq_domain_create_hierarchy(parent, 0, hwirqs, fn,
+						    cfg->ops,
+						    (void *)(long)ioapic);
 	if (!ip->irqdomain) {
 		/* Release fw handle if it was allocated above */
 		if (!cfg->dev)
@@ -2374,8 +2374,6 @@ static int mp_irqdomain_create(int ioapic)
 		return -ENOMEM;
 	}
 
-	ip->irqdomain->parent = parent;
-
 	if (cfg->type == IOAPIC_DOMAIN_LEGACY ||
 	    cfg->type == IOAPIC_DOMAIN_STRICT)
 		ioapic_dynirq_base = max(ioapic_dynirq_base,
-- 
2.38.2

[PATCH v4 11/19] x86/ioapic: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/x86/kernel/apic/io_apic.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index a868b76cd3d4..9cc4c8e0c3c4 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2364,9 +2364,9 @@ static int mp_irqdomain_create(int ioapic)
 		return -ENODEV;
 	}
 
-	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
-						 (void *)(long)ioapic);
-
+	ip->irqdomain = irq_domain_create_hierarchy(parent, 0, hwirqs, fn,
+						    cfg->ops,
+						    (void *)(long)ioapic);
 	if (!ip->irqdomain) {
 		/* Release fw handle if it was allocated above */
 		if (!cfg->dev)
@@ -2374,8 +2374,6 @@ static int mp_irqdomain_create(int ioapic)
 		return -ENOMEM;
 	}
 
-	ip->irqdomain->parent = parent;
-
 	if (cfg->type == IOAPIC_DOMAIN_LEGACY ||
 	    cfg->type == IOAPIC_DOMAIN_STRICT)
 		ioapic_dynirq_base = max(ioapic_dynirq_base,
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/19] x86/ioapic: Use irq_domain_create_hierarchy()

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>  
> -	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
> -						 (void *)(long)ioapic);
> -
> +	ip->irqdomain = irq_domain_create_hierarchy(parent, 0, hwirqs, fn,
> +						    cfg->ops,
> +						    (void *)(long)ioapic);

The 80 character limit has been lifted quite some time ago. Please use
the 100 which are now the norm.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/19] x86/ioapic: Use irq_domain_create_hierarchy()

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>  
> -	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
> -						 (void *)(long)ioapic);
> -
> +	ip->irqdomain = irq_domain_create_hierarchy(parent, 0, hwirqs, fn,
> +						    cfg->ops,
> +						    (void *)(long)ioapic);

The 80 character limit has been lifted quite some time ago. Please use
the 100 which are now the norm.

Thanks,

        tglx

Re: [PATCH v4 11/19] x86/ioapic: Use irq_domain_create_hierarchy()

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:41:36PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> >  
> > -	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
> > -						 (void *)(long)ioapic);
> > -
> > +	ip->irqdomain = irq_domain_create_hierarchy(parent, 0, hwirqs, fn,
> > +						    cfg->ops,
> > +						    (void *)(long)ioapic);
> 
> The 80 character limit has been lifted quite some time ago. Please use
> the 100 which are now the norm.

Not everyone agrees, including coding-style.rst:

	The preferred limit on the length of a single line is 80 columns.

	Statements longer than 80 columns should be broken into sensible
	chunks, unless exceeding 80 columns significantly increases
	readability and does not hide information.

I go above 80 chars when it improves readability, but it's still a soft
limit for many of us.

I'll change the above, but not sure trying too hard to fit everything in
one line really improves things in cases like:

-       uv_domain = irq_domain_create_hierarchy(x86_vector_domain, 0, 0, fn,
-                                               &uv_domain_ops, NULL);
+       uv_domain = irq_domain_create_hierarchy(x86_vector_domain, 0, 0, fn, &uv_domain_ops, NULL);

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/19] x86/ioapic: Use irq_domain_create_hierarchy()

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:41:36PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> >  
> > -	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
> > -						 (void *)(long)ioapic);
> > -
> > +	ip->irqdomain = irq_domain_create_hierarchy(parent, 0, hwirqs, fn,
> > +						    cfg->ops,
> > +						    (void *)(long)ioapic);
> 
> The 80 character limit has been lifted quite some time ago. Please use
> the 100 which are now the norm.

Not everyone agrees, including coding-style.rst:

	The preferred limit on the length of a single line is 80 columns.

	Statements longer than 80 columns should be broken into sensible
	chunks, unless exceeding 80 columns significantly increases
	readability and does not hide information.

I go above 80 chars when it improves readability, but it's still a soft
limit for many of us.

I'll change the above, but not sure trying too hard to fit everything in
one line really improves things in cases like:

-       uv_domain = irq_domain_create_hierarchy(x86_vector_domain, 0, 0, fn,
-                                               &uv_domain_ops, NULL);
+       uv_domain = irq_domain_create_hierarchy(x86_vector_domain, 0, 0, fn, &uv_domain_ops, NULL);

Johan

[PATCH v4 12/19] x86/apic: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/x86/platform/uv/uv_irq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
index 1a536a187d74..ee21d6a36a80 100644
--- a/arch/x86/platform/uv/uv_irq.c
+++ b/arch/x86/platform/uv/uv_irq.c
@@ -166,10 +166,9 @@ static struct irq_domain *uv_get_irq_domain(void)
 	if (!fn)
 		goto out;
 
-	uv_domain = irq_domain_create_tree(fn, &uv_domain_ops, NULL);
-	if (uv_domain)
-		uv_domain->parent = x86_vector_domain;
-	else
+	uv_domain = irq_domain_create_hierarchy(x86_vector_domain, 0, 0, fn,
+						&uv_domain_ops, NULL);
+	if (!uv_domain)
 		irq_domain_free_fwnode(fn);
 out:
 	mutex_unlock(&uv_lock);
-- 
2.38.2

[PATCH v4 12/19] x86/apic: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/x86/platform/uv/uv_irq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
index 1a536a187d74..ee21d6a36a80 100644
--- a/arch/x86/platform/uv/uv_irq.c
+++ b/arch/x86/platform/uv/uv_irq.c
@@ -166,10 +166,9 @@ static struct irq_domain *uv_get_irq_domain(void)
 	if (!fn)
 		goto out;
 
-	uv_domain = irq_domain_create_tree(fn, &uv_domain_ops, NULL);
-	if (uv_domain)
-		uv_domain->parent = x86_vector_domain;
-	else
+	uv_domain = irq_domain_create_hierarchy(x86_vector_domain, 0, 0, fn,
+						&uv_domain_ops, NULL);
+	if (!uv_domain)
 		irq_domain_free_fwnode(fn);
 out:
 	mutex_unlock(&uv_lock);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 13/19] irqchip/alpine-msi: Use irq_domain_add_hierarchy()

[Johan Hovold]

Use the irq_domain_add_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-alpine-msi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-alpine-msi.c b/drivers/irqchip/irq-alpine-msi.c
index 5ddb8e578ac6..604459372fdd 100644
--- a/drivers/irqchip/irq-alpine-msi.c
+++ b/drivers/irqchip/irq-alpine-msi.c
@@ -204,16 +204,14 @@ static int alpine_msix_init_domains(struct alpine_msix_data *priv,
 		return -ENXIO;
 	}
 
-	middle_domain = irq_domain_add_tree(NULL,
-					    &alpine_msix_middle_domain_ops,
-					    priv);
+	middle_domain = irq_domain_add_hierarchy(gic_domain, 0, 0, NULL,
+						 &alpine_msix_middle_domain_ops,
+						 priv);
 	if (!middle_domain) {
 		pr_err("Failed to create the MSIX middle domain\n");
 		return -ENOMEM;
 	}
 
-	middle_domain->parent = gic_domain;
-
 	msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
 					       &alpine_msix_domain_info,
 					       middle_domain);
-- 
2.38.2

[PATCH v4 13/19] irqchip/alpine-msi: Use irq_domain_add_hierarchy()

[Johan Hovold]

Use the irq_domain_add_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-alpine-msi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-alpine-msi.c b/drivers/irqchip/irq-alpine-msi.c
index 5ddb8e578ac6..604459372fdd 100644
--- a/drivers/irqchip/irq-alpine-msi.c
+++ b/drivers/irqchip/irq-alpine-msi.c
@@ -204,16 +204,14 @@ static int alpine_msix_init_domains(struct alpine_msix_data *priv,
 		return -ENXIO;
 	}
 
-	middle_domain = irq_domain_add_tree(NULL,
-					    &alpine_msix_middle_domain_ops,
-					    priv);
+	middle_domain = irq_domain_add_hierarchy(gic_domain, 0, 0, NULL,
+						 &alpine_msix_middle_domain_ops,
+						 priv);
 	if (!middle_domain) {
 		pr_err("Failed to create the MSIX middle domain\n");
 		return -ENOMEM;
 	}
 
-	middle_domain->parent = gic_domain;
-
 	msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
 					       &alpine_msix_domain_info,
 					       middle_domain);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 14/19] irqchip/gic-v2m: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-gic-v2m.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index f4d7eeb13951..f1e75b35a52a 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -287,15 +287,14 @@ static __init int gicv2m_allocate_domains(struct irq_domain *parent)
 	if (!v2m)
 		return 0;
 
-	inner_domain = irq_domain_create_tree(v2m->fwnode,
-					      &gicv2m_domain_ops, v2m);
+	inner_domain = irq_domain_create_hierarchy(parent, 0, 0, v2m->fwnode,
+						   &gicv2m_domain_ops, v2m);
 	if (!inner_domain) {
 		pr_err("Failed to create GICv2m domain\n");
 		return -ENOMEM;
 	}
 
 	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
-	inner_domain->parent = parent;
 	pci_domain = pci_msi_create_irq_domain(v2m->fwnode,
 					       &gicv2m_msi_domain_info,
 					       inner_domain);
-- 
2.38.2

[PATCH v4 14/19] irqchip/gic-v2m: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-gic-v2m.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index f4d7eeb13951..f1e75b35a52a 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -287,15 +287,14 @@ static __init int gicv2m_allocate_domains(struct irq_domain *parent)
 	if (!v2m)
 		return 0;
 
-	inner_domain = irq_domain_create_tree(v2m->fwnode,
-					      &gicv2m_domain_ops, v2m);
+	inner_domain = irq_domain_create_hierarchy(parent, 0, 0, v2m->fwnode,
+						   &gicv2m_domain_ops, v2m);
 	if (!inner_domain) {
 		pr_err("Failed to create GICv2m domain\n");
 		return -ENOMEM;
 	}
 
 	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
-	inner_domain->parent = parent;
 	pci_domain = pci_msi_create_irq_domain(v2m->fwnode,
 					       &gicv2m_msi_domain_info,
 					       inner_domain);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 15/19] irqchip/gic-v3-its: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Note that the domain host_data was first set to the struct its_node
during allocation only to immediately be overwritten with the struct
msi_domain_info.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 973ede0197e3..5634d29b644d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4909,18 +4909,19 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 	if (!info)
 		return -ENOMEM;
 
-	inner_domain = irq_domain_create_tree(handle, &its_domain_ops, its);
+	info->ops = &its_msi_domain_ops;
+	info->data = its;
+
+	inner_domain = irq_domain_create_hierarchy(its_parent,
+						   its->msi_domain_flags, 0,
+						   handle, &its_domain_ops,
+						   info);
 	if (!inner_domain) {
 		kfree(info);
 		return -ENOMEM;
 	}
 
-	inner_domain->parent = its_parent;
 	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
-	inner_domain->flags |= its->msi_domain_flags;
-	info->ops = &its_msi_domain_ops;
-	info->data = its;
-	inner_domain->host_data = info;
 
 	return 0;
 }
-- 
2.38.2

[PATCH v4 15/19] irqchip/gic-v3-its: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Note that the domain host_data was first set to the struct its_node
during allocation only to immediately be overwritten with the struct
msi_domain_info.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 973ede0197e3..5634d29b644d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4909,18 +4909,19 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 	if (!info)
 		return -ENOMEM;
 
-	inner_domain = irq_domain_create_tree(handle, &its_domain_ops, its);
+	info->ops = &its_msi_domain_ops;
+	info->data = its;
+
+	inner_domain = irq_domain_create_hierarchy(its_parent,
+						   its->msi_domain_flags, 0,
+						   handle, &its_domain_ops,
+						   info);
 	if (!inner_domain) {
 		kfree(info);
 		return -ENOMEM;
 	}
 
-	inner_domain->parent = its_parent;
 	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
-	inner_domain->flags |= its->msi_domain_flags;
-	info->ops = &its_msi_domain_ops;
-	info->data = its;
-	inner_domain->host_data = info;
 
 	return 0;
 }
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 16/19] irqchip/gic-v3-mbi: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-gic-v3-mbi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-mbi.c b/drivers/irqchip/irq-gic-v3-mbi.c
index e1efdec9e9ac..dbb8b1efda44 100644
--- a/drivers/irqchip/irq-gic-v3-mbi.c
+++ b/drivers/irqchip/irq-gic-v3-mbi.c
@@ -233,13 +233,12 @@ static int mbi_allocate_domains(struct irq_domain *parent)
 	struct irq_domain *nexus_domain, *pci_domain, *plat_domain;
 	int err;
 
-	nexus_domain = irq_domain_create_tree(parent->fwnode,
-					      &mbi_domain_ops, NULL);
+	nexus_domain = irq_domain_create_hierarchy(parent, 0, 0, parent->fwnode,
+						   &mbi_domain_ops, NULL);
 	if (!nexus_domain)
 		return -ENOMEM;
 
 	irq_domain_update_bus_token(nexus_domain, DOMAIN_BUS_NEXUS);
-	nexus_domain->parent = parent;
 
 	err = mbi_allocate_pci_domain(nexus_domain, &pci_domain);
 
-- 
2.38.2

[PATCH v4 16/19] irqchip/gic-v3-mbi: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-gic-v3-mbi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-mbi.c b/drivers/irqchip/irq-gic-v3-mbi.c
index e1efdec9e9ac..dbb8b1efda44 100644
--- a/drivers/irqchip/irq-gic-v3-mbi.c
+++ b/drivers/irqchip/irq-gic-v3-mbi.c
@@ -233,13 +233,12 @@ static int mbi_allocate_domains(struct irq_domain *parent)
 	struct irq_domain *nexus_domain, *pci_domain, *plat_domain;
 	int err;
 
-	nexus_domain = irq_domain_create_tree(parent->fwnode,
-					      &mbi_domain_ops, NULL);
+	nexus_domain = irq_domain_create_hierarchy(parent, 0, 0, parent->fwnode,
+						   &mbi_domain_ops, NULL);
 	if (!nexus_domain)
 		return -ENOMEM;
 
 	irq_domain_update_bus_token(nexus_domain, DOMAIN_BUS_NEXUS);
-	nexus_domain->parent = parent;
 
 	err = mbi_allocate_pci_domain(nexus_domain, &pci_domain);
 
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 17/19] irqchip/loongson-pch-msi: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-loongson-pch-msi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-pch-msi.c b/drivers/irqchip/irq-loongson-pch-msi.c
index a72ede90ffc6..6e1e1f011bb2 100644
--- a/drivers/irqchip/irq-loongson-pch-msi.c
+++ b/drivers/irqchip/irq-loongson-pch-msi.c
@@ -163,16 +163,15 @@ static int pch_msi_init_domains(struct pch_msi_data *priv,
 {
 	struct irq_domain *middle_domain, *msi_domain;
 
-	middle_domain = irq_domain_create_linear(domain_handle,
-						priv->num_irqs,
-						&pch_msi_middle_domain_ops,
-						priv);
+	middle_domain = irq_domain_create_hierarchy(parent, 0, priv->num_irqs,
+						    domain_handle,
+						    &pch_msi_middle_domain_ops,
+						    priv);
 	if (!middle_domain) {
 		pr_err("Failed to create the MSI middle domain\n");
 		return -ENOMEM;
 	}
 
-	middle_domain->parent = parent;
 	irq_domain_update_bus_token(middle_domain, DOMAIN_BUS_NEXUS);
 
 	msi_domain = pci_msi_create_irq_domain(domain_handle,
-- 
2.38.2

[PATCH v4 17/19] irqchip/loongson-pch-msi: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-loongson-pch-msi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-pch-msi.c b/drivers/irqchip/irq-loongson-pch-msi.c
index a72ede90ffc6..6e1e1f011bb2 100644
--- a/drivers/irqchip/irq-loongson-pch-msi.c
+++ b/drivers/irqchip/irq-loongson-pch-msi.c
@@ -163,16 +163,15 @@ static int pch_msi_init_domains(struct pch_msi_data *priv,
 {
 	struct irq_domain *middle_domain, *msi_domain;
 
-	middle_domain = irq_domain_create_linear(domain_handle,
-						priv->num_irqs,
-						&pch_msi_middle_domain_ops,
-						priv);
+	middle_domain = irq_domain_create_hierarchy(parent, 0, priv->num_irqs,
+						    domain_handle,
+						    &pch_msi_middle_domain_ops,
+						    priv);
 	if (!middle_domain) {
 		pr_err("Failed to create the MSI middle domain\n");
 		return -ENOMEM;
 	}
 
-	middle_domain->parent = parent;
 	irq_domain_update_bus_token(middle_domain, DOMAIN_BUS_NEXUS);
 
 	msi_domain = pci_msi_create_irq_domain(domain_handle,
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 18/19] irqchip/mvebu-odmi: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-mvebu-odmi.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-mvebu-odmi.c b/drivers/irqchip/irq-mvebu-odmi.c
index dc4145abdd6f..108091533e10 100644
--- a/drivers/irqchip/irq-mvebu-odmi.c
+++ b/drivers/irqchip/irq-mvebu-odmi.c
@@ -161,7 +161,7 @@ static struct msi_domain_info odmi_msi_domain_info = {
 static int __init mvebu_odmi_init(struct device_node *node,
 				  struct device_node *parent)
 {
-	struct irq_domain *inner_domain, *plat_domain;
+	struct irq_domain *parent_domain, *inner_domain, *plat_domain;
 	int ret, i;
 
 	if (of_property_read_u32(node, "marvell,odmi-frames", &odmis_count))
@@ -197,16 +197,17 @@ static int __init mvebu_odmi_init(struct device_node *node,
 		}
 	}
 
-	inner_domain = irq_domain_create_linear(of_node_to_fwnode(node),
-						odmis_count * NODMIS_PER_FRAME,
-						&odmi_domain_ops, NULL);
+	parent_domain = irq_find_host(parent);
+
+	inner_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						   odmis_count * NODMIS_PER_FRAME,
+						   of_node_to_fwnode(node),
+						   &odmi_domain_ops, NULL);
 	if (!inner_domain) {
 		ret = -ENOMEM;
 		goto err_unmap;
 	}
 
-	inner_domain->parent = irq_find_host(parent);
-
 	plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(node),
 						     &odmi_msi_domain_info,
 						     inner_domain);
-- 
2.38.2

[PATCH v4 18/19] irqchip/mvebu-odmi: Use irq_domain_create_hierarchy()

[Johan Hovold]

Use the irq_domain_create_hierarchy() helper to create the hierarchical
domain, which both serves as documentation and avoids poking at
irqdomain internals.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/irqchip/irq-mvebu-odmi.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-mvebu-odmi.c b/drivers/irqchip/irq-mvebu-odmi.c
index dc4145abdd6f..108091533e10 100644
--- a/drivers/irqchip/irq-mvebu-odmi.c
+++ b/drivers/irqchip/irq-mvebu-odmi.c
@@ -161,7 +161,7 @@ static struct msi_domain_info odmi_msi_domain_info = {
 static int __init mvebu_odmi_init(struct device_node *node,
 				  struct device_node *parent)
 {
-	struct irq_domain *inner_domain, *plat_domain;
+	struct irq_domain *parent_domain, *inner_domain, *plat_domain;
 	int ret, i;
 
 	if (of_property_read_u32(node, "marvell,odmi-frames", &odmis_count))
@@ -197,16 +197,17 @@ static int __init mvebu_odmi_init(struct device_node *node,
 		}
 	}
 
-	inner_domain = irq_domain_create_linear(of_node_to_fwnode(node),
-						odmis_count * NODMIS_PER_FRAME,
-						&odmi_domain_ops, NULL);
+	parent_domain = irq_find_host(parent);
+
+	inner_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						   odmis_count * NODMIS_PER_FRAME,
+						   of_node_to_fwnode(node),
+						   &odmi_domain_ops, NULL);
 	if (!inner_domain) {
 		ret = -ENOMEM;
 		goto err_unmap;
 	}
 
-	inner_domain->parent = irq_find_host(parent);
-
 	plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(node),
 						     &odmi_msi_domain_info,
 						     inner_domain);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Johan Hovold]

The IRQ domain structures are currently protected by the global
irq_domain_mutex. Switch to using more fine-grained per-domain locking,
which may potentially speed up parallel probing somewhat.

Note that the domain lock of the root domain (innermost domain) must be
used for hierarchical domains. For non-hierarchical domain (as for root
domains), the new root pointer is set to the domain itself so that
domain->root->mutex can be used in shared code paths.

Also note that hierarchical domains should be constructed using
irq_domain_create_hierarchy() (or irq_domain_add_hierarchy()) to avoid
poking at irqdomain internals. As a safeguard, the lockdep assertion in
irq_domain_set_mapping() will catch any offenders that fail to set the
root domain pointer.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 include/linux/irqdomain.h |  4 +++
 kernel/irq/irqdomain.c    | 61 +++++++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 7fd3939328c2..b1b06d75d31a 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -125,6 +125,8 @@ struct irq_domain_chip_generic;
  *		core code.
  * @flags:	Per irq_domain flags
  * @mapcount:	The number of mapped interrupts
+ * @mutex:	Domain lock, hierarhical domains use root domain's lock
+ * @root:	Pointer to root domain, or containing structure if non-hierarchical
  *
  * Optional elements:
  * @fwnode:	Pointer to firmware node associated with the irq_domain. Pretty easy
@@ -152,6 +154,8 @@ struct irq_domain {
 	void				*host_data;
 	unsigned int			flags;
 	unsigned int			mapcount;
+	struct mutex			mutex;
+	struct irq_domain		*root;
 
 	/* Optional data */
 	struct fwnode_handle		*fwnode;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 6f2b8a1248e1..77c31b89740d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -226,6 +226,17 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 
 	domain->revmap_size = size;
 
+	/*
+	 * Hierarchical domains use the domain lock of the root domain
+	 * (innermost domain).
+	 *
+	 * For non-hierarchical domains (as for root domains), the root
+	 * pointer is set to the domain itself so that domain->root->mutex
+	 * can be used in shared code paths.
+	 */
+	mutex_init(&domain->mutex);
+	domain->root = domain;
+
 	irq_domain_check_hierarchy(domain);
 
 	mutex_lock(&irq_domain_mutex);
@@ -503,7 +514,7 @@ static bool irq_domain_is_nomap(struct irq_domain *domain)
 static void irq_domain_clear_mapping(struct irq_domain *domain,
 				     irq_hw_number_t hwirq)
 {
-	lockdep_assert_held(&irq_domain_mutex);
+	lockdep_assert_held(&domain->root->mutex);
 
 	if (irq_domain_is_nomap(domain))
 		return;
@@ -518,7 +529,11 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
 				   irq_hw_number_t hwirq,
 				   struct irq_data *irq_data)
 {
-	lockdep_assert_held(&irq_domain_mutex);
+	/*
+	 * This also makes sure that all domains point to the same root when
+	 * called from irq_domain_insert_irq() for each domain in a hierarchy.
+	 */
+	lockdep_assert_held(&domain->root->mutex);
 
 	if (irq_domain_is_nomap(domain))
 		return;
@@ -540,7 +555,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 
 	hwirq = irq_data->hwirq;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->mutex);
 
 	irq_set_status_flags(irq, IRQ_NOREQUEST);
 
@@ -562,7 +577,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 	/* Clear reverse map for this hwirq */
 	irq_domain_clear_mapping(domain, hwirq);
 
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->mutex);
 }
 
 static int __irq_domain_associate(struct irq_domain *domain, unsigned int virq,
@@ -612,9 +627,9 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 {
 	int ret;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->mutex);
 	ret = __irq_domain_associate(domain, virq, hwirq);
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->mutex);
 
 	return ret;
 }
@@ -731,7 +746,7 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 		return 0;
 	}
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->mutex);
 
 	/* Check if mapping already exists */
 	virq = irq_find_mapping(domain, hwirq);
@@ -742,7 +757,7 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 
 	virq = __irq_create_mapping_affinity(domain, hwirq, affinity);
 out:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->mutex);
 
 	return virq;
 }
@@ -811,7 +826,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
 		type &= IRQ_TYPE_SENSE_MASK;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 
 	/*
 	 * If we've already configured this interrupt,
@@ -864,11 +879,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	/* Store trigger type */
 	irqd_set_trigger_type(irq_data, type);
 out:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return virq;
 err:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return 0;
 }
@@ -1132,6 +1147,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 	else
 		domain = irq_domain_create_tree(fwnode, ops, host_data);
 	if (domain) {
+		domain->root = parent->root;
 		domain->parent = parent;
 		domain->flags |= flags;
 	}
@@ -1528,10 +1544,10 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 			return -EINVAL;
 	}
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 	ret = ___irq_domain_alloc_irqs(domain, irq_base, nr_irqs, node, arg,
 				       realloc, affinity);
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return ret;
 }
@@ -1542,7 +1558,7 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 {
 	void __rcu **slot;
 
-	lockdep_assert_held(&irq_domain_mutex);
+	lockdep_assert_held(&d->domain->root->mutex);
 
 	if (irq_domain_is_nomap(d->domain))
 		return;
@@ -1608,7 +1624,7 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (!parent_irq_data)
 		return -ENOMEM;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 
 	/* Copy the original irq_data. */
 	*parent_irq_data = *irq_data;
@@ -1636,7 +1652,7 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	irq_domain_fix_revmap(parent_irq_data);
 	irq_domain_set_mapping(domain, irq_data->hwirq, irq_data);
 error:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return rv;
 }
@@ -1691,7 +1707,7 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 	if (WARN_ON(!parent_irq_data))
 		return -EINVAL;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 
 	irq_data->parent_data = NULL;
 
@@ -1703,7 +1719,7 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 
 	irq_domain_fix_revmap(irq_data);
 
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	kfree(parent_irq_data);
 
@@ -1719,17 +1735,20 @@ EXPORT_SYMBOL_GPL(irq_domain_pop_irq);
 void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs)
 {
 	struct irq_data *data = irq_get_irq_data(virq);
+	struct irq_domain *domain;
 	int i;
 
 	if (WARN(!data || !data->domain || !data->domain->ops->free,
 		 "NULL pointer, cannot free irq\n"))
 		return;
 
-	mutex_lock(&irq_domain_mutex);
+	domain = data->domain;
+
+	mutex_lock(&domain->root->mutex);
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_remove_irq(virq + i);
-	irq_domain_free_irqs_hierarchy(data->domain, virq, nr_irqs);
-	mutex_unlock(&irq_domain_mutex);
+	irq_domain_free_irqs_hierarchy(domain, virq, nr_irqs);
+	mutex_unlock(&domain->root->mutex);
 
 	irq_domain_free_irq_data(virq, nr_irqs);
 	irq_free_descs(virq, nr_irqs);
-- 
2.38.2

[PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Johan Hovold]

The IRQ domain structures are currently protected by the global
irq_domain_mutex. Switch to using more fine-grained per-domain locking,
which may potentially speed up parallel probing somewhat.

Note that the domain lock of the root domain (innermost domain) must be
used for hierarchical domains. For non-hierarchical domain (as for root
domains), the new root pointer is set to the domain itself so that
domain->root->mutex can be used in shared code paths.

Also note that hierarchical domains should be constructed using
irq_domain_create_hierarchy() (or irq_domain_add_hierarchy()) to avoid
poking at irqdomain internals. As a safeguard, the lockdep assertion in
irq_domain_set_mapping() will catch any offenders that fail to set the
root domain pointer.

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 include/linux/irqdomain.h |  4 +++
 kernel/irq/irqdomain.c    | 61 +++++++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 7fd3939328c2..b1b06d75d31a 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -125,6 +125,8 @@ struct irq_domain_chip_generic;
  *		core code.
  * @flags:	Per irq_domain flags
  * @mapcount:	The number of mapped interrupts
+ * @mutex:	Domain lock, hierarhical domains use root domain's lock
+ * @root:	Pointer to root domain, or containing structure if non-hierarchical
  *
  * Optional elements:
  * @fwnode:	Pointer to firmware node associated with the irq_domain. Pretty easy
@@ -152,6 +154,8 @@ struct irq_domain {
 	void				*host_data;
 	unsigned int			flags;
 	unsigned int			mapcount;
+	struct mutex			mutex;
+	struct irq_domain		*root;
 
 	/* Optional data */
 	struct fwnode_handle		*fwnode;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 6f2b8a1248e1..77c31b89740d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -226,6 +226,17 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
 
 	domain->revmap_size = size;
 
+	/*
+	 * Hierarchical domains use the domain lock of the root domain
+	 * (innermost domain).
+	 *
+	 * For non-hierarchical domains (as for root domains), the root
+	 * pointer is set to the domain itself so that domain->root->mutex
+	 * can be used in shared code paths.
+	 */
+	mutex_init(&domain->mutex);
+	domain->root = domain;
+
 	irq_domain_check_hierarchy(domain);
 
 	mutex_lock(&irq_domain_mutex);
@@ -503,7 +514,7 @@ static bool irq_domain_is_nomap(struct irq_domain *domain)
 static void irq_domain_clear_mapping(struct irq_domain *domain,
 				     irq_hw_number_t hwirq)
 {
-	lockdep_assert_held(&irq_domain_mutex);
+	lockdep_assert_held(&domain->root->mutex);
 
 	if (irq_domain_is_nomap(domain))
 		return;
@@ -518,7 +529,11 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
 				   irq_hw_number_t hwirq,
 				   struct irq_data *irq_data)
 {
-	lockdep_assert_held(&irq_domain_mutex);
+	/*
+	 * This also makes sure that all domains point to the same root when
+	 * called from irq_domain_insert_irq() for each domain in a hierarchy.
+	 */
+	lockdep_assert_held(&domain->root->mutex);
 
 	if (irq_domain_is_nomap(domain))
 		return;
@@ -540,7 +555,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 
 	hwirq = irq_data->hwirq;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->mutex);
 
 	irq_set_status_flags(irq, IRQ_NOREQUEST);
 
@@ -562,7 +577,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 	/* Clear reverse map for this hwirq */
 	irq_domain_clear_mapping(domain, hwirq);
 
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->mutex);
 }
 
 static int __irq_domain_associate(struct irq_domain *domain, unsigned int virq,
@@ -612,9 +627,9 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
 {
 	int ret;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->mutex);
 	ret = __irq_domain_associate(domain, virq, hwirq);
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->mutex);
 
 	return ret;
 }
@@ -731,7 +746,7 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 		return 0;
 	}
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->mutex);
 
 	/* Check if mapping already exists */
 	virq = irq_find_mapping(domain, hwirq);
@@ -742,7 +757,7 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
 
 	virq = __irq_create_mapping_affinity(domain, hwirq, affinity);
 out:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->mutex);
 
 	return virq;
 }
@@ -811,7 +826,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
 		type &= IRQ_TYPE_SENSE_MASK;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 
 	/*
 	 * If we've already configured this interrupt,
@@ -864,11 +879,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	/* Store trigger type */
 	irqd_set_trigger_type(irq_data, type);
 out:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return virq;
 err:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return 0;
 }
@@ -1132,6 +1147,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 	else
 		domain = irq_domain_create_tree(fwnode, ops, host_data);
 	if (domain) {
+		domain->root = parent->root;
 		domain->parent = parent;
 		domain->flags |= flags;
 	}
@@ -1528,10 +1544,10 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 			return -EINVAL;
 	}
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 	ret = ___irq_domain_alloc_irqs(domain, irq_base, nr_irqs, node, arg,
 				       realloc, affinity);
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return ret;
 }
@@ -1542,7 +1558,7 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 {
 	void __rcu **slot;
 
-	lockdep_assert_held(&irq_domain_mutex);
+	lockdep_assert_held(&d->domain->root->mutex);
 
 	if (irq_domain_is_nomap(d->domain))
 		return;
@@ -1608,7 +1624,7 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (!parent_irq_data)
 		return -ENOMEM;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 
 	/* Copy the original irq_data. */
 	*parent_irq_data = *irq_data;
@@ -1636,7 +1652,7 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	irq_domain_fix_revmap(parent_irq_data);
 	irq_domain_set_mapping(domain, irq_data->hwirq, irq_data);
 error:
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	return rv;
 }
@@ -1691,7 +1707,7 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 	if (WARN_ON(!parent_irq_data))
 		return -EINVAL;
 
-	mutex_lock(&irq_domain_mutex);
+	mutex_lock(&domain->root->mutex);
 
 	irq_data->parent_data = NULL;
 
@@ -1703,7 +1719,7 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 
 	irq_domain_fix_revmap(irq_data);
 
-	mutex_unlock(&irq_domain_mutex);
+	mutex_unlock(&domain->root->mutex);
 
 	kfree(parent_irq_data);
 
@@ -1719,17 +1735,20 @@ EXPORT_SYMBOL_GPL(irq_domain_pop_irq);
 void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs)
 {
 	struct irq_data *data = irq_get_irq_data(virq);
+	struct irq_domain *domain;
 	int i;
 
 	if (WARN(!data || !data->domain || !data->domain->ops->free,
 		 "NULL pointer, cannot free irq\n"))
 		return;
 
-	mutex_lock(&irq_domain_mutex);
+	domain = data->domain;
+
+	mutex_lock(&domain->root->mutex);
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_remove_irq(virq + i);
-	irq_domain_free_irqs_hierarchy(data->domain, virq, nr_irqs);
-	mutex_unlock(&irq_domain_mutex);
+	irq_domain_free_irqs_hierarchy(domain, virq, nr_irqs);
+	mutex_unlock(&domain->root->mutex);
 
 	irq_domain_free_irq_data(virq, nr_irqs);
 	irq_free_descs(virq, nr_irqs);
-- 
2.38.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> The IRQ domain structures are currently protected by the global
> irq_domain_mutex. Switch to using more fine-grained per-domain locking,
> which may potentially speed up parallel probing somewhat.

Does it or not?

If not then why adding all this churn for no real value?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Thomas Gleixner]

On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> The IRQ domain structures are currently protected by the global
> irq_domain_mutex. Switch to using more fine-grained per-domain locking,
> which may potentially speed up parallel probing somewhat.

Does it or not?

If not then why adding all this churn for no real value?

Thanks,

        tglx

Re: [PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:50:39PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > The IRQ domain structures are currently protected by the global
> > irq_domain_mutex. Switch to using more fine-grained per-domain locking,
> > which may potentially speed up parallel probing somewhat.
> 
> Does it or not?
> 
> If not then why adding all this churn for no real value?

It probably doesn't make much difference, but Marc wanted per-domain
locking:

    > I'd really like to avoid a global mutex. At the very least this should
    > be a per-domain mutex, otherwise this will serialise a lot more than
    > what is needed.
    
    Yeah, I considered that too, but wanted to get your comments on this
    first.
    
    Also note that the likewise global irq_domain_mutex (and
    sparse_irq_lock) are taken in some of these paths so perhaps using finer
    locking won't actually matter that much as this is mostly for parallel
    probing.

    https://lore.kernel.org/lkml/YuKHiZuNvN+K9NCc@hovoldconsulting.com/

As part of fixing the races, this series has now replaced the per-domain
revmap mutexes with the global irq_domain_mutex, which could possibly be
perceived as a step in the wrong direction in this respect.

This patch restores per-domain locking for non-hierarchical domains and
extends it to hierarchical domains. This leaves the irq_domain_mutex to
only be used for things that actually need a global lock such as the
domain list.

I consider this mostly a clean up, and I did intentionally place it last
in order to not have the fixes depend on it.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Johan Hovold]

On Tue, Jan 17, 2023 at 10:50:39PM +0100, Thomas Gleixner wrote:
> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
> > The IRQ domain structures are currently protected by the global
> > irq_domain_mutex. Switch to using more fine-grained per-domain locking,
> > which may potentially speed up parallel probing somewhat.
> 
> Does it or not?
> 
> If not then why adding all this churn for no real value?

It probably doesn't make much difference, but Marc wanted per-domain
locking:

    > I'd really like to avoid a global mutex. At the very least this should
    > be a per-domain mutex, otherwise this will serialise a lot more than
    > what is needed.
    
    Yeah, I considered that too, but wanted to get your comments on this
    first.
    
    Also note that the likewise global irq_domain_mutex (and
    sparse_irq_lock) are taken in some of these paths so perhaps using finer
    locking won't actually matter that much as this is mostly for parallel
    probing.

    https://lore.kernel.org/lkml/YuKHiZuNvN+K9NCc@hovoldconsulting.com/

As part of fixing the races, this series has now replaced the per-domain
revmap mutexes with the global irq_domain_mutex, which could possibly be
perceived as a step in the wrong direction in this respect.

This patch restores per-domain locking for non-hierarchical domains and
extends it to hierarchical domains. This leaves the irq_domain_mutex to
only be used for things that actually need a global lock such as the
domain list.

I consider this mostly a clean up, and I did intentionally place it last
in order to not have the fixes depend on it.

Johan

Re: [PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Thomas Gleixner]

On Wed, Jan 18 2023 at 10:51, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:50:39PM +0100, Thomas Gleixner wrote:
>> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>> > The IRQ domain structures are currently protected by the global
>> > irq_domain_mutex. Switch to using more fine-grained per-domain locking,
>> > which may potentially speed up parallel probing somewhat.
>> 
>> Does it or not?
>> 
>> If not then why adding all this churn for no real value?
>
> It probably doesn't make much difference, but Marc wanted per-domain
> locking:
>
>     > I'd really like to avoid a global mutex. At the very least this should
>     > be a per-domain mutex, otherwise this will serialise a lot more than
>     > what is needed.

Sure it serializes more than what is needed, but the real question is
whether it matters. If it does not matter then I prefer KISS over a just
because we can optimization.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 19/19] irqdomain: Switch to per-domain locking

[Thomas Gleixner]

On Wed, Jan 18 2023 at 10:51, Johan Hovold wrote:
> On Tue, Jan 17, 2023 at 10:50:39PM +0100, Thomas Gleixner wrote:
>> On Mon, Jan 16 2023 at 14:50, Johan Hovold wrote:
>> > The IRQ domain structures are currently protected by the global
>> > irq_domain_mutex. Switch to using more fine-grained per-domain locking,
>> > which may potentially speed up parallel probing somewhat.
>> 
>> Does it or not?
>> 
>> If not then why adding all this churn for no real value?
>
> It probably doesn't make much difference, but Marc wanted per-domain
> locking:
>
>     > I'd really like to avoid a global mutex. At the very least this should
>     > be a per-domain mutex, otherwise this will serialise a lot more than
>     > what is needed.

Sure it serializes more than what is needed, but the real question is
whether it matters. If it does not matter then I prefer KISS over a just
because we can optimization.

Thanks,

        tglx