[PATCH 0/9] trival fix or improvement about irq_desc access

[PATCH 0/9] trival fix or improvement about irq_desc access

[Pingfan Liu]

When I went through the access of irq_to_desc(), I found the following
trival issue or improvement can be done.
[1-4/9]: clean up or small improvement
[5-6/9]: bugfix for access to irq_desc under releasable context
[7/9]: clean up
[8-9/9]: bugfix for irq debugfs


Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Baokun Li <libaokun1@huawei.com>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dongli Zhang <dongli.zhang@oracle.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Qais Yousef <qais.yousef@arm.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steven Price <steven.price@arm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Rix <trix@redhat.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Yuan ZhaoXiong <yuanzhaoxiong@baidu.com>
Cc: YueHaibing <yuehaibing@huawei.com>
To: linux-kernel@vger.kernel.org


Pingfan Liu (9):
  irq/irqdesc: put the lock at the exact place in irq_sysfs_init()
  irq/irqdesc: change the name of delete_irq_desc() to irq_delete_desc()
  irq/manage: remove some unreferenced code
  s390/irq: utilize RCU instead of irq_lock_sparse() in
    show_msi_interrupt()
  x86/irq: place for_each_active_irq() in rcu read section
  pm/irq: make for_each_irq_desc() safe of irq_desc release
  irq: remove needless lock in takedown_cpu()
  irq: make irq_lock_sparse() independent of CONFIG_SPARSE_IRQ
  irq/irqdesc: rename sparse_irq_lock to bitmap_lock

 .clang-format                  |  1 -
 arch/s390/kernel/irq.c         | 11 ++++----
 arch/x86/kernel/apic/io_apic.c |  3 +++
 include/linux/irq.h            |  1 -
 include/linux/irqdesc.h        | 10 +++-----
 include/linux/irqnr.h          |  3 ---
 kernel/cpu.c                   | 15 ++++-------
 kernel/irq/cpuhotplug.c        |  4 +--
 kernel/irq/debugfs.c           |  4 +--
 kernel/irq/irqdesc.c           | 47 +++++++++++++++++-----------------
 kernel/irq/manage.c            | 15 -----------
 kernel/irq/pm.c                |  3 +++
 12 files changed, 47 insertions(+), 70 deletions(-)

-- 
2.31.1

[PATCH 1/9] irq/irqdesc: put the lock at the exact place in irq_sysfs_init()

[Pingfan Liu]

sparse_irq_lock is used to protect irq_desc, and furthermore
irq_sysfs_init() is called only once, so put irq_lock_sparse() at the
exact place, where the race condition may start.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
To: linux-kernel@vger.kernel.org
---
 kernel/irq/irqdesc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 939d21cd55c3..8d0982233277 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -312,15 +312,14 @@ static int __init irq_sysfs_init(void)
 	struct irq_desc *desc;
 	int irq;
 
-	/* Prevent concurrent irq alloc/free */
-	irq_lock_sparse();
-
 	irq_kobj_base = kobject_create_and_add("irq", kernel_kobj);
 	if (!irq_kobj_base) {
-		irq_unlock_sparse();
 		return -ENOMEM;
 	}
 
+	/* Prevent concurrent irq alloc/free */
+	irq_lock_sparse();
+
 	/* Add the already allocated interrupts */
 	for_each_irq_desc(irq, desc)
 		irq_sysfs_add(irq, desc);
-- 
2.31.1

[PATCH 2/9] irq/irqdesc: change the name of delete_irq_desc() to irq_delete_desc()

[Pingfan Liu]

Make the naming convention consistent with irq_insert_desc(),
irq_to_desc().

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
To: linux-kernel@vger.kernel.org
---
 kernel/irq/irqdesc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 8d0982233277..9feedaa08430 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -355,7 +355,7 @@ struct irq_desc *irq_to_desc(unsigned int irq)
 EXPORT_SYMBOL_GPL(irq_to_desc);
 #endif
 
-static void delete_irq_desc(unsigned int irq)
+static void irq_delete_desc(unsigned int irq)
 {
 	radix_tree_delete(&irq_desc_tree, irq);
 }
@@ -453,7 +453,7 @@ static void free_desc(unsigned int irq)
 	 * irq_sysfs_init() as well.
 	 */
 	irq_sysfs_del(desc);
-	delete_irq_desc(irq);
+	irq_delete_desc(irq);
 
 	/*
 	 * We free the descriptor, masks and stat fields via RCU. That
-- 
2.31.1

[PATCH 3/9] irq/manage: remove some unreferenced code

[Pingfan Liu]

Neither remove_percpu_irq() nor for_each_irq_nr() has any users, so they
can be removed.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Tom Rix <trix@redhat.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: llvm@lists.linux.dev
To: linux-kernel@vger.kernel.org
---
 .clang-format         |  1 -
 include/linux/irq.h   |  1 -
 include/linux/irqnr.h |  3 ---
 kernel/irq/manage.c   | 15 ---------------
 4 files changed, 20 deletions(-)

diff --git a/.clang-format b/.clang-format
index fa959436bcfd..fb161bc8ca1a 100644
--- a/.clang-format
+++ b/.clang-format
@@ -198,7 +198,6 @@ ForEachMacros:
   - 'for_each_if'
   - 'for_each_iommu'
   - 'for_each_ip_tunnel_rcu'
-  - 'for_each_irq_nr'
   - 'for_each_link_codecs'
   - 'for_each_link_cpus'
   - 'for_each_link_platforms'
diff --git a/include/linux/irq.h b/include/linux/irq.h
index f92788ccdba2..ec50007decfc 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -603,7 +603,6 @@ enum {
 
 struct irqaction;
 extern int setup_percpu_irq(unsigned int irq, struct irqaction *new);
-extern void remove_percpu_irq(unsigned int irq, struct irqaction *act);
 
 #ifdef CONFIG_DEPRECATED_IRQ_CPU_ONOFFLINE
 extern void irq_cpu_online(void);
diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index 3496baa0b07f..082fe0856e87 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -28,7 +28,4 @@ unsigned int irq_get_next_irq(unsigned int offset);
 	for (irq = irq_get_next_irq(0); irq < nr_irqs;	\
 	     irq = irq_get_next_irq(irq + 1))
 
-#define for_each_irq_nr(irq)                   \
-       for (irq = 0; irq < nr_irqs; irq++)
-
 #endif
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c03f71d5ec10..b7c86be68b58 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -2466,21 +2466,6 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
 	return NULL;
 }
 
-/**
- *	remove_percpu_irq - free a per-cpu interrupt
- *	@irq: Interrupt line to free
- *	@act: irqaction for the interrupt
- *
- * Used to remove interrupts statically setup by the early boot process.
- */
-void remove_percpu_irq(unsigned int irq, struct irqaction *act)
-{
-	struct irq_desc *desc = irq_to_desc(irq);
-
-	if (desc && irq_settings_is_per_cpu_devid(desc))
-	    __free_percpu_irq(irq, act->percpu_dev_id);
-}
-
 /**
  *	free_percpu_irq - free an interrupt allocated with request_percpu_irq
  *	@irq: Interrupt line to free
-- 
2.31.1

[PATCH 4/9] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Pingfan Liu]

irq_desc can be accessed safely in RCU read section as demonstrated by
kstat_irqs_usr(). And raw_spin_lock_irqsave() context can provide a rcu
read section, which can be utilized to get rid of irq_lock_sparse().

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: linux-kernel@vger.kernel.org
To: linux-s390@vger.kernel.org
---
 arch/s390/kernel/irq.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 3033f616e256..6302dc7874cf 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -205,12 +205,13 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
 	unsigned long flags;
 	int cpu;
 
-	irq_lock_sparse();
+	raw_spin_lock_irqsave(&desc->lock, flags);
 	desc = irq_to_desc(irq);
-	if (!desc)
-		goto out;
+	if (!desc) {
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
+		return;
+	}
 
-	raw_spin_lock_irqsave(&desc->lock, flags);
 	seq_printf(p, "%3d: ", irq);
 	for_each_online_cpu(cpu)
 		seq_printf(p, "%10u ", irq_desc_kstat_cpu(desc, cpu));
@@ -223,8 +224,6 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
 
 	seq_putc(p, '\n');
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
-out:
-	irq_unlock_sparse();
 }
 
 /*
-- 
2.31.1

[PATCH 5/9] x86/irq: place for_each_active_irq() in rcu read section

[Pingfan Liu]

Since there are access to irq_desc, and no preemption is provided at the
involved, it requires rcu read lock to protect irq_desc.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc: Marc Zyngier <maz@kernel.org>
Cc: linux-kernel@vger.kernel.org
To: x86@kernel.org
---
 arch/x86/kernel/apic/io_apic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c1bb384935b0..4bb16edcbe4d 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1333,6 +1333,7 @@ void __init print_IO_APICs(void)
 		print_IO_APIC(ioapic_idx);
 
 	printk(KERN_DEBUG "IRQ to pin mappings:\n");
+	rcu_read_lock();
 	for_each_active_irq(irq) {
 		struct irq_pin_list *entry;
 		struct irq_chip *chip;
@@ -1352,6 +1353,7 @@ void __init print_IO_APICs(void)
 			pr_cont("-> %d:%d", entry->apic, entry->pin);
 		pr_cont("\n");
 	}
+	rcu_read_unlock();
 
 	printk(KERN_INFO ".................................... done.\n");
 }
@@ -2009,6 +2011,7 @@ static inline void init_IO_APIC_traps(void)
 	struct irq_cfg *cfg;
 	unsigned int irq;
 
+	/* The early boot stage is free of irq_desc release */
 	for_each_active_irq(irq) {
 		cfg = irq_cfg(irq);
 		if (IO_APIC_IRQ(irq) && cfg && !cfg->vector) {
-- 
2.31.1

[PATCH 6/9] pm/irq: make for_each_irq_desc() safe of irq_desc release

[Pingfan Liu]

The invloved context is no a RCU read section. Furthermore there may be
more than one task at this point. Hence it demands a measure to prevent
irq_desc from freeing. Use irq_lock_sparse to serve the protection
purpose.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
To: linux-kernel@vger.kernel.org
---
 kernel/irq/pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index ca71123a6130..4b67a4c7de3c 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -133,6 +133,7 @@ void suspend_device_irqs(void)
 	struct irq_desc *desc;
 	int irq;
 
+	irq_lock_sparse();
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 		bool sync;
@@ -146,6 +147,7 @@ void suspend_device_irqs(void)
 		if (sync)
 			synchronize_irq(irq);
 	}
+	irq_unlock_sparse();
 }
 EXPORT_SYMBOL_GPL(suspend_device_irqs);
 
@@ -186,6 +188,7 @@ static void resume_irqs(bool want_early)
 	struct irq_desc *desc;
 	int irq;
 
+	/* The early resume stage is free of irq_desc release */
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 		bool is_early = desc->action &&
-- 
2.31.1

[PATCH 7/9] irq: remove needless lock in takedown_cpu()

[Pingfan Liu]

Any allocation or release of irq_desc should fall into the interface of
__irq_alloc_descs() or irq_free_descs(). And both of them hold the mutex
sparse_irq_lock. This preemptability contradicts the preempt-disabled
context when dispatching fn in cpu_stopper_thread(). So allocation or
free of irq_desc can not be demanded in cpu_stopper_thread().

On the other hand, for the safety of access to irq_desc, rcu still keeps
watching the dying cpu until the last minute rcu_report_dead(), so each
cpu_stop_fn_t can safely access irq_desc.

As a result, in takedown_cpu() irq_lock_sparse()/irq_unlock_sparse() can
be safely removed.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Baokun Li <libaokun1@huawei.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Steven Price <steven.price@arm.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Yuan ZhaoXiong <yuanzhaoxiong@baidu.com>
Cc: YueHaibing <yuehaibing@huawei.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
To: linux-kernel@vger.kernel.org
---
 kernel/cpu.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index d0a9aa0b42e8..94a6b512c26d 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1033,18 +1033,16 @@ static int takedown_cpu(unsigned int cpu)
 	kthread_park(st->thread);
 
 	/*
-	 * Prevent irq alloc/free while the dying cpu reorganizes the
-	 * interrupt affinities.
+	 * RCU keeps watching 'cpu' until do_idle()->rcu_report_dead().
+	 * And cpu_stopper's fn is dispatched with preemption disabled.
+	 * So it can not occur to release a irq_desc.
 	 */
-	irq_lock_sparse();
 
 	/*
 	 * So now all preempt/rcu users must observe !cpu_active().
 	 */
 	err = stop_machine_cpuslocked(take_cpu_down, NULL, cpumask_of(cpu));
 	if (err) {
-		/* CPU refused to die */
-		irq_unlock_sparse();
 		/* Unpark the hotplug thread so we can rollback there */
 		kthread_unpark(st->thread);
 		return err;
@@ -1061,9 +1059,6 @@ static int takedown_cpu(unsigned int cpu)
 	wait_for_ap_thread(st, false);
 	BUG_ON(st->state != CPUHP_AP_IDLE_DEAD);
 
-	/* Interrupts are moved away from the dying cpu, reenable alloc/free */
-	irq_unlock_sparse();
-
 	hotplug_cpu__broadcast_tick_pull(cpu);
 	/* This actually kills the CPU. */
 	__cpu_die(cpu);
-- 
2.31.1

[PATCH 8/9] irq: make irq_lock_sparse() independent of CONFIG_SPARSE_IRQ

[Pingfan Liu]

Even in the case of non-sparse irq, the callsite of
for_each_active_irq() in irq_debugfs_init() still cares about the sync
of allocated_irqs bitmap. Otherwise irq_debugfs_init() may show some
disappeared irq if the irq is disactived by other driver in parallel.

As there are only a few callsites of irq_lock_sparse() in the cold path,
which means the slight performance drops can be ignored.  Hence moving
irq_lock_sparse() out of CONFIG_SPARSE_IRQ to protect both irq_desc and
allocated_irqs bitmap, instead of making irq_lock_sparse() dependent on
GENERIC_IRQ_DEBUGFS.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
To: linux-kernel@vger.kernel.org
---
 include/linux/irqdesc.h |  6 ++----
 kernel/irq/irqdesc.c    | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index a77584593f7d..6c01231fec00 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -104,12 +104,10 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-#ifdef CONFIG_SPARSE_IRQ
 extern void irq_lock_sparse(void);
 extern void irq_unlock_sparse(void);
-#else
-static inline void irq_lock_sparse(void) { }
-static inline void irq_unlock_sparse(void) { }
+
+#ifndef CONFIG_SPARSE_IRQ
 extern struct irq_desc irq_desc[NR_IRQS];
 #endif
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9feedaa08430..a5cefd7c9ef7 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -375,16 +375,6 @@ static void free_masks(struct irq_desc *desc)
 static inline void free_masks(struct irq_desc *desc) { }
 #endif
 
-void irq_lock_sparse(void)
-{
-	mutex_lock(&sparse_irq_lock);
-}
-
-void irq_unlock_sparse(void)
-{
-	mutex_unlock(&sparse_irq_lock);
-}
-
 static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
 				   const struct cpumask *affinity,
 				   struct module *owner)
@@ -721,6 +711,16 @@ int generic_handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq)
 }
 #endif
 
+void irq_lock_sparse(void)
+{
+	mutex_lock(&sparse_irq_lock);
+}
+
+void irq_unlock_sparse(void)
+{
+	mutex_unlock(&sparse_irq_lock);
+}
+
 /* Dynamic interrupt handling */
 
 /**
-- 
2.31.1

[PATCH 9/9] irq/irqdesc: rename sparse_irq_lock to bitmap_lock

[Pingfan Liu]

Now that sparse_irq_lock serves for both sparse and non-sparse purpose,
it is better to rename it as bitmap_lock. Corresponding, rename
irq_lock_sparse() as irq_lock_bitmap().

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Pingfan Liu <kernelfans@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Qais Yousef <qais.yousef@arm.com>
Cc: Yuan ZhaoXiong <yuanzhaoxiong@baidu.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Dongli Zhang <dongli.zhang@oracle.com>
Cc: YueHaibing <yuehaibing@huawei.com>
Cc: Steven Price <steven.price@arm.com>
To: linux-kernel@vger.kernel.org
---
 include/linux/irqdesc.h |  4 ++--
 kernel/cpu.c            |  4 ++--
 kernel/irq/cpuhotplug.c |  4 ++--
 kernel/irq/debugfs.c    |  4 ++--
 kernel/irq/irqdesc.c    | 26 +++++++++++++-------------
 kernel/irq/pm.c         |  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 6c01231fec00..8301653fc2b2 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -104,8 +104,8 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-extern void irq_lock_sparse(void);
-extern void irq_unlock_sparse(void);
+extern void irq_lock_bitmap(void);
+extern void irq_unlock_bitmap(void);
 
 #ifndef CONFIG_SPARSE_IRQ
 extern struct irq_desc irq_desc[NR_IRQS];
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 94a6b512c26d..8874f0242893 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -601,11 +601,11 @@ static int bringup_cpu(unsigned int cpu)
 	 * setup the vector space for the cpu which comes online.
 	 * Prevent irq alloc/free across the bringup.
 	 */
-	irq_lock_sparse();
+	irq_lock_bitmap();
 
 	/* Arch-specific enabling code. */
 	ret = __cpu_up(cpu, idle);
-	irq_unlock_sparse();
+	irq_unlock_bitmap();
 	if (ret)
 		return ret;
 	return bringup_wait_for_ap(cpu);
diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c
index 1ed2b1739363..84547b5781be 100644
--- a/kernel/irq/cpuhotplug.c
+++ b/kernel/irq/cpuhotplug.c
@@ -220,14 +220,14 @@ int irq_affinity_online_cpu(unsigned int cpu)
 	struct irq_desc *desc;
 	unsigned int irq;
 
-	irq_lock_sparse();
+	irq_lock_bitmap();
 	for_each_active_irq(irq) {
 		desc = irq_to_desc(irq);
 		raw_spin_lock_irq(&desc->lock);
 		irq_restore_affinity_of_irq(desc, cpu);
 		raw_spin_unlock_irq(&desc->lock);
 	}
-	irq_unlock_sparse();
+	irq_unlock_bitmap();
 
 	return 0;
 }
diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index 2b43f5f5033d..af4bd2ca2372 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -251,10 +251,10 @@ static int __init irq_debugfs_init(void)
 
 	irq_dir = debugfs_create_dir("irqs", root_dir);
 
-	irq_lock_sparse();
+	irq_lock_bitmap();
 	for_each_active_irq(irq)
 		irq_add_debugfs_entry(irq, irq_to_desc(irq));
-	irq_unlock_sparse();
+	irq_unlock_bitmap();
 
 	return 0;
 }
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index a5cefd7c9ef7..acd80cb8a511 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -130,7 +130,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
 int nr_irqs = NR_IRQS;
 EXPORT_SYMBOL_GPL(nr_irqs);
 
-static DEFINE_MUTEX(sparse_irq_lock);
+static DEFINE_MUTEX(bitmap_lock);
 static DECLARE_BITMAP(allocated_irqs, IRQ_BITMAP_BITS);
 
 #ifdef CONFIG_SPARSE_IRQ
@@ -318,12 +318,12 @@ static int __init irq_sysfs_init(void)
 	}
 
 	/* Prevent concurrent irq alloc/free */
-	irq_lock_sparse();
+	irq_lock_bitmap();
 
 	/* Add the already allocated interrupts */
 	for_each_irq_desc(irq, desc)
 		irq_sysfs_add(irq, desc);
-	irq_unlock_sparse();
+	irq_unlock_bitmap();
 
 	return 0;
 }
@@ -607,9 +607,9 @@ static int irq_expand_nr_irqs(unsigned int nr)
 
 void irq_mark_irq(unsigned int irq)
 {
-	mutex_lock(&sparse_irq_lock);
+	mutex_lock(&bitmap_lock);
 	bitmap_set(allocated_irqs, irq, 1);
-	mutex_unlock(&sparse_irq_lock);
+	mutex_unlock(&bitmap_lock);
 }
 
 #ifdef CONFIG_GENERIC_IRQ_LEGACY
@@ -711,14 +711,14 @@ int generic_handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq)
 }
 #endif
 
-void irq_lock_sparse(void)
+void irq_lock_bitmap(void)
 {
-	mutex_lock(&sparse_irq_lock);
+	mutex_lock(&bitmap_lock);
 }
 
-void irq_unlock_sparse(void)
+void irq_unlock_bitmap(void)
 {
-	mutex_unlock(&sparse_irq_lock);
+	mutex_unlock(&bitmap_lock);
 }
 
 /* Dynamic interrupt handling */
@@ -735,12 +735,12 @@ void irq_free_descs(unsigned int from, unsigned int cnt)
 	if (from >= nr_irqs || (from + cnt) > nr_irqs)
 		return;
 
-	mutex_lock(&sparse_irq_lock);
+	mutex_lock(&bitmap_lock);
 	for (i = 0; i < cnt; i++)
 		free_desc(from + i);
 
 	bitmap_clear(allocated_irqs, from, cnt);
-	mutex_unlock(&sparse_irq_lock);
+	mutex_unlock(&bitmap_lock);
 }
 EXPORT_SYMBOL_GPL(irq_free_descs);
 
@@ -779,7 +779,7 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 		from = arch_dynirq_lower_bound(from);
 	}
 
-	mutex_lock(&sparse_irq_lock);
+	mutex_lock(&bitmap_lock);
 
 	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
 					   from, cnt, 0);
@@ -794,7 +794,7 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 	}
 	ret = alloc_descs(start, cnt, node, affinity, owner);
 unlock:
-	mutex_unlock(&sparse_irq_lock);
+	mutex_unlock(&bitmap_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(__irq_alloc_descs);
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 4b67a4c7de3c..9b81a738d84f 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -133,7 +133,7 @@ void suspend_device_irqs(void)
 	struct irq_desc *desc;
 	int irq;
 
-	irq_lock_sparse();
+	irq_lock_bitmap();
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 		bool sync;
@@ -147,7 +147,7 @@ void suspend_device_irqs(void)
 		if (sync)
 			synchronize_irq(irq);
 	}
-	irq_unlock_sparse();
+	irq_unlock_bitmap();
 }
 EXPORT_SYMBOL_GPL(suspend_device_irqs);
 
-- 
2.31.1

Re: [PATCH 6/9] pm/irq: make for_each_irq_desc() safe of irq_desc release

[Rafael J. Wysocki]

On Wed, Apr 20, 2022 at 4:06 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> The invloved context is no a RCU read section. Furthermore there may be
> more than one task at this point. Hence it demands a measure to prevent
> irq_desc from freeing. Use irq_lock_sparse to serve the protection
> purpose.

Can you please describe an example scenario in which the added locking
will prevent a failure from occurring?

> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> To: linux-kernel@vger.kernel.org
> ---
>  kernel/irq/pm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index ca71123a6130..4b67a4c7de3c 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -133,6 +133,7 @@ void suspend_device_irqs(void)
>         struct irq_desc *desc;
>         int irq;
>
> +       irq_lock_sparse();
>         for_each_irq_desc(irq, desc) {
>                 unsigned long flags;
>                 bool sync;
> @@ -146,6 +147,7 @@ void suspend_device_irqs(void)
>                 if (sync)
>                         synchronize_irq(irq);
>         }
> +       irq_unlock_sparse();
>  }
>  EXPORT_SYMBOL_GPL(suspend_device_irqs);
>
> @@ -186,6 +188,7 @@ static void resume_irqs(bool want_early)
>         struct irq_desc *desc;
>         int irq;
>
> +       /* The early resume stage is free of irq_desc release */
>         for_each_irq_desc(irq, desc) {
>                 unsigned long flags;
>                 bool is_early = desc->action &&
> --
> 2.31.1
>

Re: [PATCH 4/9] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Heiko Carstens]

On Wed, Apr 20, 2022 at 10:05:16PM +0800, Pingfan Liu wrote:
> irq_desc can be accessed safely in RCU read section as demonstrated by
> kstat_irqs_usr(). And raw_spin_lock_irqsave() context can provide a rcu
> read section, which can be utilized to get rid of irq_lock_sparse().
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> To: linux-s390@vger.kernel.org
> ---
>  arch/s390/kernel/irq.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
> index 3033f616e256..6302dc7874cf 100644
> --- a/arch/s390/kernel/irq.c
> +++ b/arch/s390/kernel/irq.c
> @@ -205,12 +205,13 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
>  	unsigned long flags;
>  	int cpu;
>  
> -	irq_lock_sparse();
> +	raw_spin_lock_irqsave(&desc->lock, flags);
>  	desc = irq_to_desc(irq);

How is this supposed to work? desc get's initialized after its random
stack value has been used as a pointer to lock something...

Re: [PATCH 3/9] irq/manage: remove some unreferenced code

[Miguel Ojeda]

On Wed, Apr 20, 2022 at 4:05 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> Neither remove_percpu_irq() nor for_each_irq_nr() has any users, so they
> can be removed.

For `.clang-format`:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH 6/9] pm/irq: make for_each_irq_desc() safe of irq_desc release

[Pingfan Liu]

On Wed, Apr 20, 2022 at 06:23:48PM +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 20, 2022 at 4:06 PM Pingfan Liu <kernelfans@gmail.com> wrote:
> >
> > The invloved context is no a RCU read section. Furthermore there may be
> > more than one task at this point. Hence it demands a measure to prevent
> > irq_desc from freeing. Use irq_lock_sparse to serve the protection
> > purpose.
> 
> Can you please describe an example scenario in which the added locking
> will prevent a failure from occurring?
> 

Sorry to forget mentioning that this is based on the code analysis.

Suppose the following scenario:
Two threads invloved
  threadA "hibernate" runs suspend_device_irqs()
  threadB "rcu_cpu_kthread" runs rcu_core()->rcu_do_batch(), which releases
  object, let's say irq_desc

Zoom in:
  threadA                                               threadB
  for_each_irq_desc(irq, desc) {
      get irq_descA which is under freeing
                                                    --->preempted by rcu_core()->rcu_do_batch()  which releases irq_descA
      raw_spin_lock_irqsave(&desc->lock, flags);
      //Oops

And since in the involved code piece, threadA runs in a preemptible
context, and there may be more than one thread at this stage. So the
preempted can happen.


Thanks,

	Pingfan


> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > To: linux-kernel@vger.kernel.org
> > ---
> >  kernel/irq/pm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index ca71123a6130..4b67a4c7de3c 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -133,6 +133,7 @@ void suspend_device_irqs(void)
> >         struct irq_desc *desc;
> >         int irq;
> >
> > +       irq_lock_sparse();
> >         for_each_irq_desc(irq, desc) {
> >                 unsigned long flags;
> >                 bool sync;
> > @@ -146,6 +147,7 @@ void suspend_device_irqs(void)
> >                 if (sync)
> >                         synchronize_irq(irq);
> >         }
> > +       irq_unlock_sparse();
> >  }
> >  EXPORT_SYMBOL_GPL(suspend_device_irqs);
> >
> > @@ -186,6 +188,7 @@ static void resume_irqs(bool want_early)
> >         struct irq_desc *desc;
> >         int irq;
> >
> > +       /* The early resume stage is free of irq_desc release */
> >         for_each_irq_desc(irq, desc) {
> >                 unsigned long flags;
> >                 bool is_early = desc->action &&
> > --
> > 2.31.1
> >

Re: [PATCH 4/9] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Pingfan Liu]

On Wed, Apr 20, 2022 at 08:16:37PM +0200, Heiko Carstens wrote:
> On Wed, Apr 20, 2022 at 10:05:16PM +0800, Pingfan Liu wrote:
> > irq_desc can be accessed safely in RCU read section as demonstrated by
> > kstat_irqs_usr(). And raw_spin_lock_irqsave() context can provide a rcu
> > read section, which can be utilized to get rid of irq_lock_sparse().
> > 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-s390@vger.kernel.org
> > ---
> >  arch/s390/kernel/irq.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
> > index 3033f616e256..6302dc7874cf 100644
> > --- a/arch/s390/kernel/irq.c
> > +++ b/arch/s390/kernel/irq.c
> > @@ -205,12 +205,13 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
> >  	unsigned long flags;
> >  	int cpu;
> >  
> > -	irq_lock_sparse();
> > +	raw_spin_lock_irqsave(&desc->lock, flags);
> >  	desc = irq_to_desc(irq);
> 
> How is this supposed to work? desc get's initialized after its random
> stack value has been used as a pointer to lock something...

Oops. You are right. What about using rcu_read_lock() directly?


diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 3033f616e256..45393919fe61 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -205,7 +205,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
 	unsigned long flags;
 	int cpu;
 
-	irq_lock_sparse();
+	rcu_read_lock();
 	desc = irq_to_desc(irq);
 	if (!desc)
 		goto out;
@@ -224,7 +224,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
 	seq_putc(p, '\n');
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 out:
-	irq_unlock_sparse();
+	rcu_read_unlock();
 }
 
 /*


Thanks,

	Pingfan

Re: [PATCH 6/9] pm/irq: make for_each_irq_desc() safe of irq_desc release

[Rafael J. Wysocki]

On Thu, Apr 21, 2022 at 5:31 AM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> On Wed, Apr 20, 2022 at 06:23:48PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Apr 20, 2022 at 4:06 PM Pingfan Liu <kernelfans@gmail.com> wrote:
> > >
> > > The invloved context is no a RCU read section. Furthermore there may be
> > > more than one task at this point. Hence it demands a measure to prevent
> > > irq_desc from freeing. Use irq_lock_sparse to serve the protection
> > > purpose.
> >
> > Can you please describe an example scenario in which the added locking
> > will prevent a failure from occurring?
> >
>
> Sorry to forget mentioning that this is based on the code analysis.
>
> Suppose the following scenario:
> Two threads invloved
>   threadA "hibernate" runs suspend_device_irqs()
>   threadB "rcu_cpu_kthread" runs rcu_core()->rcu_do_batch(), which releases
>   object, let's say irq_desc
>
> Zoom in:
>   threadA                                               threadB
>   for_each_irq_desc(irq, desc) {
>       get irq_descA which is under freeing
>                                                     --->preempted by rcu_core()->rcu_do_batch()  which releases irq_descA
>       raw_spin_lock_irqsave(&desc->lock, flags);
>       //Oops
>
> And since in the involved code piece, threadA runs in a preemptible
> context, and there may be more than one thread at this stage. So the
> preempted can happen.

Well, I'm still not sure that this can ever trigger in practice, but I
guess the locking can be added for extra safety.

Anyway, the above information should go into the changelog IMO.

That said ->

> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > To: linux-kernel@vger.kernel.org
> > > ---
> > >  kernel/irq/pm.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > index ca71123a6130..4b67a4c7de3c 100644
> > > --- a/kernel/irq/pm.c
> > > +++ b/kernel/irq/pm.c
> > > @@ -133,6 +133,7 @@ void suspend_device_irqs(void)
> > >         struct irq_desc *desc;
> > >         int irq;
> > >
> > > +       irq_lock_sparse();
> > >         for_each_irq_desc(irq, desc) {
> > >                 unsigned long flags;
> > >                 bool sync;
> > > @@ -146,6 +147,7 @@ void suspend_device_irqs(void)
> > >                 if (sync)
> > >                         synchronize_irq(irq);

-> is it entirely safe to call synchronize_irq() under irq_lock_sparse?

> > >         }
> > > +       irq_unlock_sparse();
> > >  }
> > >  EXPORT_SYMBOL_GPL(suspend_device_irqs);
> > >
> > > @@ -186,6 +188,7 @@ static void resume_irqs(bool want_early)
> > >         struct irq_desc *desc;
> > >         int irq;
> > >
> > > +       /* The early resume stage is free of irq_desc release */
> > >         for_each_irq_desc(irq, desc) {
> > >                 unsigned long flags;
> > >                 bool is_early = desc->action &&
> > > --
> > > 2.31.1
> > >

Re: [PATCH 4/9] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Heiko Carstens]

On Thu, Apr 21, 2022 at 11:36:17AM +0800, Pingfan Liu wrote:
> Oops. You are right. What about using rcu_read_lock() directly?
> 
> 
> diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
> index 3033f616e256..45393919fe61 100644
> --- a/arch/s390/kernel/irq.c
> +++ b/arch/s390/kernel/irq.c
> @@ -205,7 +205,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
>  	unsigned long flags;
>  	int cpu;
>  
> -	irq_lock_sparse();
> +	rcu_read_lock();
>  	desc = irq_to_desc(irq);
>  	if (!desc)
>  		goto out;
> @@ -224,7 +224,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
>  	seq_putc(p, '\n');
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>  out:
> -	irq_unlock_sparse();
> +	rcu_read_unlock();

That looks like it should work. Please resend and also add a reference
to commit 74bdf7815dfb ("genirq: Speedup show_interrupts()") which
explains why this works.

Re: [PATCH 7/9] irq: remove needless lock in takedown_cpu()

[Thomas Gleixner]

On Wed, Apr 20 2022 at 22:05, Pingfan Liu wrote:

First of all, the subject prefix for the core interrupt subsystem is
'genirq' and the sentence after the colon starts with an uppercase
letter. See:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index d0a9aa0b42e8..94a6b512c26d 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1033,18 +1033,16 @@ static int takedown_cpu(unsigned int cpu)
>  	kthread_park(st->thread);
>  
>  	/*
> -	 * Prevent irq alloc/free while the dying cpu reorganizes the
> -	 * interrupt affinities.
> +	 * RCU keeps watching 'cpu' until do_idle()->rcu_report_dead().
> +	 * And cpu_stopper's fn is dispatched with preemption disabled.
> +	 * So it can not occur to release a irq_desc.
>  	 */
> -	irq_lock_sparse();

Not everything is about RCU here. You really need to look at all moving
parts:

irq_migrate_all_off_this_cpu() relies on the allocated_irqs bitmap and
the sparse tree to be in consistent state, which is only guaranteed when
the sparse lock is held.

I'm not sure what you are trying to solve here. Not taking sparse_irq_lock
here is not gaining anything.

Thanks,

        tglx

Re: [PATCH 4/9] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Pingfan Liu]

On Thu, Apr 21, 2022 at 01:42:59PM +0200, Heiko Carstens wrote:
> On Thu, Apr 21, 2022 at 11:36:17AM +0800, Pingfan Liu wrote:
> > Oops. You are right. What about using rcu_read_lock() directly?
> > 
> > 
> > diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
> > index 3033f616e256..45393919fe61 100644
> > --- a/arch/s390/kernel/irq.c
> > +++ b/arch/s390/kernel/irq.c
> > @@ -205,7 +205,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
> >  	unsigned long flags;
> >  	int cpu;
> >  
> > -	irq_lock_sparse();
> > +	rcu_read_lock();
> >  	desc = irq_to_desc(irq);
> >  	if (!desc)
> >  		goto out;
> > @@ -224,7 +224,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
> >  	seq_putc(p, '\n');
> >  	raw_spin_unlock_irqrestore(&desc->lock, flags);
> >  out:
> > -	irq_unlock_sparse();
> > +	rcu_read_unlock();
> 
> That looks like it should work. Please resend and also add a reference
> to commit 74bdf7815dfb ("genirq: Speedup show_interrupts()") which
> explains why this works.

Thanks for your review. I will follow up with V2.

Regards,

	Pingfan

[PATCHv2] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Pingfan Liu]

As demonstrated by commit 74bdf7815dfb ("genirq: Speedup show_interrupts()"),
irq_desc can be accessed safely in RCU read section.

Hence here resorting to rcu read lock to get rid of irq_lock_sparse().

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: linux-kernel@vger.kernel.org
To: linux-s390@vger.kernel.org

---
 arch/s390/kernel/irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 3033f616e256..45393919fe61 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -205,7 +205,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
 	unsigned long flags;
 	int cpu;
 
-	irq_lock_sparse();
+	rcu_read_lock();
 	desc = irq_to_desc(irq);
 	if (!desc)
 		goto out;
@@ -224,7 +224,7 @@ static void show_msi_interrupt(struct seq_file *p, int irq)
 	seq_putc(p, '\n');
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 out:
-	irq_unlock_sparse();
+	rcu_read_unlock();
 }
 
 /*
-- 
2.31.1

Re: [PATCH 6/9] pm/irq: make for_each_irq_desc() safe of irq_desc release

[Pingfan Liu]

On Thu, Apr 21, 2022 at 12:57:28PM +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 21, 2022 at 5:31 AM Pingfan Liu <kernelfans@gmail.com> wrote:
> >
> > On Wed, Apr 20, 2022 at 06:23:48PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Apr 20, 2022 at 4:06 PM Pingfan Liu <kernelfans@gmail.com> wrote:
> > > >
> > > > The invloved context is no a RCU read section. Furthermore there may be
> > > > more than one task at this point. Hence it demands a measure to prevent
> > > > irq_desc from freeing. Use irq_lock_sparse to serve the protection
> > > > purpose.
> > >
> > > Can you please describe an example scenario in which the added locking
> > > will prevent a failure from occurring?
> > >
> >
> > Sorry to forget mentioning that this is based on the code analysis.
> >
> > Suppose the following scenario:
> > Two threads invloved
> >   threadA "hibernate" runs suspend_device_irqs()
> >   threadB "rcu_cpu_kthread" runs rcu_core()->rcu_do_batch(), which releases
> >   object, let's say irq_desc
> >
> > Zoom in:
> >   threadA                                               threadB
> >   for_each_irq_desc(irq, desc) {
> >       get irq_descA which is under freeing
> >                                                     --->preempted by rcu_core()->rcu_do_batch()  which releases irq_descA
> >       raw_spin_lock_irqsave(&desc->lock, flags);
> >       //Oops
> >
> > And since in the involved code piece, threadA runs in a preemptible
> > context, and there may be more than one thread at this stage. So the
> > preempted can happen.
> 
> Well, I'm still not sure that this can ever trigger in practice, but I

Yes, I also think it hardly happen. I had gone through all
accesses to irq_desc in kernel, and just want to make anything
completely obey the rule.
> guess the locking can be added for extra safety.
> 
> Anyway, the above information should go into the changelog IMO.
> 

OK, I will update it in V2.
> That said ->
> 
> > > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > To: linux-kernel@vger.kernel.org
> > > > ---
> > > >  kernel/irq/pm.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > > index ca71123a6130..4b67a4c7de3c 100644
> > > > --- a/kernel/irq/pm.c
> > > > +++ b/kernel/irq/pm.c
> > > > @@ -133,6 +133,7 @@ void suspend_device_irqs(void)
> > > >         struct irq_desc *desc;
> > > >         int irq;
> > > >
> > > > +       irq_lock_sparse();
> > > >         for_each_irq_desc(irq, desc) {
> > > >                 unsigned long flags;
> > > >                 bool sync;
> > > > @@ -146,6 +147,7 @@ void suspend_device_irqs(void)
> > > >                 if (sync)
> > > >                         synchronize_irq(irq);
> 
> -> is it entirely safe to call synchronize_irq() under irq_lock_sparse?

synchronize_irq - wait for pending IRQ handlers (on other CPUs). It
only holds irq_desc->lock and has no connections with irq sparse tree or
bitmap. I can not see any deadlock issue or miss something?

Thanks for your time.

Regards,

	Pingfan
> 
> > > >         }
> > > > +       irq_unlock_sparse();
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(suspend_device_irqs);
> > > >
> > > > @@ -186,6 +188,7 @@ static void resume_irqs(bool want_early)
> > > >         struct irq_desc *desc;
> > > >         int irq;
> > > >
> > > > +       /* The early resume stage is free of irq_desc release */
> > > >         for_each_irq_desc(irq, desc) {
> > > >                 unsigned long flags;
> > > >                 bool is_early = desc->action &&
> > > > --
> > > > 2.31.1
> > > >

Re: [PATCH 7/9] irq: remove needless lock in takedown_cpu()

[Pingfan Liu]

On Thu, Apr 21, 2022 at 06:11:56PM +0200, Thomas Gleixner wrote:
> On Wed, Apr 20 2022 at 22:05, Pingfan Liu wrote:
> 
> First of all, the subject prefix for the core interrupt subsystem is
> 'genirq' and the sentence after the colon starts with an uppercase
> letter. See:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index d0a9aa0b42e8..94a6b512c26d 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -1033,18 +1033,16 @@ static int takedown_cpu(unsigned int cpu)
> >  	kthread_park(st->thread);
> >  
> >  	/*
> > -	 * Prevent irq alloc/free while the dying cpu reorganizes the
> > -	 * interrupt affinities.
> > +	 * RCU keeps watching 'cpu' until do_idle()->rcu_report_dead().
> > +	 * And cpu_stopper's fn is dispatched with preemption disabled.
> > +	 * So it can not occur to release a irq_desc.
> >  	 */
> > -	irq_lock_sparse();
> 
> Not everything is about RCU here. You really need to look at all moving
> parts:
> 
> irq_migrate_all_off_this_cpu() relies on the allocated_irqs bitmap and
> the sparse tree to be in consistent state, which is only guaranteed when
> the sparse lock is held.
> 

For the irq which transfer from active to inactive(disappearing) after
fetching, desc->lock can serve the sync purpose. In this case,
irq_lock_sparse() is not needed. For a emergeing irq, I am not sure
about it.

> I'm not sure what you are trying to solve here. Not taking sparse_irq_lock
> here is not gaining anything.
>

It was a big lock preventing my original series to make kexec-reboot parallel on
arm64/riscv platform. But my new series takes a different way. And this
big lock is not a problem any longer.

Thanks for your time.

Regards,

	Pingfan

Re: [PATCH 7/9] irq: remove needless lock in takedown_cpu()

[Thomas Gleixner]

On Mon, Apr 25 2022 at 10:57, Pingfan Liu wrote:
> On Thu, Apr 21, 2022 at 06:11:56PM +0200, Thomas Gleixner wrote:
>> > -	irq_lock_sparse();
>> 
>> Not everything is about RCU here. You really need to look at all moving
>> parts:
>> 
>> irq_migrate_all_off_this_cpu() relies on the allocated_irqs bitmap and
>> the sparse tree to be in consistent state, which is only guaranteed when
>> the sparse lock is held.
>> 
>
> For the irq which transfer from active to inactive(disappearing) after
> fetching, desc->lock can serve the sync purpose. In this case,
> irq_lock_sparse() is not needed. For a emergeing irq, I am not sure
> about it.

No, it's required for the free case. The alloc case is
uninteresting. Care to look into the code?

irq_free_descs()
   lock(sparse);
   free_descs();
   bitmap_clear(allocated_irqs, from, cnt);
   unlock_sparse);
 
As free_descs() sets the sparse tree entry to NULL, up to the point
where bitmap_clear() finishes the state is inconsistent.

Now look at irq_migrate_all_off_this_cpu() and figure out what happens
when stop_machine() hits into the inconsistent state.

This can be fixed, but not by making mysterious claims about RCU and
desc->lock.

Thanks,

        tglx

Re: [PATCHv2] s390/irq: utilize RCU instead of irq_lock_sparse() in show_msi_interrupt()

[Heiko Carstens]

On Fri, Apr 22, 2022 at 06:02:12PM +0800, Pingfan Liu wrote:
> As demonstrated by commit 74bdf7815dfb ("genirq: Speedup show_interrupts()"),
> irq_desc can be accessed safely in RCU read section.
> 
> Hence here resorting to rcu read lock to get rid of irq_lock_sparse().
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> To: linux-s390@vger.kernel.org
> 
> ---
>  arch/s390/kernel/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks!

Re: [PATCH 7/9] irq: remove needless lock in takedown_cpu()

[Pingfan Liu]

On Mon, Apr 25, 2022 at 11:43:03AM +0200, Thomas Gleixner wrote:
> On Mon, Apr 25 2022 at 10:57, Pingfan Liu wrote:
> > On Thu, Apr 21, 2022 at 06:11:56PM +0200, Thomas Gleixner wrote:
> >> > -	irq_lock_sparse();
> >> 
> >> Not everything is about RCU here. You really need to look at all moving
> >> parts:
> >> 
> >> irq_migrate_all_off_this_cpu() relies on the allocated_irqs bitmap and
> >> the sparse tree to be in consistent state, which is only guaranteed when
> >> the sparse lock is held.
> >> 
> >
> > For the irq which transfer from active to inactive(disappearing) after
> > fetching, desc->lock can serve the sync purpose. In this case,
> > irq_lock_sparse() is not needed. For a emergeing irq, I am not sure
> > about it.
> 
> No, it's required for the free case. The alloc case is
> uninteresting. Care to look into the code?
> 

Yes, it is a good exercise. Thanks for the enlightenment.

> irq_free_descs()
>    lock(sparse);
>    free_descs();
>    bitmap_clear(allocated_irqs, from, cnt);
>    unlock_sparse);
>  
> As free_descs() sets the sparse tree entry to NULL, up to the point
> where bitmap_clear() finishes the state is inconsistent.
> 
> Now look at irq_migrate_all_off_this_cpu() and figure out what happens
> when stop_machine() hits into the inconsistent state.
> 

So the following code should fix the inconsistence between bitmap and
sparse tree.
diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c
index 1ed2b1739363..cd0d180f082d 100644
--- a/kernel/irq/cpuhotplug.c
+++ b/kernel/irq/cpuhotplug.c
@@ -161,6 +161,8 @@ void irq_migrate_all_off_this_cpu(void)
                bool affinity_broken;

                desc = irq_to_desc(irq);
+               if (!desc)
+                       continue;
                raw_spin_lock(&desc->lock);
                affinity_broken = migrate_one_irq(desc);
                raw_spin_unlock(&desc->lock);

> This can be fixed, but not by making mysterious claims about RCU and
> desc->lock.
> 

But I still think that desc->lock is critical to the consistence of the
irq _affinity_ if removing sparse lock in takedown_cpu().

For the free case, after applying the above patch, it should work.
void irq_migrate_all_off_this_cpu(void)
{
	for_each_active_irq(irq) {

		desc = irq_to_desc(irq);
		if (!desc)
			continue;
			                               ---> if breaking
						       in by free, then
						       migrate_one_irq()
						       will skip it
						       since the irq is
						       not activated any
						       long
		raw_spin_lock(&desc->lock);
		affinity_broken = migrate_one_irq(desc);
		raw_spin_unlock(&desc->lock);
		...
	}
}

But for the alloc case, it could be a problem.
void irq_migrate_all_off_this_cpu(void)
{
	for_each_active_irq(irq) {

		desc = irq_to_desc(irq);
		if (!desc)
			continue;
		raw_spin_lock(&desc->lock);
		affinity_broken = migrate_one_irq(desc);
		raw_spin_unlock(&desc->lock);
		...
                                                   ---> any new irq will
						   not be detected. But alloc_descs(start, cnt, node, affinity)
						   still associate the
						   irq with this cpu.
						   There is _no_
						   opportunity to clear
						   out this cpu from
						   desc->irq_common_data.affinity.

						   This is the affinity
						   inconsistent problem.
}


Thanks,

	Pingfan

[PATCHv2] genirq/PM: Make for_each_irq_desc() safe of irq_desc release

[Pingfan Liu]

First, this is a suspicion of the code, not a really encountered bug.

*** The scenario ***

Two threads involved
  threadA "hibernate" runs suspend_device_irqs()
  threadB "rcu_cpu_kthread" runs rcu_core()->rcu_do_batch(), which releases
  object, let's say irq_desc

Zoom in:
  threadA                                               threadB
  for_each_irq_desc(irq, desc) {
      get irq_descA which is under freeing
                                                    --->preempted by rcu_core()->rcu_do_batch()  which releases irq_descA
      raw_spin_lock_irqsave(&desc->lock, flags);
      //Oops

And since in the involved code piece, suspend_device_irqs() runs in a
preemptible context, and there may be more than one thread at this
point. So the preemption can happen.

*** The fix ***

Since there is a blockable synchronize_irq() inside the code piece,
resorting to irq_lock_sparse() to protect the irq_desc from
disappearing.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
To: linux-kernel@vger.kernel.org
---
v1 -> v2: improve commit log
 kernel/irq/pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index ca71123a6130..4b67a4c7de3c 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -133,6 +133,7 @@ void suspend_device_irqs(void)
 	struct irq_desc *desc;
 	int irq;
 
+	irq_lock_sparse();
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 		bool sync;
@@ -146,6 +147,7 @@ void suspend_device_irqs(void)
 		if (sync)
 			synchronize_irq(irq);
 	}
+	irq_unlock_sparse();
 }
 EXPORT_SYMBOL_GPL(suspend_device_irqs);
 
@@ -186,6 +188,7 @@ static void resume_irqs(bool want_early)
 	struct irq_desc *desc;
 	int irq;
 
+	/* The early resume stage is free of irq_desc release */
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 		bool is_early = desc->action &&
-- 
2.31.1