From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E848682892; Mon, 15 Apr 2024 16:51:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.176.79.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713199864; cv=none; b=ntUg9DfEIySqIWmsC7qXhXa7gHzyrv4VTlQKogF/qzAabqNzjEuY/ycfDRHSZiYgGSF/wB6D4PSQQmna3DSElSqDW6T22dQHomvhqjKNCS/cYxVlr/dRhEuY+XpxvWFEvQ5Ssgu1AXyjF7WGHszlqxWh7V3oN37bAK91TUDZ7yU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713199864; c=relaxed/simple; bh=42gjuAfnn8UcdjXUdQUGdlYqrMyCJ01W93Gmzugo0zI=; h=Date:From:To:CC:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=lDoQEkDNRbJ7TzbA9SK+/EzVIy58/wsy74pMjreAVbxhwqm/pBupmYpWIjoXWoK1Y0M7vXTEOQMuRW0YDgIc3efWpQs1N29QW9noXV1z46Gyhhw5V2mNzbE1E+j9QD9TTEBxEoz5sBCCu/5ITejJo1hSPpDNlJaSU9ZgoIHZmDc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=185.176.79.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4VJCjS2DSVz6K7Fp; Tue, 16 Apr 2024 00:46:04 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 3B972140736; Tue, 16 Apr 2024 00:50:59 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Mon, 15 Apr 2024 17:50:58 +0100 Date: Mon, 15 Apr 2024 17:50:57 +0100 From: Jonathan Cameron To: "Rafael J. Wysocki" CC: , , , , , , , , Russell King , Miguel Luis , James Morse , Salil Mehta , "Jean-Philippe Brucker" , Catalin Marinas , Will Deacon , , , Subject: Re: [PATCH v5 02/18] ACPI: processor: Set the ACPI_COMPANION for the struct cpu instance Message-ID: <20240415175057.00002e11@Huawei.com> In-Reply-To: References: <20240412143719.11398-1-Jonathan.Cameron@huawei.com> <20240412143719.11398-3-Jonathan.Cameron@huawei.com> <20240415164854.0000264f@Huawei.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: loongarch@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-ClientProxiedBy: lhrpeml500003.china.huawei.com (7.191.162.67) To lhrpeml500005.china.huawei.com (7.191.163.240) On Mon, 15 Apr 2024 18:19:17 +0200 "Rafael J. Wysocki" wrote: > On Mon, Apr 15, 2024 at 6:16=E2=80=AFPM Rafael J. Wysocki wrote: > > > > On Mon, Apr 15, 2024 at 5:49=E2=80=AFPM Jonathan Cameron > > wrote: =20 > > > > > > On Fri, 12 Apr 2024 20:10:54 +0200 > > > "Rafael J. Wysocki" wrote: > > > =20 > > > > On Fri, Apr 12, 2024 at 4:38=E2=80=AFPM Jonathan Cameron > > > > wrote: =20 > > > > > > > > > > The arm64 specific arch_register_cpu() needs to access the _STA > > > > > method of the DSDT object so make it available by assigning the > > > > > appropriate handle to the struct cpu instance. > > > > > > > > > > Signed-off-by: Jonathan Cameron > > > > > --- > > > > > drivers/acpi/acpi_processor.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_pr= ocessor.c > > > > > index 7a0dd35d62c9..93e029403d05 100644 > > > > > --- a/drivers/acpi/acpi_processor.c > > > > > +++ b/drivers/acpi/acpi_processor.c > > > > > @@ -235,6 +235,7 @@ static int acpi_processor_get_info(struct acp= i_device *device) > > > > > union acpi_object object =3D { 0 }; > > > > > struct acpi_buffer buffer =3D { sizeof(union acpi_object)= , &object }; > > > > > struct acpi_processor *pr =3D acpi_driver_data(device); > > > > > + struct cpu *c; > > > > > int device_declaration =3D 0; > > > > > acpi_status status =3D AE_OK; > > > > > static int cpu0_initialized; > > > > > @@ -314,6 +315,8 @@ static int acpi_processor_get_info(struct acp= i_device *device) > > > > > cpufreq_add_device("acpi-cpufreq"); > > > > > } > > > > > > > > > > + c =3D &per_cpu(cpu_devices, pr->id); > > > > > + ACPI_COMPANION_SET(&c->dev, device); =20 > > > > > > > > This is also set for per_cpu(cpu_sys_devices, pr->id) in > > > > acpi_processor_add(), via acpi_bind_one(). =20 > > > > > > Hi Rafael, > > > > > > cpu_sys_devices gets filled with a pointer to this same structure. > > > The contents gets set in register_cpu() so at this point > > > it doesn't point anywhere. As a side note register_cpu() > > > memsets to zero the value I set it to in the code above which isn't > > > great, particularly as I want to use this in post_eject for > > > arm64. > > > > > > We could make a copy of the handle and put it back after > > > the memset in register_cpu() but that is also ugly. > > > It's the best I've come up with to make sure this is still set > > > come remove time but is rather odd. =20 > > > > > > > > Moreover, there is some pr->id validation in acpi_processor_add(), = so > > > > it seems premature to use it here this way. > > > > > > > > I think that ACPI_COMPANION_SET() should be called from here on > > > > per_cpu(cpu_sys_devices, pr->id) after validating pr->id (so the > > > > pr->id validation should all be done here) and then NULL can be pas= sed > > > > as acpi_dev to acpi_bind_one() in acpi_processor_add(). Then, there > > > > will be one physical device corresponding to the processor ACPI dev= ice > > > > and no confusion. =20 > > > > > > I'm fairly sure this is pointing to the same device but agreed this > > > is a tiny bit confusing. However we can't use cpu_sys_devices at this= point > > > so I'm not immediately seeing a cleaner solution :( =20 > > > > Well, OK. > > > > Please at least consider doing the pr->id validation checks before > > setting the ACPI companion for &per_cpu(cpu_devices, pr->id). > > > > Also, acpi_bind_one() needs to be called on the "physical" devices > > passed to ACPI_COMPANION_SET() (with NULL as the second argument) for > > the reference counting and physical device lookup to work. > > > > Please also note that acpi_primary_dev_companion() should return > > per_cpu(cpu_sys_devices, pr->id) for the processor ACPI device, which > > depends on the order of acpi_bind_one() calls involving the same ACPI > > device. =20 >=20 > Of course, if the value set by ACPI_COMPANION_SET() is cleared > subsequently, the above is not needed, but then using > ACPI_COMPANION_SET() is questionable overall. Agreed + smoothing over that by stashing and putting it back doesn't work because there is an additional call to acpi_bind_one() inbetween here and the one you reference. The arch_register_cpu() calls end up calling register_cpu() / device_register() / acpi_device_notify() / acpi_bind_one() With current code that fails (silently) If I make sure the handle is set before register_cpu() then it succeeds, but we end up with duplicate sysfs files etc because we bind twice. I think the only way around this is larger reorganization of the CPU hotplug code to pull the arch_register_cpu() call to where the acpi_bind_one() call is. However that changes a lot more than I'd like (and I don't have it working yet). Alternatively find somewhere else to stash the handle, or just add it as a parameter to arch_register_cpu(). Right now this feels the easier path to me. arch_register_cpu(int cpu, acpi_handle handle)=20 Would that be a path you'd consider? Jonathan