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

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?

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