Received-SPF: pass (google.com: domain of linux-kernel+bounces-35090-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 23 Jan 2024 10:29:28 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
CC: <linux-pm@vger.kernel.org>, <loongarch@lists.linux.dev>,
	<linux-acpi@vger.kernel.org>, <linux-arch@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
	<linux-riscv@lists.infradead.org>, <kvmarm@lists.linux.dev>,
	<x86@kernel.org>, <acpica-devel@lists.linuxfoundation.org>,
	<linux-csky@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-ia64@vger.kernel.org>, <linux-parisc@vger.kernel.org>, Salil Mehta
	<salil.mehta@huawei.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>,
	<jianyong.wu@arm.com>, <justin.he@arm.com>, James Morse <james.morse@arm.com>
Subject: Re: [PATCH RFC v3 18/21] ACPI: processor: Only call
 arch_unregister_cpu() if HOTPLUG_CPU is selected
Message-ID: <20240123102928.0000270c@Huawei.com>
In-Reply-To: <ZYBB32fMWB6of7Jb@shell.armlinux.org.uk>
References: <ZXmn46ptis59F0CO@shell.armlinux.org.uk>
	<E1rDOhH-00DvlO-UP@rmk-PC.armlinux.org.uk>
	<20231215165009.000035f2@Huawei.com>
	<ZYBB32fMWB6of7Jb@shell.armlinux.org.uk>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Mon, 18 Dec 2023 12:58:07 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Fri, Dec 15, 2023 at 04:50:09PM +0000, Jonathan Cameron wrote:
> > On Wed, 13 Dec 2023 12:50:43 +0000
> > Russell King (Oracle) <rmk+kernel@armlinux.org.uk> wrote:
> >   
> > > From: James Morse <james.morse@arm.com>
> > > 
> > > The kbuild robot points out that configurations without HOTPLUG_CPU
> > > selected can try to build acpi_processor_post_eject() without success
> > > as arch_unregister_cpu() is not defined.
> > > 
> > > Check this explicitly. This will be merged into:
> > > | ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug
> > > for any subsequent posting.
> > > 
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > Signed-off-by: James Morse <james.morse@arm.com>
> > > Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> > > Tested-by: Jianyong Wu <jianyong.wu@arm.com>
> > > ---
> > > This should probably be squashed into an earlier patch.  
> > 
> > Agreed. If not
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> I'm not convinced that "ACPI: Add post_eject to struct acpi_scan_handler
> for cpu hotplug" is the correct commit to squash this into.
> 
> As far as acpi_processor.c is concerned, This commit merely renames
> acpi_processor_remove() to be acpi_processor_post_eject(). The function
> references arch_unregister_cpu() before and after this change, and its
> build is dependent on CONFIG_ACPI_HOTPLUG_PRESENT_CPU being defined.
> 
> Commit "ACPI: convert acpi_processor_post_eject() to use IS_ENABLED()"
> removed the ifdef CONFIG_ACPI_HOTPLUG_PRESENT_CPU surrounding
> acpi_processor_post_eject, and that symbol depends on
> CONFIG_HOTPLUG_CPU, so I think this commit is also fine.
> 
> Commit "ACPI: Check _STA present bit before making CPUs not present"
> rewrites the function - the original body gets called
> acpi_processor_make_not_present() and a new acpi_processor_post_eject()
> is created. At this point, it doesn't reference arch_unregister_cpu().
> 
> Commit "ACPI: add support to register CPUs based on the _STA enabled
> bit" adds a reference to arch_unregister_cpu() in this new
> acpi_processor_post_eject() - so I think this is the correct commit
> this change should be merged into.

That or where that change ends up given your earlier suggestion to
move that change as well.  I find it hard to care as long as
the bisection issue is squashed by the change.  If we make the code
drop out before the build issue is introduced that's fine because
we are arguing we shouldn't be running it anyway so such protection
is fine if not necessary for build fix purposes.

J

>