diff mbox series

[1/1] UBUNTU: SAUCE: irqchip/gic-v3-its: Don't return errors in its_irq_domain_activate()

Message ID 20200108224102.18932-2-sultan.alsawaf@canonical.com
State New
Headers show
Series Fix ThunderX crashes on boot | expand

Commit Message

Sultan Alsawaf Jan. 8, 2020, 10:41 p.m. UTC 
BugLink: https://bugs.launchpad.net/bugs/1857074

Commit bb9b428a5c83 ("genirq/irqdomain: Allow irq_domain_activate_irq() to fail")
added error handling for when irq domain activation would fail. However,
our irqchip driver was not written with the intention of handling domain
activation failure. When an irqchip's domain activation function fails,
__irq_domain_activate_irq() in kernel/irq/irqdomain.c parses the error
and disables the parent irq domain in response. This is not what was
intended to happen for ITS_FLAGS_WORKAROUND_CAVIUM_23144, which leaves
IRQs erroneously disabled on some CPUs.

There is no legitimate failure case for its_irq_domain_activate(), so we
don't need to subscribe to __irq_domain_activate_irq()'s error handling,
and the current state of the driver cannot cope with the error handling
anyway. To that end, always return 0 from its_irq_domain_activate() so
that IRQs aren't erroneously disabled for machines with
ITS_FLAGS_WORKAROUND_CAVIUM_23144.

Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marcelo Henrique Cerri Jan. 8, 2020, 10:44 p.m. UTC | #1 
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Marcelo Henrique Cerri Jan. 8, 2020, 10:45 p.m. UTC | #2 
This is intended only to bionic:linux 4.15.

On Wed, Jan 08, 2020 at 02:41:02PM -0800, Sultan Alsawaf wrote:
> BugLink: https://bugs.launchpad.net/bugs/1857074
> 
> Commit bb9b428a5c83 ("genirq/irqdomain: Allow irq_domain_activate_irq() to fail")
> added error handling for when irq domain activation would fail. However,
> our irqchip driver was not written with the intention of handling domain
> activation failure. When an irqchip's domain activation function fails,
> __irq_domain_activate_irq() in kernel/irq/irqdomain.c parses the error
> and disables the parent irq domain in response. This is not what was
> intended to happen for ITS_FLAGS_WORKAROUND_CAVIUM_23144, which leaves
> IRQs erroneously disabled on some CPUs.
> 
> There is no legitimate failure case for its_irq_domain_activate(), so we
> don't need to subscribe to __irq_domain_activate_irq()'s error handling,
> and the current state of the driver cannot cope with the error handling
> anyway. To that end, always return 0 from its_irq_domain_activate() so
> that IRQs aren't erroneously disabled for machines with
> ITS_FLAGS_WORKAROUND_CAVIUM_23144.
> 
> Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 5cc27ad2abd1..67386413b907 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2407,7 +2407,7 @@ static int its_irq_domain_activate(struct irq_domain *domain,
>  	cpu = cpumask_first_and(cpu_mask, cpu_online_mask);
>  	if (cpu >= nr_cpu_ids) {
>  		if (its_dev->its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144)
> -			return -EINVAL;
> +			return 0;
>  
>  		cpu = cpumask_first(cpu_online_mask);
>  	}
> -- 
> 2.20.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Connor Kuehl Jan. 8, 2020, 10:46 p.m. UTC | #3 
On 1/8/20 2:41 PM, Sultan Alsawaf wrote:
> BugLink: https://bugs.launchpad.net/bugs/1857074
> 
> Commit bb9b428a5c83 ("genirq/irqdomain: Allow irq_domain_activate_irq() to fail")
> added error handling for when irq domain activation would fail. However,
> our irqchip driver was not written with the intention of handling domain
> activation failure. When an irqchip's domain activation function fails,
> __irq_domain_activate_irq() in kernel/irq/irqdomain.c parses the error
> and disables the parent irq domain in response. This is not what was
> intended to happen for ITS_FLAGS_WORKAROUND_CAVIUM_23144, which leaves
> IRQs erroneously disabled on some CPUs.
> 
> There is no legitimate failure case for its_irq_domain_activate(), so we
> don't need to subscribe to __irq_domain_activate_irq()'s error handling,
> and the current state of the driver cannot cope with the error handling
> anyway. To that end, always return 0 from its_irq_domain_activate() so
> that IRQs aren't erroneously disabled for machines with
> ITS_FLAGS_WORKAROUND_CAVIUM_23144.
> 
> Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>

Acked-by: Connor Kuehl <connor.kuehl@canonical.com>

> ---
>   drivers/irqchip/irq-gic-v3-its.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 5cc27ad2abd1..67386413b907 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2407,7 +2407,7 @@ static int its_irq_domain_activate(struct irq_domain *domain,
>   	cpu = cpumask_first_and(cpu_mask, cpu_online_mask);
>   	if (cpu >= nr_cpu_ids) {
>   		if (its_dev->its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144)
> -			return -EINVAL;
> +			return 0;
>   
>   		cpu = cpumask_first(cpu_online_mask);
>   	}
>
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 5cc27ad2abd1..67386413b907 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2407,7 +2407,7 @@  static int its_irq_domain_activate(struct irq_domain *domain,
 	cpu = cpumask_first_and(cpu_mask, cpu_online_mask);
 	if (cpu >= nr_cpu_ids) {
 		if (its_dev->its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144)
-			return -EINVAL;
+			return 0;
 
 		cpu = cpumask_first(cpu_online_mask);
 	}