[PATCH] irqchip/i8259: fix shutdown order by moving syscore_ops registration

[PATCH] irqchip/i8259: fix shutdown order by moving syscore_ops registration

[Aaro Koskinen]

When using cpufreq on Loongson 2F MIPS platform, "poweroff"
command gets frequently stuck in syscore_shutdown(). The reason is
that i8259A_shutdown() gets called before cpufreq_suspend(), and if we
have pending work then irq_work_sync() in cpufreq_dbs_governor_stop()
gets stuck forever as we have all interrupts masked already.

irq-i8259 is registering syscore_ops using device_initcall(),
while cpufreq uses core_initcall(). Fix the shutdown order simply
by registering the irq syscore_ops during the early IRQ init instead
of using a separate initcall at later stage.

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
 drivers/irqchip/irq-i8259.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-i8259.c b/drivers/irqchip/irq-i8259.c
index b0d4aab1a58c..d000870d9b6b 100644
--- a/drivers/irqchip/irq-i8259.c
+++ b/drivers/irqchip/irq-i8259.c
@@ -225,14 +225,6 @@ static struct syscore_ops i8259_syscore_ops = {
 	.shutdown = i8259A_shutdown,
 };
 
-static int __init i8259A_init_sysfs(void)
-{
-	register_syscore_ops(&i8259_syscore_ops);
-	return 0;
-}
-
-device_initcall(i8259A_init_sysfs);
-
 static void init_8259A(int auto_eoi)
 {
 	unsigned long flags;
@@ -332,6 +324,7 @@ struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
 		panic("Failed to add i8259 IRQ domain");
 
 	setup_irq(I8259A_IRQ_BASE + PIC_CASCADE_IR, &irq2);
+	register_syscore_ops(&i8259_syscore_ops);
 	return domain;
 }
 
-- 
2.17.0

Re: [PATCH] irqchip/i8259: fix shutdown order by moving syscore_ops registration

[Marc Zyngier]

[Adding the MIPS folks]

On 06/02/2019 21:26, Aaro Koskinen wrote:
> When using cpufreq on Loongson 2F MIPS platform, "poweroff"
> command gets frequently stuck in syscore_shutdown(). The reason is
> that i8259A_shutdown() gets called before cpufreq_suspend(), and if we
> have pending work then irq_work_sync() in cpufreq_dbs_governor_stop()
> gets stuck forever as we have all interrupts masked already.
> 
> irq-i8259 is registering syscore_ops using device_initcall(),
> while cpufreq uses core_initcall(). Fix the shutdown order simply
> by registering the irq syscore_ops during the early IRQ init instead
> of using a separate initcall at later stage.
> 
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> ---
>  drivers/irqchip/irq-i8259.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-i8259.c b/drivers/irqchip/irq-i8259.c
> index b0d4aab1a58c..d000870d9b6b 100644
> --- a/drivers/irqchip/irq-i8259.c
> +++ b/drivers/irqchip/irq-i8259.c
> @@ -225,14 +225,6 @@ static struct syscore_ops i8259_syscore_ops = {
>  	.shutdown = i8259A_shutdown,
>  };
>  
> -static int __init i8259A_init_sysfs(void)
> -{
> -	register_syscore_ops(&i8259_syscore_ops);
> -	return 0;
> -}
> -
> -device_initcall(i8259A_init_sysfs);
> -
>  static void init_8259A(int auto_eoi)
>  {
>  	unsigned long flags;
> @@ -332,6 +324,7 @@ struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
>  		panic("Failed to add i8259 IRQ domain");
>  
>  	setup_irq(I8259A_IRQ_BASE + PIC_CASCADE_IR, &irq2);
> +	register_syscore_ops(&i8259_syscore_ops);
>  	return domain;
>  }
>  
> 

Given that this is a change of behaviour that is likely to affect other
platforms (I see at least another 6 MIPS machines using the i8259),
could someone make sure that this doesn't cause any regression? This is
unlikely to affect the SGI boxes, as they predate any notion of power
management, but something like Malta could potentially be affected.

Please let me know.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqchip/i8259: fix shutdown order by moving syscore_ops registration

[Aaro Koskinen]

Hi,

On Thu, Feb 07, 2019 at 08:56:37AM +0000, Marc Zyngier wrote:
> On 06/02/2019 21:26, Aaro Koskinen wrote:
> >  static void init_8259A(int auto_eoi)
> >  {
> >  	unsigned long flags;
> > @@ -332,6 +324,7 @@ struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
> >  		panic("Failed to add i8259 IRQ domain");
> >  
> >  	setup_irq(I8259A_IRQ_BASE + PIC_CASCADE_IR, &irq2);
> > +	register_syscore_ops(&i8259_syscore_ops);
> >  	return domain;
> >  }
> >  
> > 
> 
> Given that this is a change of behaviour that is likely to affect other
> platforms (I see at least another 6 MIPS machines using the i8259),
> could someone make sure that this doesn't cause any regression? This is
> unlikely to affect the SGI boxes, as they predate any notion of power
> management, but something like Malta could potentially be affected.

For shutdown, I don't think there are many syscore_ops users on these
platforms. Actually I could find only two that I think could be used:
	- cpufreq (issue fixed by this patch, and Loongson is the only user
	  anyway)
	- leds-trigger

Then suspend/resume: i8259 doesn't implement suspend, so there is no
change in behaviour. In resume it does PIC re-init, but syscore_resume()
is done with interrupts disabled so the order shouldn't matter.

A.

Re: [PATCH] irqchip/i8259: fix shutdown order by moving syscore_ops registration

[Marc Zyngier]

On Thu, 07 Feb 2019 20:58:12 +0000,
Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> 
> Hi,
> 
> On Thu, Feb 07, 2019 at 08:56:37AM +0000, Marc Zyngier wrote:
> > On 06/02/2019 21:26, Aaro Koskinen wrote:
> > >  static void init_8259A(int auto_eoi)
> > >  {
> > >  	unsigned long flags;
> > > @@ -332,6 +324,7 @@ struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
> > >  		panic("Failed to add i8259 IRQ domain");
> > >  
> > >  	setup_irq(I8259A_IRQ_BASE + PIC_CASCADE_IR, &irq2);
> > > +	register_syscore_ops(&i8259_syscore_ops);
> > >  	return domain;
> > >  }
> > >  
> > > 
> > 
> > Given that this is a change of behaviour that is likely to affect other
> > platforms (I see at least another 6 MIPS machines using the i8259),
> > could someone make sure that this doesn't cause any regression? This is
> > unlikely to affect the SGI boxes, as they predate any notion of power
> > management, but something like Malta could potentially be affected.
> 
> For shutdown, I don't think there are many syscore_ops users on these
> platforms. Actually I could find only two that I think could be used:
> 	- cpufreq (issue fixed by this patch, and Loongson is the only user
> 	  anyway)
> 	- leds-trigger
> 
> Then suspend/resume: i8259 doesn't implement suspend, so there is no
> change in behaviour. In resume it does PIC re-init, but syscore_resume()
> is done with interrupts disabled so the order shouldn't matter.

In the absence of any comment from the MIPS guys over the past week,
I've queued this. Please let me know should it break anything.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] irqchip/i8259: fix shutdown order by moving syscore_ops registration

[Paul Burton]

Hi Marc,

On Thu, Feb 14, 2019 at 10:43:06AM +0000, Marc Zyngier wrote:
> On Thu, 07 Feb 2019 20:58:12 +0000,
> Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> > On Thu, Feb 07, 2019 at 08:56:37AM +0000, Marc Zyngier wrote:
> > > On 06/02/2019 21:26, Aaro Koskinen wrote:
> > > >  static void init_8259A(int auto_eoi)
> > > >  {
> > > >  	unsigned long flags;
> > > > @@ -332,6 +324,7 @@ struct irq_domain * __init __init_i8259_irqs(struct device_node *node)
> > > >  		panic("Failed to add i8259 IRQ domain");
> > > >  
> > > >  	setup_irq(I8259A_IRQ_BASE + PIC_CASCADE_IR, &irq2);
> > > > +	register_syscore_ops(&i8259_syscore_ops);
> > > >  	return domain;
> > > >  }
> > > >  
> > > > 
> > > 
> > > Given that this is a change of behaviour that is likely to affect other
> > > platforms (I see at least another 6 MIPS machines using the i8259),
> > > could someone make sure that this doesn't cause any regression? This is
> > > unlikely to affect the SGI boxes, as they predate any notion of power
> > > management, but something like Malta could potentially be affected.
> > 
> > For shutdown, I don't think there are many syscore_ops users on these
> > platforms. Actually I could find only two that I think could be used:
> > 	- cpufreq (issue fixed by this patch, and Loongson is the only user
> > 	  anyway)
> > 	- leds-trigger
> > 
> > Then suspend/resume: i8259 doesn't implement suspend, so there is no
> > change in behaviour. In resume it does PIC re-init, but syscore_resume()
> > is done with interrupts disabled so the order shouldn't matter.
> 
> In the absence of any comment from the MIPS guys over the past week,
> I've queued this. Please let me know should it break anything.

Apologies for my sluggish response, but for what it's worth the patch
looks reasonable at a glance. Malta is the only other I8259-using
hardware that I technically have access to, but at this point Malta
isn't actively used & it's really seen more as "the board QEMU defaults
to" than a useful real hardware platform.

Thanks,
    Paul