diff mbox

[v29,4/9] arm64: kdump: implement machine_crash_shutdown()

Message ID 20170112042143.GF20972@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

AKASHI Takahiro Jan. 12, 2017, 4:21 a.m. UTC
On Wed, Jan 11, 2017 at 10:54:05AM +0000, Will Deacon wrote:
> On Wed, Jan 11, 2017 at 03:36:28PM +0900, AKASHI Takahiro wrote:
> > On Tue, Jan 10, 2017 at 11:32:48AM +0000, Will Deacon wrote:
> > > On Wed, Dec 28, 2016 at 01:36:01PM +0900, AKASHI Takahiro wrote:
> > > > @@ -22,6 +25,7 @@
> > > >  extern const unsigned char arm64_relocate_new_kernel[];
> > > >  extern const unsigned long arm64_relocate_new_kernel_size;
> > > >  
> > > > +static bool in_crash_kexec;
> > > 
> > > Do you actually need this bool? Why not call kexec_crash_loaded() instead?
> > 
> > The two have different meanings:
> > "in_crash_kexec" indicates that kdump is taking place, while
> > kexec_crash_loaded() tells us only whether crash dump kernel has been
> > loaded or not.
> > 
> > It is crucial to distinguish them especially for machine_kexec()
> > which can be called on normal kexec even if kdump has been set up.
> 
> Ah, I see. So how about just doing:
> 
>   if (kimage == kexec_crash_image)
> 
> in machine_kexec?

Yeah, it should work.
Do you want to merge the following hunk,
or expect that I will re-send the whole patch series
(with other changes if any)?

-Takahiro AkASHI

> Will

Comments

Will Deacon Jan. 12, 2017, 12:01 p.m. UTC | #1
On Thu, Jan 12, 2017 at 01:21:44PM +0900, AKASHI Takahiro wrote:
> On Wed, Jan 11, 2017 at 10:54:05AM +0000, Will Deacon wrote:
> > On Wed, Jan 11, 2017 at 03:36:28PM +0900, AKASHI Takahiro wrote:
> > > On Tue, Jan 10, 2017 at 11:32:48AM +0000, Will Deacon wrote:
> > > > On Wed, Dec 28, 2016 at 01:36:01PM +0900, AKASHI Takahiro wrote:
> > > > > @@ -22,6 +25,7 @@
> > > > >  extern const unsigned char arm64_relocate_new_kernel[];
> > > > >  extern const unsigned long arm64_relocate_new_kernel_size;
> > > > >  
> > > > > +static bool in_crash_kexec;
> > > > 
> > > > Do you actually need this bool? Why not call kexec_crash_loaded() instead?
> > > 
> > > The two have different meanings:
> > > "in_crash_kexec" indicates that kdump is taking place, while
> > > kexec_crash_loaded() tells us only whether crash dump kernel has been
> > > loaded or not.
> > > 
> > > It is crucial to distinguish them especially for machine_kexec()
> > > which can be called on normal kexec even if kdump has been set up.
> > 
> > Ah, I see. So how about just doing:
> > 
> >   if (kimage == kexec_crash_image)
> > 
> > in machine_kexec?
> 
> Yeah, it should work.
> Do you want to merge the following hunk,
> or expect that I will re-send the whole patch series
> (with other changes if any)?

Thanks, I'll fold it in and shout if I run into any problems. My plan is
to queue this for 4.11.

Will
Will Deacon Jan. 23, 2017, 5:46 p.m. UTC | #2
On Thu, Jan 12, 2017 at 12:01:11PM +0000, Will Deacon wrote:
> On Thu, Jan 12, 2017 at 01:21:44PM +0900, AKASHI Takahiro wrote:
> > On Wed, Jan 11, 2017 at 10:54:05AM +0000, Will Deacon wrote:
> > > On Wed, Jan 11, 2017 at 03:36:28PM +0900, AKASHI Takahiro wrote:
> > > > On Tue, Jan 10, 2017 at 11:32:48AM +0000, Will Deacon wrote:
> > > > > On Wed, Dec 28, 2016 at 01:36:01PM +0900, AKASHI Takahiro wrote:
> > > > > > @@ -22,6 +25,7 @@
> > > > > >  extern const unsigned char arm64_relocate_new_kernel[];
> > > > > >  extern const unsigned long arm64_relocate_new_kernel_size;
> > > > > >  
> > > > > > +static bool in_crash_kexec;
> > > > > 
> > > > > Do you actually need this bool? Why not call kexec_crash_loaded() instead?
> > > > 
> > > > The two have different meanings:
> > > > "in_crash_kexec" indicates that kdump is taking place, while
> > > > kexec_crash_loaded() tells us only whether crash dump kernel has been
> > > > loaded or not.
> > > > 
> > > > It is crucial to distinguish them especially for machine_kexec()
> > > > which can be called on normal kexec even if kdump has been set up.
> > > 
> > > Ah, I see. So how about just doing:
> > > 
> > >   if (kimage == kexec_crash_image)
> > > 
> > > in machine_kexec?
> > 
> > Yeah, it should work.
> > Do you want to merge the following hunk,
> > or expect that I will re-send the whole patch series
> > (with other changes if any)?
> 
> Thanks, I'll fold it in and shout if I run into any problems. My plan is
> to queue this for 4.11.

Given the DT discussion with Mark, I assume you'll post a new version with
this rolled in.

Will
AKASHI Takahiro Jan. 24, 2017, 7:52 a.m. UTC | #3
Will,

On Mon, Jan 23, 2017 at 05:46:34PM +0000, Will Deacon wrote:
> On Thu, Jan 12, 2017 at 12:01:11PM +0000, Will Deacon wrote:
> > On Thu, Jan 12, 2017 at 01:21:44PM +0900, AKASHI Takahiro wrote:
> > > On Wed, Jan 11, 2017 at 10:54:05AM +0000, Will Deacon wrote:
> > > > On Wed, Jan 11, 2017 at 03:36:28PM +0900, AKASHI Takahiro wrote:
> > > > > On Tue, Jan 10, 2017 at 11:32:48AM +0000, Will Deacon wrote:
> > > > > > On Wed, Dec 28, 2016 at 01:36:01PM +0900, AKASHI Takahiro wrote:
> > > > > > > @@ -22,6 +25,7 @@
> > > > > > >  extern const unsigned char arm64_relocate_new_kernel[];
> > > > > > >  extern const unsigned long arm64_relocate_new_kernel_size;
> > > > > > >  
> > > > > > > +static bool in_crash_kexec;
> > > > > > 
> > > > > > Do you actually need this bool? Why not call kexec_crash_loaded() instead?
> > > > > 
> > > > > The two have different meanings:
> > > > > "in_crash_kexec" indicates that kdump is taking place, while
> > > > > kexec_crash_loaded() tells us only whether crash dump kernel has been
> > > > > loaded or not.
> > > > > 
> > > > > It is crucial to distinguish them especially for machine_kexec()
> > > > > which can be called on normal kexec even if kdump has been set up.
> > > > 
> > > > Ah, I see. So how about just doing:
> > > > 
> > > >   if (kimage == kexec_crash_image)
> > > > 
> > > > in machine_kexec?
> > > 
> > > Yeah, it should work.
> > > Do you want to merge the following hunk,
> > > or expect that I will re-send the whole patch series
> > > (with other changes if any)?
> > 
> > Thanks, I'll fold it in and shout if I run into any problems. My plan is
> > to queue this for 4.11.
> 
> Given the DT discussion with Mark, I assume you'll post a new version with
> this rolled in.

Yes, I will!

-Takahiro AKASHI

> Will
diff mbox

Patch

===8<==
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 994fe0bc5cc0..c0fc3d458195 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -26,7 +26,6 @@ 
 extern const unsigned char arm64_relocate_new_kernel[];
 extern const unsigned long arm64_relocate_new_kernel_size;
 
-static bool in_crash_kexec;
 static unsigned long kimage_start;
 
 /**
@@ -154,7 +153,7 @@  void machine_kexec(struct kimage *kimage)
 	 * New cpus may have become stuck_in_kernel after we loaded the image.
 	 */
 	BUG_ON((cpus_are_stuck_in_kernel() || (num_online_cpus() > 1)) &&
-			!WARN_ON(in_crash_kexec));
+			!WARN_ON(kimage == kexec_crash_image));
 
 	reboot_code_buffer_phys = page_to_phys(kimage->control_code_page);
 	reboot_code_buffer = phys_to_virt(reboot_code_buffer_phys);
@@ -206,8 +205,8 @@  void machine_kexec(struct kimage *kimage)
 	 * relocation is complete.
 	 */
 
-	cpu_soft_restart(!in_crash_kexec, reboot_code_buffer_phys, kimage->head,
-		kimage_start, 0);
+	cpu_soft_restart(kimage != kexec_crash_image,
+		reboot_code_buffer_phys, kimage->head, kimage_start, 0);
 
 	BUG(); /* Should never get here. */
 }
@@ -250,8 +249,6 @@  void machine_crash_shutdown(struct pt_regs *regs)
 {
 	local_irq_disable();
 
-	in_crash_kexec = true;
-
 	/* shutdown non-crashing cpus */
 	smp_send_crash_stop();