[PATCH v5 0/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[PATCH v5 0/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[Bibo Mao]

During suspend and resume, other CPUs except CPU0 are hot-unpluged and
IRQs are migrated to CPU0. So it is not necessary to restore irq
affinity for eiointc irq controller.

Also there is small optimization for the interrupt dispatch function
eiointc_irq_dispatch(). For example there are 256 IRQs supported for
eiointc on Loongson Loongson-3A5000 and Loongson-2K2000 system, 128 IRQs
on Loongson-2K0500 platform, eiointc irq handler reads the bitmap and find
pending irqs when irq happens. So there are four times of consecutive
iocsr_read64 operations for all the bits to find all pending irqs. If the
pending bitmap is zero, it means that there is no pending irq for the this
irq bitmap range, we can skip handling to avoid some useless operations
such as clearing hw ISR.

---
 Changes in v5:
  1. Refine changlog

 Changes in v4:
  1. Adjust order of the patch and put the simple patch as first one.
  2. Modify comments in function eiointc_irq_dispatch() suitable for all
hw platforms.

 Changes in v3:
   Split the patch into three small patches

 Changes in v2:
   Modify changelog and comments
---

Bibo Mao (3):
  irqchip/loongson-eiointc: Typo fix in function eiointc_domain_alloc
  irqchip/loongson-eiointc: Skip handling if there is no pending irq
  irqchip/loongson-eiointc: Refine irq affinity setting during resume

 drivers/irqchip/irq-loongson-eiointc.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)


base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.39.3

[PATCH v5 1/3] irqchip/loongson-eiointc: Typo fix in function eiointc_domain_alloc

[Bibo Mao]

There is small typo in function eiointc_domain_alloc(), and there is no
definition about eiointc struct, instead its name should be eiointc_priv
struct. It is strange that there is no warning with gcc compiler. This
patch fixes the small typo issue.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Acked-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/irqchip/irq-loongson-eiointc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index 1623cd779175..b3736bdd4b9f 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -241,7 +241,7 @@ static int eiointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	int ret;
 	unsigned int i, type;
 	unsigned long hwirq = 0;
-	struct eiointc *priv = domain->host_data;
+	struct eiointc_priv *priv = domain->host_data;
 
 	ret = irq_domain_translate_onecell(domain, arg, &hwirq, &type);
 	if (ret)
-- 
2.39.3

[PATCH v5 2/3] irqchip/loongson-eiointc: Skip handling if there is no pending irq

[Bibo Mao]

It is one simple optimization in the interrupt dispatch function
eiointc_irq_dispatch(). There are 256 IRQs supported for eiointc on
Loongson-3A5000 and Loongson-2K2000 platform, 128 IRQs on Loongson-2K0500
platform, eiointc irq handler reads the bitmap and find pending irqs
when irq happens. So there are several consecutive iocsr_read64
operations for the all bits to find all pending irqs. If the pending
bitmap is zero, it means that there is no pending irq for the this
irq bitmap range, we can skip handling to avoid some useless operations
such as clearing hw ISR.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Acked-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/irqchip/irq-loongson-eiointc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index b3736bdd4b9f..6a71a8c29ac7 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -198,6 +198,12 @@ static void eiointc_irq_dispatch(struct irq_desc *desc)
 
 	for (i = 0; i < eiointc_priv[0]->vec_count / VEC_COUNT_PER_REG; i++) {
 		pending = iocsr_read64(EIOINTC_REG_ISR + (i << 3));
+
+		/* Skip handling if pending bitmap is zero */
+		if (!pending)
+			continue;
+
+		/* Clear the IRQs */
 		iocsr_write64(pending, EIOINTC_REG_ISR + (i << 3));
 		while (pending) {
 			int bit = __ffs(pending);
-- 
2.39.3

[PATCH v5 3/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[Bibo Mao]

During suspend and resume, CPUs except CPU0 can be hot-unpluged and IRQs
will be migrated to CPU0. So it is not necessary to restore irq affinity
for eiointc irq controller when system resumes. This patch removes this
piece of code about irq affinity restoring in function eiointc_resume().

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 drivers/irqchip/irq-loongson-eiointc.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index 6a71a8c29ac7..b64cbe3052e8 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -310,23 +310,7 @@ static int eiointc_suspend(void)
 
 static void eiointc_resume(void)
 {
-	int i, j;
-	struct irq_desc *desc;
-	struct irq_data *irq_data;
-
 	eiointc_router_init(0);
-
-	for (i = 0; i < nr_pics; i++) {
-		for (j = 0; j < eiointc_priv[0]->vec_count; j++) {
-			desc = irq_resolve_mapping(eiointc_priv[i]->eiointc_domain, j);
-			if (desc && desc->handle_irq && desc->handle_irq != handle_bad_irq) {
-				raw_spin_lock(&desc->lock);
-				irq_data = irq_domain_get_irq_data(eiointc_priv[i]->eiointc_domain, irq_desc_get_irq(desc));
-				eiointc_set_irq_affinity(irq_data, irq_data->common->affinity, 0);
-				raw_spin_unlock(&desc->lock);
-			}
-		}
-	}
 }
 
 static struct syscore_ops eiointc_syscore_ops = {
-- 
2.39.3

Re: [PATCH v5 1/3] irqchip/loongson-eiointc: Typo fix in function eiointc_domain_alloc

[Thomas Gleixner]

On Tue, Jan 30 2024 at 16:27, Bibo Mao wrote:
> There is small typo in function eiointc_domain_alloc(), and there is
> no

This is not a typo. The code uses an undeclared struct type, no?

> definition about eiointc struct, instead its name should be eiointc_priv
> struct. It is strange that there is no warning with gcc compiler.

It's absolutely not strange. The compiler treats 'struct eiointc *priv'
as forward declaration and it does not complain because the assignment
is a void pointer and it's handed into irq_domain_set_info() as a void
pointer argument. C is wonderful, isn't it?

> This patch fixes the small typo issue.

# git grep 'This patch' Documentation/process/

> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> Acked-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  drivers/irqchip/irq-loongson-eiointc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
> index 1623cd779175..b3736bdd4b9f 100644
> --- a/drivers/irqchip/irq-loongson-eiointc.c
> +++ b/drivers/irqchip/irq-loongson-eiointc.c
> @@ -241,7 +241,7 @@ static int eiointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  	int ret;
>  	unsigned int i, type;
>  	unsigned long hwirq = 0;
> -	struct eiointc *priv = domain->host_data;
> +	struct eiointc_priv *priv = domain->host_data;
>  
>  	ret = irq_domain_translate_onecell(domain, arg, &hwirq, &type);
>  	if (ret)

[tip: irq/urgent] irqchip/loongson-eiointc: Use correct struct type in eiointc_domain_alloc()

[tip-bot2 for Bibo Mao]

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

Commit-ID:     f1c2765c6afcd1f71f76ed8c9bf94acedab4cecb
Gitweb:        https://git.kernel.org/tip/f1c2765c6afcd1f71f76ed8c9bf94acedab4cecb
Author:        Bibo Mao <maobibo@loongson.cn>
AuthorDate:    Tue, 30 Jan 2024 16:27:20 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Feb 2024 10:26:15 +01:00

irqchip/loongson-eiointc: Use correct struct type in eiointc_domain_alloc()

eiointc_domain_alloc() uses struct eiointc, which is not defined, for a
pointer. Older compilers treat that as a forward declaration and due to
assignment of a void pointer there is no warning emitted. As the variable
is then handed in as a void pointer argument to irq_domain_set_info() the
code is functional.

Use struct eiointc_priv instead.

[ tglx: Rewrote changelog ]

Fixes: dd281e1a1a93 ("irqchip: Add Loongson Extended I/O interrupt controller support")
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Huacai Chen <chenhuacai@loongson.cn>
Link: https://lore.kernel.org/r/20240130082722.2912576-2-maobibo@loongson.cn
---
 drivers/irqchip/irq-loongson-eiointc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index 1623cd7..b3736bd 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -241,7 +241,7 @@ static int eiointc_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	int ret;
 	unsigned int i, type;
 	unsigned long hwirq = 0;
-	struct eiointc *priv = domain->host_data;
+	struct eiointc_priv *priv = domain->host_data;
 
 	ret = irq_domain_translate_onecell(domain, arg, &hwirq, &type);
 	if (ret)

Re: [PATCH v5 3/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[Thomas Gleixner]

On Tue, Jan 30 2024 at 16:27, Bibo Mao wrote:
> During suspend and resume, CPUs except CPU0 can be hot-unpluged and IRQs
> will be migrated to CPU0. So it is not necessary to restore irq affinity
> for eiointc irq controller when system resumes.

That's not the reason. The point is that eiointc_router_init() which is
invoked in the resume path affines all interrupts to CPU0, so the
restore operation is redundant, no?

> This patch removes this piece of code about irq affinity restoring in
> function eiointc_resume().

Again. 'This patch' is pointless because we already know that this is a
patch, no?

[tip: irq/core] irqchip/loongson-eiointc: Remove explicit interrupt affinity restore on resume

[tip-bot2 for Bibo Mao]

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

Commit-ID:     83c0708719f77018cd3b98b0011c9526a3e0e2ca
Gitweb:        https://git.kernel.org/tip/83c0708719f77018cd3b98b0011c9526a3e0e2ca
Author:        Bibo Mao <maobibo@loongson.cn>
AuthorDate:    Tue, 30 Jan 2024 16:27:22 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Feb 2024 10:53:14 +01:00

irqchip/loongson-eiointc: Remove explicit interrupt affinity restore on resume

During suspend all CPUs except CPU0 are hot-unpluged and all active
interrupts are migrated to CPU0.

On resume eiointc_router_init() affines all interrupts to CPU0, so the
subsequent explicit interrupt affinity restore is redundant.

Remove it.

[ tglx: Rewrote changelog ]

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20240130082722.2912576-4-maobibo@loongson.cn
---
 drivers/irqchip/irq-loongson-eiointc.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index fad22e2..405f622 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -310,23 +310,7 @@ static int eiointc_suspend(void)
 
 static void eiointc_resume(void)
 {
-	int i, j;
-	struct irq_desc *desc;
-	struct irq_data *irq_data;
-
 	eiointc_router_init(0);
-
-	for (i = 0; i < nr_pics; i++) {
-		for (j = 0; j < eiointc_priv[0]->vec_count; j++) {
-			desc = irq_resolve_mapping(eiointc_priv[i]->eiointc_domain, j);
-			if (desc && desc->handle_irq && desc->handle_irq != handle_bad_irq) {
-				raw_spin_lock(&desc->lock);
-				irq_data = irq_domain_get_irq_data(eiointc_priv[i]->eiointc_domain, irq_desc_get_irq(desc));
-				eiointc_set_irq_affinity(irq_data, irq_data->common->affinity, 0);
-				raw_spin_unlock(&desc->lock);
-			}
-		}
-	}
 }
 
 static struct syscore_ops eiointc_syscore_ops = {

[tip: irq/core] irqchip/loongson-eiointc: Skip handling if there is no pending interrupt

[tip-bot2 for Bibo Mao]

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

Commit-ID:     3eece72ded7f67776731709702f3d1b9893b6a4f
Gitweb:        https://git.kernel.org/tip/3eece72ded7f67776731709702f3d1b9893b6a4f
Author:        Bibo Mao <maobibo@loongson.cn>
AuthorDate:    Tue, 30 Jan 2024 16:27:21 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Feb 2024 10:53:14 +01:00

irqchip/loongson-eiointc: Skip handling if there is no pending interrupt

eiointc_irq_dispatch() iterates over the pending bit registers of the
interrupt controller and evaluates the result even if there is no interrupt
pending in a particular 64bit chunk.

Skip handling and especially the pointless write back for clearing the
non-pending bits if a chunk is empty.

[ tglx: Massaged changelog ]

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Huacai Chen <chenhuacai@loongson.cn>
Link: https://lore.kernel.org/r/20240130082722.2912576-3-maobibo@loongson.cn

---
 drivers/irqchip/irq-loongson-eiointc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index 1623cd7..fad22e2 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -198,6 +198,12 @@ static void eiointc_irq_dispatch(struct irq_desc *desc)
 
 	for (i = 0; i < eiointc_priv[0]->vec_count / VEC_COUNT_PER_REG; i++) {
 		pending = iocsr_read64(EIOINTC_REG_ISR + (i << 3));
+
+		/* Skip handling if pending bitmap is zero */
+		if (!pending)
+			continue;
+
+		/* Clear the IRQs */
 		iocsr_write64(pending, EIOINTC_REG_ISR + (i << 3));
 		while (pending) {
 			int bit = __ffs(pending);

Re: [PATCH v5 3/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[maobibo]

Hi Thomas,

On 2024/2/13 下午5:49, Thomas Gleixner wrote:
> On Tue, Jan 30 2024 at 16:27, Bibo Mao wrote:
>> During suspend and resume, CPUs except CPU0 can be hot-unpluged and IRQs
>> will be migrated to CPU0. So it is not necessary to restore irq affinity
>> for eiointc irq controller when system resumes.
> 
> That's not the reason. The point is that eiointc_router_init() which is
> invoked in the resume path affines all interrupts to CPU0, so the
> restore operation is redundant, no?
yes, it is. eiointc_router_init() will enable the irqs and affine all 
interrupts to CPU0. And it is redundant, the aim of deleted code wants
to resume older interrupt affinity when executing "echo mem > 
/sys/power/state".
> 
>> This patch removes this piece of code about irq affinity restoring in
>> function eiointc_resume().
> 
> Again. 'This patch' is pointless because we already know that this is a
> patch, no?
Thanks for your kindly help, English is somewhat difficult for me :)

Regards
Bibo Mao

Re: [PATCH v5 3/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[Huacai Chen]

Hi, Thomas,

On Tue, Feb 13, 2024 at 5:49 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, Jan 30 2024 at 16:27, Bibo Mao wrote:
> > During suspend and resume, CPUs except CPU0 can be hot-unpluged and IRQs
> > will be migrated to CPU0. So it is not necessary to restore irq affinity
> > for eiointc irq controller when system resumes.
>
> That's not the reason. The point is that eiointc_router_init() which is
> invoked in the resume path affines all interrupts to CPU0, so the
> restore operation is redundant, no?
I'm sorry for the late response but I think this is a little wrong.
When irq_migrate_all_off_this_cpu() is called at hot-unplug, if an
irqdesc is irqd_affinity_is_managed() then its affinity is untouched
(doesn't change to CPU0). Then after resume we should not keep its
affinity on CPU0 set by eiointc_router_init() , but need to restore
its old affinity.

Huacai

>
> > This patch removes this piece of code about irq affinity restoring in
> > function eiointc_resume().
>
> Again. 'This patch' is pointless because we already know that this is a
> patch, no?
>

Re: [PATCH v5 3/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[Thomas Gleixner]

On Wed, Mar 13 2024 at 14:20, Huacai Chen wrote:
> On Tue, Feb 13, 2024 at 5:49 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> On Tue, Jan 30 2024 at 16:27, Bibo Mao wrote:
>> > During suspend and resume, CPUs except CPU0 can be hot-unpluged and IRQs
>> > will be migrated to CPU0. So it is not necessary to restore irq affinity
>> > for eiointc irq controller when system resumes.
>>
>> That's not the reason. The point is that eiointc_router_init() which is
>> invoked in the resume path affines all interrupts to CPU0, so the
>> restore operation is redundant, no?
> I'm sorry for the late response but I think this is a little wrong.
> When irq_migrate_all_off_this_cpu() is called at hot-unplug, if an
> irqdesc is irqd_affinity_is_managed() then its affinity is untouched
> (doesn't change to CPU0). Then after resume we should not keep its
> affinity on CPU0 set by eiointc_router_init() , but need to restore
> its old affinity.

Affinity is restored when the interrupt is started up again, so yes the
affinity setting should not be changed.

Re: [PATCH v5 3/3] irqchip/loongson-eiointc: Refine irq affinity setting during resume

[Huacai Chen]

On Wed, Mar 13, 2024 at 8:39 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Mar 13 2024 at 14:20, Huacai Chen wrote:
> > On Tue, Feb 13, 2024 at 5:49 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> On Tue, Jan 30 2024 at 16:27, Bibo Mao wrote:
> >> > During suspend and resume, CPUs except CPU0 can be hot-unpluged and IRQs
> >> > will be migrated to CPU0. So it is not necessary to restore irq affinity
> >> > for eiointc irq controller when system resumes.
> >>
> >> That's not the reason. The point is that eiointc_router_init() which is
> >> invoked in the resume path affines all interrupts to CPU0, so the
> >> restore operation is redundant, no?
> > I'm sorry for the late response but I think this is a little wrong.
> > When irq_migrate_all_off_this_cpu() is called at hot-unplug, if an
> > irqdesc is irqd_affinity_is_managed() then its affinity is untouched
> > (doesn't change to CPU0). Then after resume we should not keep its
> > affinity on CPU0 set by eiointc_router_init() , but need to restore
> > its old affinity.
>
> Affinity is restored when the interrupt is started up again, so yes the
> affinity setting should not be changed.
OK, thanks.

Huacai