| Message ID | 1390802904-28399-2-git-send-email-nicolas.pitre@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 01/27/2014 07:08 AM, Nicolas Pitre wrote: > ARM and ARM64 are the only two architectures implementing > arch_cpu_idle_prepare() simply to call local_fiq_enable(). > > We have secondary_start_kernel() already calling local_fiq_enable() and > this is done a second time in arch_cpu_idle_prepare() in that case. And > enabling FIQs has nothing to do with idling the CPU to start with. > > So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot > CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier > at late_initcall time but this shouldn't make a difference in practice > i.e. when FIQs are actually used. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm/kernel/process.c | 5 ----- > arch/arm/kernel/setup.c | 7 +++++++ > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > index 92f7b15dd2..725b8c95e0 100644 > --- a/arch/arm/kernel/process.c > +++ b/arch/arm/kernel/process.c > @@ -142,11 +142,6 @@ static void default_idle(void) > local_irq_enable(); > } > > -void arch_cpu_idle_prepare(void) > -{ > - local_fiq_enable(); > -} > - > void arch_cpu_idle_enter(void) > { > ledtrig_cpu(CPU_LED_IDLE_START); > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index 987a7f5bce..d027b1a6fe 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -789,6 +789,13 @@ static int __init init_machine_late(void) > } > late_initcall(init_machine_late); > > +static int __init init_fiq_boot_cpu(void) > +{ > + local_fiq_enable(); > + return 0; > +} > +late_initcall(init_fiq_boot_cpu); > + > #ifdef CONFIG_KEXEC > static inline unsigned long long get_total_mem(void) > { >
On Mon, Jan 27, 2014 at 01:08:16AM -0500, Nicolas Pitre wrote: > ARM and ARM64 are the only two architectures implementing > arch_cpu_idle_prepare() simply to call local_fiq_enable(). > > We have secondary_start_kernel() already calling local_fiq_enable() and > this is done a second time in arch_cpu_idle_prepare() in that case. And > enabling FIQs has nothing to do with idling the CPU to start with. > > So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot > CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier > at late_initcall time but this shouldn't make a difference in practice > i.e. when FIQs are actually used. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/kernel/process.c | 5 ----- > arch/arm/kernel/setup.c | 7 +++++++ > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > index 92f7b15dd2..725b8c95e0 100644 > --- a/arch/arm/kernel/process.c > +++ b/arch/arm/kernel/process.c > @@ -142,11 +142,6 @@ static void default_idle(void) > local_irq_enable(); > } > > -void arch_cpu_idle_prepare(void) > -{ > - local_fiq_enable(); > -} > - > void arch_cpu_idle_enter(void) > { > ledtrig_cpu(CPU_LED_IDLE_START); > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index 987a7f5bce..d027b1a6fe 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -789,6 +789,13 @@ static int __init init_machine_late(void) > } > late_initcall(init_machine_late); > > +static int __init init_fiq_boot_cpu(void) > +{ > + local_fiq_enable(); > + return 0; > +} > +late_initcall(init_fiq_boot_cpu); arch_cpu_idle_prepare() gets called from the swapper thread, and changes the swapper thread's CPSR. init_fiq_boot_cpu() gets called from PID1, the init thread, and changes the init thread's CPSR, which will already have FIQs enabled by way of how kernel threads are created. Hence, the above code fragment has no effect what so ever, and those platforms using FIQs will not have FIQs delivered if they're idle (because the swapper will have FIQs masked at the CPU.) NAK.
On Mon, 27 Jan 2014, Russell King - ARM Linux wrote: > On Mon, Jan 27, 2014 at 01:08:16AM -0500, Nicolas Pitre wrote: > > ARM and ARM64 are the only two architectures implementing > > arch_cpu_idle_prepare() simply to call local_fiq_enable(). > > > > We have secondary_start_kernel() already calling local_fiq_enable() and > > this is done a second time in arch_cpu_idle_prepare() in that case. And > > enabling FIQs has nothing to do with idling the CPU to start with. > > > > So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot > > CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier > > at late_initcall time but this shouldn't make a difference in practice > > i.e. when FIQs are actually used. > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > --- > > arch/arm/kernel/process.c | 5 ----- > > arch/arm/kernel/setup.c | 7 +++++++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > > index 92f7b15dd2..725b8c95e0 100644 > > --- a/arch/arm/kernel/process.c > > +++ b/arch/arm/kernel/process.c > > @@ -142,11 +142,6 @@ static void default_idle(void) > > local_irq_enable(); > > } > > > > -void arch_cpu_idle_prepare(void) > > -{ > > - local_fiq_enable(); > > -} > > - > > void arch_cpu_idle_enter(void) > > { > > ledtrig_cpu(CPU_LED_IDLE_START); > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > > index 987a7f5bce..d027b1a6fe 100644 > > --- a/arch/arm/kernel/setup.c > > +++ b/arch/arm/kernel/setup.c > > @@ -789,6 +789,13 @@ static int __init init_machine_late(void) > > } > > late_initcall(init_machine_late); > > > > +static int __init init_fiq_boot_cpu(void) > > +{ > > + local_fiq_enable(); > > + return 0; > > +} > > +late_initcall(init_fiq_boot_cpu); > > arch_cpu_idle_prepare() gets called from the swapper thread, and changes > the swapper thread's CPSR. init_fiq_boot_cpu() gets called from PID1, the > init thread, and changes the init thread's CPSR, which will already have > FIQs enabled by way of how kernel threads are created. > > Hence, the above code fragment has no effect what so ever, and those > platforms using FIQs will not have FIQs delivered if they're idle > (because the swapper will have FIQs masked at the CPU.) You're right. What about moving local_fiq_enable() to trap_init() then? Nicolas
On Mon, Jan 27, 2014 at 10:45:59AM -0500, Nicolas Pitre wrote: > On Mon, 27 Jan 2014, Russell King - ARM Linux wrote: > > > On Mon, Jan 27, 2014 at 01:08:16AM -0500, Nicolas Pitre wrote: > > > ARM and ARM64 are the only two architectures implementing > > > arch_cpu_idle_prepare() simply to call local_fiq_enable(). > > > > > > We have secondary_start_kernel() already calling local_fiq_enable() and > > > this is done a second time in arch_cpu_idle_prepare() in that case. And > > > enabling FIQs has nothing to do with idling the CPU to start with. > > > > > > So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot > > > CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier > > > at late_initcall time but this shouldn't make a difference in practice > > > i.e. when FIQs are actually used. > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > --- > > > arch/arm/kernel/process.c | 5 ----- > > > arch/arm/kernel/setup.c | 7 +++++++ > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > > > index 92f7b15dd2..725b8c95e0 100644 > > > --- a/arch/arm/kernel/process.c > > > +++ b/arch/arm/kernel/process.c > > > @@ -142,11 +142,6 @@ static void default_idle(void) > > > local_irq_enable(); > > > } > > > > > > -void arch_cpu_idle_prepare(void) > > > -{ > > > - local_fiq_enable(); > > > -} > > > - > > > void arch_cpu_idle_enter(void) > > > { > > > ledtrig_cpu(CPU_LED_IDLE_START); > > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > > > index 987a7f5bce..d027b1a6fe 100644 > > > --- a/arch/arm/kernel/setup.c > > > +++ b/arch/arm/kernel/setup.c > > > @@ -789,6 +789,13 @@ static int __init init_machine_late(void) > > > } > > > late_initcall(init_machine_late); > > > > > > +static int __init init_fiq_boot_cpu(void) > > > +{ > > > + local_fiq_enable(); > > > + return 0; > > > +} > > > +late_initcall(init_fiq_boot_cpu); > > > > arch_cpu_idle_prepare() gets called from the swapper thread, and changes > > the swapper thread's CPSR. init_fiq_boot_cpu() gets called from PID1, the > > init thread, and changes the init thread's CPSR, which will already have > > FIQs enabled by way of how kernel threads are created. > > > > Hence, the above code fragment has no effect what so ever, and those > > platforms using FIQs will not have FIQs delivered if they're idle > > (because the swapper will have FIQs masked at the CPU.) > > You're right. > > What about moving local_fiq_enable() to trap_init() then? That's potentially unsafe, as we haven't touched any of the IRQ controllers at that point - we can't guarantee what state they'd be in. Given that the default FIQ is to just return, a FIQ being raised at that point will end up with an infinite loop re-entering the FIQ handler.
On Mon, Jan 27, 2014 at 09:22:55AM +0100, Daniel Lezcano wrote: > On 01/27/2014 07:08 AM, Nicolas Pitre wrote: >> ARM and ARM64 are the only two architectures implementing >> arch_cpu_idle_prepare() simply to call local_fiq_enable(). >> >> We have secondary_start_kernel() already calling local_fiq_enable() and >> this is done a second time in arch_cpu_idle_prepare() in that case. And >> enabling FIQs has nothing to do with idling the CPU to start with. >> >> So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot >> CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier >> at late_initcall time but this shouldn't make a difference in practice >> i.e. when FIQs are actually used. >> >> Signed-off-by: Nicolas Pitre <nico@linaro.org> > > Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> What kind of review did you do when giving that attributation?
On 01/27/2014 05:07 PM, Russell King - ARM Linux wrote: > On Mon, Jan 27, 2014 at 09:22:55AM +0100, Daniel Lezcano wrote: >> On 01/27/2014 07:08 AM, Nicolas Pitre wrote: >>> ARM and ARM64 are the only two architectures implementing >>> arch_cpu_idle_prepare() simply to call local_fiq_enable(). >>> >>> We have secondary_start_kernel() already calling local_fiq_enable() and >>> this is done a second time in arch_cpu_idle_prepare() in that case. And >>> enabling FIQs has nothing to do with idling the CPU to start with. >>> >>> So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot >>> CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier >>> at late_initcall time but this shouldn't make a difference in practice >>> i.e. when FIQs are actually used. >>> >>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >> >> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > What kind of review did you do when giving that attributation? I did the review to the best of my knowledge and with good will. I read your comment on this patch and I learnt one more thing. Today, I am smarter than yesterday and dumber than tomorrow :) -- Daniel
On Mon, Jan 27, 2014 at 06:12:53PM +0100, Daniel Lezcano wrote: > On 01/27/2014 05:07 PM, Russell King - ARM Linux wrote: >> On Mon, Jan 27, 2014 at 09:22:55AM +0100, Daniel Lezcano wrote: >>> On 01/27/2014 07:08 AM, Nicolas Pitre wrote: >>>> ARM and ARM64 are the only two architectures implementing >>>> arch_cpu_idle_prepare() simply to call local_fiq_enable(). >>>> >>>> We have secondary_start_kernel() already calling local_fiq_enable() and >>>> this is done a second time in arch_cpu_idle_prepare() in that case. And >>>> enabling FIQs has nothing to do with idling the CPU to start with. >>>> >>>> So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot >>>> CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier >>>> at late_initcall time but this shouldn't make a difference in practice >>>> i.e. when FIQs are actually used. >>>> >>>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>> >>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> What kind of review did you do when giving that attributation? > > I did the review to the best of my knowledge and with good will. > > I read your comment on this patch and I learnt one more thing. > > Today, I am smarter than yesterday and dumber than tomorrow :) Just be aware that putting a comment along with the reviewed-by tag is always a good idea. I know that's a little more work, but this has been raised a number of times by various people over the years. A reviewed-by tag on its own doesn't mean much, as it could mean that you've just glanced over the code and decided "yea, it looks okay", or it could mean that you've spent all day verifying that the code change is indeed correct. Consequently, some will ignore emails which just contain a reviewed-by attributation.
On 01/27/2014 06:21 PM, Russell King - ARM Linux wrote: > On Mon, Jan 27, 2014 at 06:12:53PM +0100, Daniel Lezcano wrote: >> On 01/27/2014 05:07 PM, Russell King - ARM Linux wrote: >>> On Mon, Jan 27, 2014 at 09:22:55AM +0100, Daniel Lezcano wrote: >>>> On 01/27/2014 07:08 AM, Nicolas Pitre wrote: >>>>> ARM and ARM64 are the only two architectures implementing >>>>> arch_cpu_idle_prepare() simply to call local_fiq_enable(). >>>>> >>>>> We have secondary_start_kernel() already calling local_fiq_enable() and >>>>> this is done a second time in arch_cpu_idle_prepare() in that case. And >>>>> enabling FIQs has nothing to do with idling the CPU to start with. >>>>> >>>>> So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot >>>>> CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier >>>>> at late_initcall time but this shouldn't make a difference in practice >>>>> i.e. when FIQs are actually used. >>>>> >>>>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>>> >>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> What kind of review did you do when giving that attributation? >> >> I did the review to the best of my knowledge and with good will. >> >> I read your comment on this patch and I learnt one more thing. >> >> Today, I am smarter than yesterday and dumber than tomorrow :) > > Just be aware that putting a comment along with the reviewed-by tag > is always a good idea. I know that's a little more work, but this has > been raised a number of times by various people over the years. > > A reviewed-by tag on its own doesn't mean much, as it could mean that > you've just glanced over the code and decided "yea, it looks okay", or > it could mean that you've spent all day verifying that the code change > is indeed correct. > > Consequently, some will ignore emails which just contain a reviewed-by > attributation. Thanks for the clarification. I will take care of giving a comment next time. -- Daniel
On Mon, Jan 27, 2014 at 05:21:10PM +0000, Russell King - ARM Linux wrote: > A reviewed-by tag on its own doesn't mean much, as it could mean that > you've just glanced over the code and decided "yea, it looks okay", or > it could mean that you've spent all day verifying that the code change > is indeed correct. One should use Acked-by for the 'yea, it looks okay' thing.
On Mon, 27 Jan 2014, Russell King - ARM Linux wrote: > On Mon, Jan 27, 2014 at 10:45:59AM -0500, Nicolas Pitre wrote: > > On Mon, 27 Jan 2014, Russell King - ARM Linux wrote: > > > > > On Mon, Jan 27, 2014 at 01:08:16AM -0500, Nicolas Pitre wrote: > > > > ARM and ARM64 are the only two architectures implementing > > > > arch_cpu_idle_prepare() simply to call local_fiq_enable(). > > > > > > > > We have secondary_start_kernel() already calling local_fiq_enable() and > > > > this is done a second time in arch_cpu_idle_prepare() in that case. And > > > > enabling FIQs has nothing to do with idling the CPU to start with. > > > > > > > > So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot > > > > CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier > > > > at late_initcall time but this shouldn't make a difference in practice > > > > i.e. when FIQs are actually used. > > > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > > --- > > > > arch/arm/kernel/process.c | 5 ----- > > > > arch/arm/kernel/setup.c | 7 +++++++ > > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > > > > index 92f7b15dd2..725b8c95e0 100644 > > > > --- a/arch/arm/kernel/process.c > > > > +++ b/arch/arm/kernel/process.c > > > > @@ -142,11 +142,6 @@ static void default_idle(void) > > > > local_irq_enable(); > > > > } > > > > > > > > -void arch_cpu_idle_prepare(void) > > > > -{ > > > > - local_fiq_enable(); > > > > -} > > > > - > > > > void arch_cpu_idle_enter(void) > > > > { > > > > ledtrig_cpu(CPU_LED_IDLE_START); > > > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > > > > index 987a7f5bce..d027b1a6fe 100644 > > > > --- a/arch/arm/kernel/setup.c > > > > +++ b/arch/arm/kernel/setup.c > > > > @@ -789,6 +789,13 @@ static int __init init_machine_late(void) > > > > } > > > > late_initcall(init_machine_late); > > > > > > > > +static int __init init_fiq_boot_cpu(void) > > > > +{ > > > > + local_fiq_enable(); > > > > + return 0; > > > > +} > > > > +late_initcall(init_fiq_boot_cpu); > > > > > > arch_cpu_idle_prepare() gets called from the swapper thread, and changes > > > the swapper thread's CPSR. init_fiq_boot_cpu() gets called from PID1, the > > > init thread, and changes the init thread's CPSR, which will already have > > > FIQs enabled by way of how kernel threads are created. > > > > > > Hence, the above code fragment has no effect what so ever, and those > > > platforms using FIQs will not have FIQs delivered if they're idle > > > (because the swapper will have FIQs masked at the CPU.) > > > > You're right. > > > > What about moving local_fiq_enable() to trap_init() then? > > That's potentially unsafe, as we haven't touched any of the IRQ > controllers at that point - we can't guarantee what state they'd be > in. Given that the default FIQ is to just return, a FIQ being raised > at that point will end up with an infinite loop re-entering the FIQ > handler. Okay... I don't see any obvious way to work around that besides adding another explicit hook, which arch_cpu_idle_prepare() incidentally already is. So, unless you have a better idea, I'll drop this patch and leave ARM as the only user of arch_cpu_idle_prepare(). Nicolas
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index 92f7b15dd2..725b8c95e0 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -142,11 +142,6 @@ static void default_idle(void) local_irq_enable(); } -void arch_cpu_idle_prepare(void) -{ - local_fiq_enable(); -} - void arch_cpu_idle_enter(void) { ledtrig_cpu(CPU_LED_IDLE_START); diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 987a7f5bce..d027b1a6fe 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -789,6 +789,13 @@ static int __init init_machine_late(void) } late_initcall(init_machine_late); +static int __init init_fiq_boot_cpu(void) +{ + local_fiq_enable(); + return 0; +} +late_initcall(init_fiq_boot_cpu); + #ifdef CONFIG_KEXEC static inline unsigned long long get_total_mem(void) {
ARM and ARM64 are the only two architectures implementing arch_cpu_idle_prepare() simply to call local_fiq_enable(). We have secondary_start_kernel() already calling local_fiq_enable() and this is done a second time in arch_cpu_idle_prepare() in that case. And enabling FIQs has nothing to do with idling the CPU to start with. So let's introduce init_fiq_boot_cpu() to take care of FIQs on the boot CPU and remove arch_cpu_idle_prepare(). This is now done a bit earlier at late_initcall time but this shouldn't make a difference in practice i.e. when FIQs are actually used. Signed-off-by: Nicolas Pitre <nico@linaro.org> --- arch/arm/kernel/process.c | 5 ----- arch/arm/kernel/setup.c | 7 +++++++ 2 files changed, 7 insertions(+), 5 deletions(-)