From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D25B6C4345F for ; Tue, 30 Apr 2024 04:17:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=r9ftQY85MGHOCRh6RePq/SswT8xY9+npoQL6sLXzUuA=; b=0nJemPcOKA+sdi cKNmwqMalYap7uDnX5CkjtFPIe5ceZRq43EV1SXRddWU7+chfGC70YB8Cw0tUVjTkWibvY0bs013v isXGzk8Qw0mrOo6OqO8sR8La49GjhH8MG72+MWJ/KjT7QHpmXJXgVZrSY9feEUEeu8kRqFZkFWeyf CspPzqjYZaEQsxWDhoZUq/TJ6h/ZeHcfhj9nqTllW26LCwtKBqzjdkdMJku+wiW+dNIQKEzXwZpDM Y/fqG9leJ9HXmgvaHWPKrfDNOkx0QOckvd89PDpAnO2LADxaIMqqEy4ClP6l2Dqf1VFJmYuksUWfy zEHx7i7IzUWCOWPG+iBw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1ew8-0000000519W-1Wlc; Tue, 30 Apr 2024 04:17:48 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1ew5-0000000518H-2imk for linux-arm-kernel@lists.infradead.org; Tue, 30 Apr 2024 04:17:47 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714450661; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WFct75rj5zT1fcuVMkkp7XGDkSWnsAkN+HgQaZ4kvE4=; b=HpktrYCeLgIzEdXNwSsl8G3MCHG5fVOfj02/W8rHHhoU0Uq/jAhzKB8KZuXefyLl4Qxh4D cn5JivQzYoepWnvMDPIzvJoI9+1/58WGokQEc66ViVlsz4CU2CS2konV5usIwNleuOsff/ JgKnRpid3LEEAqj9T66FS4MN2wNi1Ps= Received: from mail-pg1-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-270-X-NTO-hHMNa9eXoB00jj_w-1; Tue, 30 Apr 2024 00:17:39 -0400 X-MC-Unique: X-NTO-hHMNa9eXoB00jj_w-1 Received: by mail-pg1-f198.google.com with SMTP id 41be03b00d2f7-613dbdf5c1bso1750882a12.0 for ; Mon, 29 Apr 2024 21:17:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714450658; x=1715055458; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WFct75rj5zT1fcuVMkkp7XGDkSWnsAkN+HgQaZ4kvE4=; b=ea+DHdyArF7T+exYisL1rIimKV0WUhBLzvaxKSLi/HDSmex0T6HLuKJXWhcLv9/jar UKa35e9Kqvd9YTX4ik+OkYv7bcXj/GeyySR6dGiWNCX2qlM3YeGvJq9kMAQLJwZs5zqD SkkpDEOOzjfgr07mISfNIfbZYw5SbEn2DCpPVTL9DxYNHfCB4WvofVUtfiOVostJFa2O KL+Ru3yY40xZumeH70zUsHFyFX9iHLT/VsPDFSDA9EbuPxUJgFk+AJzpjqjiGWCEIU3J 8TBU+ePt5A0h9XR935S4rsSYLtBHBSpo6XptfXLjw1xXsP+6cTvkL7GCWuzKSzVlFh2A qoZA== X-Forwarded-Encrypted: i=1; AJvYcCVvOlPYQQwQvQ3GXOGXc0hriRISBB7tqKqSSnYaRzHuSpeKMgoxtDRCAMmg2wj9jeoHFjhtWFfYli25Ue8xs3cW+25yKqvDfMy3+jPtsK8AqTaseY0= X-Gm-Message-State: AOJu0YyMoY6fLJYKPPjfb94S/OxNlKOsJ+dNcOynADe9myI9pRtvtna5 aJWkKOXWsEsm5vBdHjeMQSHCLa9xeJrysZ4s9rdNpUVey6+BM9XtoZ29hY0+MbDmNzwCgd4FU4Z MH3Qw0vL06u1lKKQGZe/iBDbdersIDaq3/2IKdWKv6LqedjDUqYb8XZp8Hf8Uw9Iat+Or1g4g X-Received: by 2002:a05:6a21:99a1:b0:1aa:5e75:d31f with SMTP id ve33-20020a056a2199a100b001aa5e75d31fmr2010459pzb.16.1714450657973; Mon, 29 Apr 2024 21:17:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFES1V3wsak41bo4jYaZviVPRarqHfJAK9UnRag7Jd4ap+/A5BK+dXuv6S+yktJTMceHUvKDw== X-Received: by 2002:a05:6a21:99a1:b0:1aa:5e75:d31f with SMTP id ve33-20020a056a2199a100b001aa5e75d31fmr2010426pzb.16.1714450657571; Mon, 29 Apr 2024 21:17:37 -0700 (PDT) Received: from [192.168.68.50] ([43.252.112.88]) by smtp.gmail.com with ESMTPSA id h12-20020a170902680c00b001ec4db46318sm118687plk.232.2024.04.29.21.17.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 29 Apr 2024 21:17:36 -0700 (PDT) Message-ID: <80a2e07f-ecb2-48af-b2be-646f17e0e63e@redhat.com> Date: Tue, 30 Apr 2024 14:17:24 +1000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier To: Jonathan Cameron , Thomas Gleixner , Peter Zijlstra , 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, kvmarm@lists.linux.dev, x86@kernel.org, Russell King , "Rafael J . Wysocki" , Miguel Luis , James Morse , Salil Mehta , Jean-Philippe Brucker , Catalin Marinas , Will Deacon , Marc Zyngier , Hanjun Guo Cc: Ingo Molnar , Borislav Petkov , Dave Hansen , linuxarm@huawei.com, justin.he@arm.com, jianyong.wu@arm.com, Lorenzo Pieralisi , Sudeep Holla References: <20240426135126.12802-1-Jonathan.Cameron@huawei.com> <20240426135126.12802-5-Jonathan.Cameron@huawei.com> From: Gavin Shan In-Reply-To: <20240426135126.12802-5-Jonathan.Cameron@huawei.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240429_211745_801317_42506E0B X-CRM114-Status: GOOD ( 37.84 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 4/26/24 23:51, Jonathan Cameron wrote: > Make the per_cpu(processors, cpu) entries available earlier so that > they are available in arch_register_cpu() as ARM64 will need access > to the acpi_handle to distinguish between acpi_processor_add() > and earlier registration attempts (which will fail as _STA cannot > be checked). > > Reorder the remove flow to clear this per_cpu() after > arch_unregister_cpu() has completed, allowing it to be used in > there as well. > > Note that on x86 for the CPU hotplug case, the pr->id prior to > acpi_map_cpu() may be invalid. Thus the per_cpu() structures > must be initialized after that call or after checking the ID > is valid (not hotplug path). > > Signed-off-by: Jonathan Cameron > > --- > v8: On buggy bios detection when setting per_cpu structures > do not carry on. > Fix up the clearing of per cpu structures to remove unwanted > side effects and ensure an error code isn't use to reference them. > --- > drivers/acpi/acpi_processor.c | 79 +++++++++++++++++++++-------------- > 1 file changed, 48 insertions(+), 31 deletions(-) > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index ba0a6f0ac841..3b180e21f325 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -183,8 +183,38 @@ static void __init acpi_pcc_cpufreq_init(void) {} > #endif /* CONFIG_X86 */ > > /* Initialization */ > +static DEFINE_PER_CPU(void *, processor_device_array); > + > +static bool acpi_processor_set_per_cpu(struct acpi_processor *pr, > + struct acpi_device *device) > +{ > + BUG_ON(pr->id >= nr_cpu_ids); One blank line after BUG_ON() if we need to follow original implementation. > + /* > + * Buggy BIOS check. > + * ACPI id of processors can be reported wrongly by the BIOS. > + * Don't trust it blindly > + */ > + if (per_cpu(processor_device_array, pr->id) != NULL && > + per_cpu(processor_device_array, pr->id) != device) { > + dev_warn(&device->dev, > + "BIOS reported wrong ACPI id %d for the processor\n", > + pr->id); > + /* Give up, but do not abort the namespace scan. */ It depends on how the return value is handled by the caller if the namespace is continued to be scanned. The caller can be acpi_processor_hotadd_init() and acpi_processor_get_info() after this patch is applied. So I think this specific comment need to be moved to the caller. Besides, it seems acpi_processor_set_per_cpu() isn't properly called and memory leakage can happen. More details are given below. > + return false; > + } > + /* > + * processor_device_array is not cleared on errors to allow buggy BIOS > + * checks. > + */ > + per_cpu(processor_device_array, pr->id) = device; > + per_cpu(processors, pr->id) = pr; > + > + return true; > +} > + > #ifdef CONFIG_ACPI_HOTPLUG_CPU > -static int acpi_processor_hotadd_init(struct acpi_processor *pr) > +static int acpi_processor_hotadd_init(struct acpi_processor *pr, > + struct acpi_device *device) > { > int ret; > > @@ -198,8 +228,15 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr) > if (ret) > goto out; > > + if (!acpi_processor_set_per_cpu(pr, device)) { > + acpi_unmap_cpu(pr->id); > + goto out; > + } > + With the 'goto out', zero is returned from acpi_processor_hotadd_init() to acpi_processor_get_info(). The zero return value is carried from acpi_map_cpu() in acpi_processor_hotadd_init(). If I'm correct, we need return errno from acpi_processor_get_info() to acpi_processor_add() so that cleanup can be done. For example, the cleanup corresponding to the 'err' tag can be done in acpi_processor_add(). Otherwise, we will have memory leakage. > ret = arch_register_cpu(pr->id); > if (ret) { > + /* Leave the processor device array in place to detect buggy bios */ > + per_cpu(processors, pr->id) = NULL; > acpi_unmap_cpu(pr->id); > goto out; > } > @@ -217,7 +254,8 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr) > return ret; > } > #else > -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr) > +static inline int acpi_processor_hotadd_init(struct acpi_processor *pr, > + struct acpi_device *device) > { > return -ENODEV; > } > @@ -316,10 +354,13 @@ static int acpi_processor_get_info(struct acpi_device *device) > * because cpuid <-> apicid mapping is persistent now. > */ > if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) { > - int ret = acpi_processor_hotadd_init(pr); > + int ret = acpi_processor_hotadd_init(pr, device); > > if (ret) > return ret; > + } else { > + if (!acpi_processor_set_per_cpu(pr, device)) > + return 0; > } > For non-hotplug case, we still need pass the error to acpi_processor_add() so that cleanup corresponding 'err' tag can be done. Otherwise, we will have memory leakage. > /* > @@ -365,8 +406,6 @@ static int acpi_processor_get_info(struct acpi_device *device) > * (cpu_data(cpu)) values, like CPU feature flags, family, model, etc. > * Such things have to be put in and set up by the processor driver's .probe(). > */ > -static DEFINE_PER_CPU(void *, processor_device_array); > - > static int acpi_processor_add(struct acpi_device *device, > const struct acpi_device_id *id) > { > @@ -395,28 +434,6 @@ static int acpi_processor_add(struct acpi_device *device, > if (result) /* Processor is not physically present or unavailable */ > return 0; > > - BUG_ON(pr->id >= nr_cpu_ids); > - > - /* > - * Buggy BIOS check. > - * ACPI id of processors can be reported wrongly by the BIOS. > - * Don't trust it blindly > - */ > - if (per_cpu(processor_device_array, pr->id) != NULL && > - per_cpu(processor_device_array, pr->id) != device) { > - dev_warn(&device->dev, > - "BIOS reported wrong ACPI id %d for the processor\n", > - pr->id); > - /* Give up, but do not abort the namespace scan. */ > - goto err; > - } > - /* > - * processor_device_array is not cleared on errors to allow buggy BIOS > - * checks. > - */ > - per_cpu(processor_device_array, pr->id) = device; > - per_cpu(processors, pr->id) = pr; > - > dev = get_cpu_device(pr->id); > if (!dev) { > result = -ENODEV; > @@ -469,10 +486,6 @@ static void acpi_processor_remove(struct acpi_device *device) > device_release_driver(pr->dev); > acpi_unbind_one(pr->dev); > > - /* Clean up. */ > - per_cpu(processor_device_array, pr->id) = NULL; > - per_cpu(processors, pr->id) = NULL; > - > cpu_maps_update_begin(); > cpus_write_lock(); > > @@ -480,6 +493,10 @@ static void acpi_processor_remove(struct acpi_device *device) > arch_unregister_cpu(pr->id); > acpi_unmap_cpu(pr->id); > > + /* Clean up. */ > + per_cpu(processor_device_array, pr->id) = NULL; > + per_cpu(processors, pr->id) = NULL; > + > cpus_write_unlock(); > cpu_maps_update_done(); > Thanks, Gavin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel