Received-SPF: pass (google.com: domain of linux-kernel+bounces-43922-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 30 Jan 2024 11:58:08 +0800
From: Baoquan He <bhe@redhat.com>
To: Michael Kelley <mhklinux@outlook.com>
Cc: "kexec@lists.infradead.org" <kexec@lists.infradead.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	"nathan@kernel.org" <nathan@kernel.org>
Subject: Re: [PATCH linux-next 1/3] x86, crash: don't nest CONFIG_CRASH_DUMP
 ifdef inside CONFIG_KEXEC_CODE ifdef scope
Message-ID: <Zbhz0D3DgDyTsboe@MiWiFi-R3L-srv>
References: <20240129135033.157195-1-bhe@redhat.com>
 <SN6PR02MB41571397201804BD486C6148D47E2@SN6PR02MB4157.namprd02.prod.outlook.com>
 <ZbhH+sg1BPi+R0j4@MiWiFi-R3L-srv>
 <SN6PR02MB41575A5568875B85C8AA7F4BD47D2@SN6PR02MB4157.namprd02.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SN6PR02MB41575A5568875B85C8AA7F4BD47D2@SN6PR02MB4157.namprd02.prod.outlook.com>

On 01/30/24 at 01:39am, Michael Kelley wrote:
> From: Baoquan He <bhe@redhat.com>
> > 
> > On 01/29/24 at 06:27pm, Michael Kelley wrote:
> > > From: Baoquan He <bhe@redhat.com> Sent: Monday, January 29, 2024
> > 5:51 AM
> > > >
> > > > Michael pointed out that the #ifdef CONFIG_CRASH_DUMP is nested inside
> > > > arch/x86/xen/enlighten_hvm.c.
> > >
> > > Did some words get left out in the above sentence?  It mentions the Xen
> > > case, but not the Hyper-V case.  I'm not sure what you intended.
> > 
> > Thanks a lot for your careful reviewing.
> > 
> > Yeah, I tried to list all affected file names, seems my vim editor threw
> > away some words. And I forgot mentioning the change in reboot.c.
> > 
> > I adjusted log as below according to your comments, do you think it's OK
> > now?
> 
> Yes -- looks like everything is included and clear up my confusion.  But
> I still have two small nits per below. :-)

Right, I will grabbed them into v2. Thanks again.

> 
> > 
> > ===
> > Michael pointed out that the #ifdef CONFIG_CRASH_DUMP is nested inside
> > CONFIG_KEXEC_CODE ifdef scope in some XEN, HyperV codes.
> 
> s/Hyper-V/HyperV/
> 
> > 
> > Although the nesting works well too since CONFIG_CRASH_DUMP has
> > dependency on CONFIG_KEXEC_CORE, it may cause confusion because there
> > are places where it's not nested, and people may think it needs be nested
> 
> s/needs to be/needs be/
> 
> > even though it doesn't have to.
> > 
> > Fix that by moving  CONFIG_CRASH_DUMP ifdeffery of codes out of
> > CONFIG_KEXEC_CODE ifdeffery scope.
> > 
> > And also put function machine_crash_shutdown() definition inside
> > CONFIG_CRASH_DUMP ifdef scope instead of CONFIG_KEXEC_CORE ifdef.
> > 
> > And also fix a building error Nathan reported as below by replacing
> > CONFIG_KEXEC_CORE ifdef with CONFIG_VMCORE_INFO ifdef.
> > ......
> > ===
> > 
> > Thanks
> > Baoquan
>