[PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

The plic_set_affinity always call plic_irq_enable(), which clears up
the priority setting even the irq is only masked. This make the irq
unmasked unexpectly.

Replace the plic_irq_enable/disable() with plic_irq_toggle() to
avoid changing priority setting.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/irqchip/irq-sifive-plic.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index bf69a4802b71..5bf5050996da 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
 
 static void plic_irq_disable(struct irq_data *d)
 {
+	plic_irq_mask(d);
 	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
 }
 
@@ -179,12 +180,14 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	plic_irq_disable(d);
+	/* Invalidate the original routing entry */
+	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
+	/* Setting the new routing entry if irq is enabled */
 	if (!irqd_irq_disabled(d))
-		plic_irq_enable(d);
+		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
 
 	return IRQ_SET_MASK_OK_DONE;
 }
-- 
2.50.1


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

[PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

The plic_set_affinity always call plic_irq_enable(), which clears up
the priority setting even the irq is only masked. This make the irq
unmasked unexpectly.

Replace the plic_irq_enable/disable() with plic_irq_toggle() to
avoid changing priority setting.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/irqchip/irq-sifive-plic.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index bf69a4802b71..5bf5050996da 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
 
 static void plic_irq_disable(struct irq_data *d)
 {
+	plic_irq_mask(d);
 	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
 }
 
@@ -179,12 +180,14 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	plic_irq_disable(d);
+	/* Invalidate the original routing entry */
+	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
+	/* Setting the new routing entry if irq is enabled */
 	if (!irqd_irq_disabled(d))
-		plic_irq_enable(d);
+		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
 
 	return IRQ_SET_MASK_OK_DONE;
 }
-- 
2.50.1

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Nam Cao]

Inochi Amaoto <inochiama@gmail.com> writes:

> The plic_set_affinity always call plic_irq_enable(), which clears up
> the priority setting even the irq is only masked. This make the irq
> unmasked unexpectly.
>
> Replace the plic_irq_enable/disable() with plic_irq_toggle() to
> avoid changing priority setting.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index bf69a4802b71..5bf5050996da 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
>  
>  static void plic_irq_disable(struct irq_data *d)
>  {
> +	plic_irq_mask(d);
>  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
>  }

This part is not required for the problem you are addressing, right?

I do not oppose the change, I'm just curious if I miss something here.

>  
> @@ -179,12 +180,14 @@ static int plic_set_affinity(struct irq_data *d,
>  	if (cpu >= nr_cpu_ids)
>  		return -EINVAL;
>  
> -	plic_irq_disable(d);
> +	/* Invalidate the original routing entry */
> +	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
>  
>  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>  
> +	/* Setting the new routing entry if irq is enabled */
>  	if (!irqd_irq_disabled(d))
> -		plic_irq_enable(d);
> +		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
>  
>  	return IRQ_SET_MASK_OK_DONE;
>  }

This part makes sense:

Reviewed-by: Nam Cao <namcao@linutronix.de>
Tested-by: Nam Cao <namcao@linutronix.de> # VisionFive 2

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

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Nam Cao]

Inochi Amaoto <inochiama@gmail.com> writes:

> The plic_set_affinity always call plic_irq_enable(), which clears up
> the priority setting even the irq is only masked. This make the irq
> unmasked unexpectly.
>
> Replace the plic_irq_enable/disable() with plic_irq_toggle() to
> avoid changing priority setting.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index bf69a4802b71..5bf5050996da 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
>  
>  static void plic_irq_disable(struct irq_data *d)
>  {
> +	plic_irq_mask(d);
>  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
>  }

This part is not required for the problem you are addressing, right?

I do not oppose the change, I'm just curious if I miss something here.

>  
> @@ -179,12 +180,14 @@ static int plic_set_affinity(struct irq_data *d,
>  	if (cpu >= nr_cpu_ids)
>  		return -EINVAL;
>  
> -	plic_irq_disable(d);
> +	/* Invalidate the original routing entry */
> +	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
>  
>  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
>  
> +	/* Setting the new routing entry if irq is enabled */
>  	if (!irqd_irq_disabled(d))
> -		plic_irq_enable(d);
> +		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
>  
>  	return IRQ_SET_MASK_OK_DONE;
>  }

This part makes sense:

Reviewed-by: Nam Cao <namcao@linutronix.de>
Tested-by: Nam Cao <namcao@linutronix.de> # VisionFive 2

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

On Thu, Aug 07, 2025 at 02:39:42PM +0200, Nam Cao wrote:
> Inochi Amaoto <inochiama@gmail.com> writes:
> 
> > The plic_set_affinity always call plic_irq_enable(), which clears up
> > the priority setting even the irq is only masked. This make the irq
> > unmasked unexpectly.
> >
> > Replace the plic_irq_enable/disable() with plic_irq_toggle() to
> > avoid changing priority setting.
> >
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index bf69a4802b71..5bf5050996da 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
> >  
> >  static void plic_irq_disable(struct irq_data *d)
> >  {
> > +	plic_irq_mask(d);
> >  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
> >  }
> 
> This part is not required for the problem you are addressing, right?
> 
> I do not oppose the change, I'm just curious if I miss something here.
> 

It is true, this is added because it is needed to follow
the disable required of the irqchip. I think it is better
to split to a separate one.

> >  
> > @@ -179,12 +180,14 @@ static int plic_set_affinity(struct irq_data *d,
> >  	if (cpu >= nr_cpu_ids)
> >  		return -EINVAL;
> >  
> > -	plic_irq_disable(d);
> > +	/* Invalidate the original routing entry */
> > +	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
> >  
> >  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
> >  
> > +	/* Setting the new routing entry if irq is enabled */
> >  	if (!irqd_irq_disabled(d))
> > -		plic_irq_enable(d);
> > +		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
> >  
> >  	return IRQ_SET_MASK_OK_DONE;
> >  }
> 
> This part makes sense:
> 
> Reviewed-by: Nam Cao <namcao@linutronix.de>
> Tested-by: Nam Cao <namcao@linutronix.de> # VisionFive 2

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

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

On Thu, Aug 07, 2025 at 02:39:42PM +0200, Nam Cao wrote:
> Inochi Amaoto <inochiama@gmail.com> writes:
> 
> > The plic_set_affinity always call plic_irq_enable(), which clears up
> > the priority setting even the irq is only masked. This make the irq
> > unmasked unexpectly.
> >
> > Replace the plic_irq_enable/disable() with plic_irq_toggle() to
> > avoid changing priority setting.
> >
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index bf69a4802b71..5bf5050996da 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
> >  
> >  static void plic_irq_disable(struct irq_data *d)
> >  {
> > +	plic_irq_mask(d);
> >  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
> >  }
> 
> This part is not required for the problem you are addressing, right?
> 
> I do not oppose the change, I'm just curious if I miss something here.
> 

It is true, this is added because it is needed to follow
the disable required of the irqchip. I think it is better
to split to a separate one.

> >  
> > @@ -179,12 +180,14 @@ static int plic_set_affinity(struct irq_data *d,
> >  	if (cpu >= nr_cpu_ids)
> >  		return -EINVAL;
> >  
> > -	plic_irq_disable(d);
> > +	/* Invalidate the original routing entry */
> > +	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
> >  
> >  	irq_data_update_effective_affinity(d, cpumask_of(cpu));
> >  
> > +	/* Setting the new routing entry if irq is enabled */
> >  	if (!irqd_irq_disabled(d))
> > -		plic_irq_enable(d);
> > +		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
> >  
> >  	return IRQ_SET_MASK_OK_DONE;
> >  }
> 
> This part makes sense:
> 
> Reviewed-by: Nam Cao <namcao@linutronix.de>
> Tested-by: Nam Cao <namcao@linutronix.de> # VisionFive 2

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

On Fri, Aug 08, 2025 at 06:01:39AM +0800, Inochi Amaoto wrote:
> On Thu, Aug 07, 2025 at 02:39:42PM +0200, Nam Cao wrote:
> > Inochi Amaoto <inochiama@gmail.com> writes:
> > 
> > > The plic_set_affinity always call plic_irq_enable(), which clears up
> > > the priority setting even the irq is only masked. This make the irq
> > > unmasked unexpectly.
> > >
> > > Replace the plic_irq_enable/disable() with plic_irq_toggle() to
> > > avoid changing priority setting.
> > >
> > > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > > ---
> > >  drivers/irqchip/irq-sifive-plic.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index bf69a4802b71..5bf5050996da 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
> > >  
> > >  static void plic_irq_disable(struct irq_data *d)
> > >  {
> > > +	plic_irq_mask(d);
> > >  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
> > >  }
> > 
> > This part is not required for the problem you are addressing, right?
> > 
> > I do not oppose the change, I'm just curious if I miss something here.
> > 
> 
> It is true, this is added because it is needed to follow
> the disable required of the irqchip. I think it is better
> to split to a separate one.
> 

After some dig in, I found it is not very necessary to add this,
When all enable bit is clear, the PRIORIT register of irq is
not functional, so only umask the irq does not make sense. Only
calling irq_enable does enable the irq.

I prefer to add a comment to describe this behavior, instead of
adding this change in a separate patch.

Regards,
Inochi

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

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

On Fri, Aug 08, 2025 at 06:01:39AM +0800, Inochi Amaoto wrote:
> On Thu, Aug 07, 2025 at 02:39:42PM +0200, Nam Cao wrote:
> > Inochi Amaoto <inochiama@gmail.com> writes:
> > 
> > > The plic_set_affinity always call plic_irq_enable(), which clears up
> > > the priority setting even the irq is only masked. This make the irq
> > > unmasked unexpectly.
> > >
> > > Replace the plic_irq_enable/disable() with plic_irq_toggle() to
> > > avoid changing priority setting.
> > >
> > > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > > ---
> > >  drivers/irqchip/irq-sifive-plic.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index bf69a4802b71..5bf5050996da 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -148,6 +148,7 @@ static void plic_irq_enable(struct irq_data *d)
> > >  
> > >  static void plic_irq_disable(struct irq_data *d)
> > >  {
> > > +	plic_irq_mask(d);
> > >  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
> > >  }
> > 
> > This part is not required for the problem you are addressing, right?
> > 
> > I do not oppose the change, I'm just curious if I miss something here.
> > 
> 
> It is true, this is added because it is needed to follow
> the disable required of the irqchip. I think it is better
> to split to a separate one.
> 

After some dig in, I found it is not very necessary to add this,
When all enable bit is clear, the PRIORIT register of irq is
not functional, so only umask the irq does not make sense. Only
calling irq_enable does enable the irq.

I prefer to add a comment to describe this behavior, instead of
adding this change in a separate patch.

Regards,
Inochi

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Nam Cao]

Inochi Amaoto <inochiama@gmail.com> writes:

> On Fri, Aug 08, 2025 at 06:01:39AM +0800, Inochi Amaoto wrote:
> After some dig in, I found it is not very necessary to add this,
> When all enable bit is clear, the PRIORIT register of irq is
> not functional, so only umask the irq does not make sense. Only
> calling irq_enable does enable the irq.

Yeah, I contemplated doing this myself when I added the unmask to
plic_irq_enable(), because it looks more natural that plic_irq_enable()
and plic_irq_disable() are opposite. But I don't think it is necessary.

> I prefer to add a comment to describe this behavior, instead of
> adding this change in a separate patch.

No preference from me.

Nam

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

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Nam Cao]

Inochi Amaoto <inochiama@gmail.com> writes:

> On Fri, Aug 08, 2025 at 06:01:39AM +0800, Inochi Amaoto wrote:
> After some dig in, I found it is not very necessary to add this,
> When all enable bit is clear, the PRIORIT register of irq is
> not functional, so only umask the irq does not make sense. Only
> calling irq_enable does enable the irq.

Yeah, I contemplated doing this myself when I added the unmask to
plic_irq_enable(), because it looks more natural that plic_irq_enable()
and plic_irq_disable() are opposite. But I don't think it is necessary.

> I prefer to add a comment to describe this behavior, instead of
> adding this change in a separate patch.

No preference from me.

Nam

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

On Fri, Aug 08, 2025 at 06:52:42AM +0200, Nam Cao wrote:
> Inochi Amaoto <inochiama@gmail.com> writes:
> 
> > On Fri, Aug 08, 2025 at 06:01:39AM +0800, Inochi Amaoto wrote:
> > After some dig in, I found it is not very necessary to add this,
> > When all enable bit is clear, the PRIORIT register of irq is
> > not functional, so only umask the irq does not make sense. Only
> > calling irq_enable does enable the irq.
> 
> Yeah, I contemplated doing this myself when I added the unmask to
> plic_irq_enable(), because it looks more natural that plic_irq_enable()
> and plic_irq_disable() are opposite. But I don't think it is necessary.
> 
> > I prefer to add a comment to describe this behavior, instead of
> > adding this change in a separate patch.
> 
> No preference from me.
> 

OK, I will just remove this mask function.

Regards,
Inochi

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

Re: [PATCH] irqchip/sifive-plic: Respect mask state when setting affinity

[Inochi Amaoto]

On Fri, Aug 08, 2025 at 06:52:42AM +0200, Nam Cao wrote:
> Inochi Amaoto <inochiama@gmail.com> writes:
> 
> > On Fri, Aug 08, 2025 at 06:01:39AM +0800, Inochi Amaoto wrote:
> > After some dig in, I found it is not very necessary to add this,
> > When all enable bit is clear, the PRIORIT register of irq is
> > not functional, so only umask the irq does not make sense. Only
> > calling irq_enable does enable the irq.
> 
> Yeah, I contemplated doing this myself when I added the unmask to
> plic_irq_enable(), because it looks more natural that plic_irq_enable()
> and plic_irq_disable() are opposite. But I don't think it is necessary.
> 
> > I prefer to add a comment to describe this behavior, instead of
> > adding this change in a separate patch.
> 
> No preference from me.
> 

OK, I will just remove this mask function.

Regards,
Inochi