Date:   Tue, 21 Mar 2023 10:30:17 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Eric DeVolder <eric.devolder@oracle.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, vgoyal@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, nramas@linux.microsoft.com, thomas.lendacky@amd.com,
        robh@kernel.org, efault@gmx.de, rppt@kernel.org, david@redhat.com,
        sourabhjain@linux.ibm.com, konrad.wilk@oracle.com,
        boris.ostrovsky@oracle.com
Subject: Re: [PATCH v20 2/8] crash: add generic infrastructure for crash
 hotplug support
Message-ID: <ZBkWub0rTMp2+kxd@MiWiFi-R3L-srv>
References: <20230317212128.21424-1-eric.devolder@oracle.com>
 <20230317212128.21424-3-eric.devolder@oracle.com>
 <ZBgVjjZdiEA5YxQX@MiWiFi-R3L-srv>
 <12fa9836-4ddd-2961-7d32-4754506c84ae@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <12fa9836-4ddd-2961-7d32-4754506c84ae@oracle.com>
Precedence: bulk

On 03/20/23 at 10:06am, Eric DeVolder wrote:
> 
> 
> On 3/20/23 03:13, Baoquan He wrote:
> > On 03/17/23 at 05:21pm, Eric DeVolder wrote:
> > ......
> > > @@ -697,3 +700,137 @@ static int __init crash_save_vmcoreinfo_init(void)
> > >   }
> > >   subsys_initcall(crash_save_vmcoreinfo_init);
> > > +
> > > +#ifdef CONFIG_CRASH_HOTPLUG
> > > +#undef pr_fmt
> > > +#define pr_fmt(fmt) "crash hp: " fmt
> > > +/*
> > > + * To accurately reflect hot un/plug changes of cpu and memory resources
> > > + * (including onling and offlining of those resources), the elfcorehdr
> > > + * (which is passed to the crash kernel via the elfcorehdr= parameter)
> > > + * must be updated with the new list of CPUs and memories.
> > > + *
> > > + * In order to make changes to elfcorehdr, two conditions are needed:
> > > + * First, the segment containing the elfcorehdr must be large enough
> > > + * to permit a growing number of resources; the elfcorehdr memory size
> > > + * is based on NR_CPUS_DEFAULT and CRASH_MAX_MEMORY_RANGES.
> > > + * Second, purgatory must explicitly exclude the elfcorehdr from the
> > > + * list of segments it checks (since the elfcorehdr changes and thus
> > > + * would require an update to purgatory itself to update the digest).
> > > + */
> > > +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> > > +{
> > > +	/* Obtain lock while changing crash information */
> > > +	if (!kexec_trylock())
> > > +		return;
> > > +
> > > +	/* Check kdump is loaded */
> > > +	if (kexec_crash_image) {
> > 
> > Here, what I mean is:
> > 
> > 	/* Obtain lock while changing crash information */
> > 	if (!kexec_trylock())
> > 		return;
> > 
> > 	/*If kdump is not loaded*/
> > 	if (!kexec_crash_image)
> > 		goto out;	
> > 
> > Then we reduce one tab of indentation for the following code block, e.g
> > the for loop block will have smaller pressure on breaking the 80 chars
> > limitation.
> > 
> 
> Ah, yes, ok. I'll make that change. Do you prefer I post that soon, or give this v20 some more time?

Yeah, giving v20 a while for reviewing sounds great. After all this
is only programming style issue. Other than this, the overall looks good
to me, let's wait and see what other people's concern, esp x86
maintainer's opinion.