1. Patchew
  2. linux
  3. View series

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

Inochi Amaoto posted 1 patch 1 month, 4 weeks ago
There is a newer version of this series
drivers/irqchip/irq-sifive-plic.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
[PATCH] irqchip/sifive-plic: Respect mask state when setting affinity
Posted by Inochi Amaoto 1 month, 4 weeks ago 
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
Posted by Nam Cao 1 month, 4 weeks ago 
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
Posted by Inochi Amaoto 1 month, 4 weeks ago 
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
Posted by Inochi Amaoto 1 month, 4 weeks ago 
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
Posted by Nam Cao 1 month, 4 weeks ago 
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
Posted by Inochi Amaoto 1 month, 3 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.