diff mbox series

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

Message ID 20230116135044.14998-9-johan+linaro@kernel.org
State New
Headers
Series irqdomain: fix mapping race and clean up locking |

Commit Message

Johan Hovold Jan. 16, 2023, 1:50 p.m. UTC 
  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(-)

Comments

Thomas Gleixner Jan. 17, 2023, 9:34 p.m. UTC | #1 
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
Johan Hovold Jan. 18, 2023, 9:30 a.m. UTC | #2 
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
diff mbox series

Patch

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 */