Message ID | 2885079.e9J7NaK4W3@kreacher |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp589207wrd; Fri, 3 Mar 2023 11:28:46 -0800 (PST) X-Google-Smtp-Source: AK7set//Aw0xlrhhPj/8l6X4q4B7YNZoPjV6L3onPLBD3QZHMRzPC6ALJrwn4bSqVuZKFu3AbB+T X-Received: by 2002:aa7:d649:0:b0:4ac:d973:bb2c with SMTP id v9-20020aa7d649000000b004acd973bb2cmr3312986edr.28.1677871726111; Fri, 03 Mar 2023 11:28:46 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1677871726; cv=none; d=google.com; s=arc-20160816; b=eJx3Z4t//cj624CTlhhbL4NDAvXjgORw27zN0pnKvm/9zPRVHHBkw5uflKoSbKeZqX fOyuYnz08Brni+PeMBdVkFNnpk6YxMuYHLHM+AfPYIr+m5Q6NFJEE/Xb8v6iHnDc+PIb sgCGE6L+QMAmV88UuWMwjwyX5rGnTgaeoremvjqpFTqPpXmKY6MLwDsMvHtazafICL2F IdvVX8cLgxSNzGIGKiKIVApvXLzXzq0RwG06PxKU0Y1n2dm3fYvAgOr0TO6G3U2ESjcL 1hhMH5iGtypuE2i9icCXKZx0GAdETGcPRYKC0/ebEuszMd9vs6kKt3kTiF+WFH0xhcs0 2aJA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=ZhrIekPUUAnYz+mcHuRiHAve8sfkvQy0FBk9EbebcwI=; b=Xk8NJhir4WXCkBWIpgaFDutFrBDTL73ROQA6BN+VO+4i/mQ1fjdOjp82nmKytOE8ua +0RxlMa7nx6Wbry7HzV8qA/gD7pLVb8CFAsZoa1DmJjG3g4+Z/tzUsLlbWeCE9w1aIJQ /4oPMMJBDB6lUCcrcpwjaPchmZ0n2NeL40H0N/d3ipdFt0Hw2UQAyS/T1e1feMg828wE UU5cnF9qDLMCYnFN/OCVnZKzayF5hqq4F+6iS49tr2tqHEBSVmNil2ZQiBX1+f4JRD2M yk9NAjGu+kxZksolpfLpKRUjIr28kaK9T+yUMAcBfKyRcfUo9+Nf2zFUrv5j4/XE/zw2 5sdQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j3-20020aa7c0c3000000b004bddb9bb058si3473858edp.57.2023.03.03.11.28.23; Fri, 03 Mar 2023 11:28:46 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231637AbjCCTY0 (ORCPT <rfc822;davidbtadokoro@gmail.com> + 99 others); Fri, 3 Mar 2023 14:24:26 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55488 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231588AbjCCTYR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 3 Mar 2023 14:24:17 -0500 Received: from cloudserver094114.home.pl (cloudserver094114.home.pl [79.96.170.134]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D3BB85ADF3; Fri, 3 Mar 2023 11:24:15 -0800 (PST) Received: from localhost (127.0.0.1) (HELO v370.home.net.pl) by /usr/run/smtp (/usr/run/postfix/private/idea_relay_lmtp) via UNIX with SMTP (IdeaSmtpServer 5.1.0) id fa951605dcb9aaf0; Fri, 3 Mar 2023 20:24:14 +0100 Received: from kreacher.localnet (unknown [213.134.183.41]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by v370.home.net.pl (Postfix) with ESMTPSA id 29E4920619DE; Fri, 3 Mar 2023 20:24:13 +0100 (CET) From: "Rafael J. Wysocki" <rjw@rjwysocki.net> To: Linux PM <linux-pm@vger.kernel.org> Cc: Zhang Rui <rui.zhang@intel.com>, Linux ACPI <linux-acpi@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, Daniel Lezcano <daniel.lezcano@linaro.org>, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, Viresh Kumar <viresh.kumar@linaro.org>, Quanxian Wang <quanxian.wang@intel.com> Subject: [PATCH v1 1/4] ACPI: processor: Reorder acpi_processor_driver_init() Date: Fri, 03 Mar 2023 20:19:46 +0100 Message-ID: <2885079.e9J7NaK4W3@kreacher> In-Reply-To: <2148907.irdbgypaU6@kreacher> References: <2148907.irdbgypaU6@kreacher> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="UTF-8" X-CLIENT-IP: 213.134.183.41 X-CLIENT-HOSTNAME: 213.134.183.41 X-VADE-SPAMSTATE: clean X-VADE-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedvhedrudelledguddukecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfjqffogffrnfdpggftiffpkfenuceurghilhhouhhtmecuudehtdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhephffvvefufffkjghfggfgtgesthfuredttddtjeenucfhrhhomhepfdftrghfrggvlhculfdrucghhihsohgtkhhifdcuoehrjhifsehrjhifhihsohgtkhhirdhnvghtqeenucggtffrrghtthgvrhhnpeefudduuedtuefgleffudeigeeitdeufeelvdejgefftdethffhhfethfeljefgteenucffohhmrghinhepkhgvrhhnvghlrdhorhhgnecukfhppedvudefrddufeegrddukeefrdegudenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepihhnvghtpedvudefrddufeegrddukeefrdeguddphhgvlhhopehkrhgvrggthhgvrhdrlhhotggrlhhnvghtpdhmrghilhhfrhhomhepfdftrghfrggvlhculfdrucghhihsohgtkhhifdcuoehrjhifsehrjhifhihsohgtkhhirdhnvghtqedpnhgspghrtghpthhtohepkedprhgtphhtthhopehlihhnuhigqdhpmhesvhhgvghrrdhkvghrnhgvlhdrohhrghdprhgtphhtthhopehruhhirdiihhgrnhhgsehinhhtvghlrdgtohhmpdhrtghpthhtoheplhhinhhugidqrggtphhisehvghgvrhdrkhgvrhhnvghlrdhorhhgpdhrtghpthhtoheplhhinhhugidqkhgvrhhnvghlsehvghgv rhdrkhgvrhhnvghlrdhorhhgpdhrtghpthhtohepuggrnhhivghlrdhlvgiitggrnhhosehlihhnrghrohdrohhrghdprhgtphhtthhopehsrhhinhhivhgrshdrphgrnhgurhhuvhgruggrsehlihhnuhigrdhinhhtvghlrdgtohhm X-DCC--Metrics: v370.home.net.pl 1024; Body=8 Fuz1=8 Fuz2=8 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759376023126632063?= X-GMAIL-MSGID: =?utf-8?q?1759376023126632063?= |
Series |
thermal: core/ACPI: Fix processor cooling device regression
|
|
Commit Message
Rafael J. Wysocki
March 3, 2023, 7:19 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> The cpufreq policy notifier in the ACPI processor driver may as well be registered before the driver itself, which causes acpi_processor_cpufreq_init to be true (unless the notifier registration fails, which is unlikely at that point) when the ACPI CPU thermal cooling devices are registered, so the processor_get_max_state() result does not change while acpi_processor_driver_init() is running. Change the ordering in acpi_processor_driver_init() accordingly to prevent the max_state value from remaining 0 permanently for all ACPI CPU cooling devices. Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") Reported-by: Wang, Quanxian <quanxian.wang@intel.com> Link: https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/processor_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Comments
On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The cpufreq policy notifier in the ACPI processor driver may as > well be registered before the driver itself, which causes > acpi_processor_cpufreq_init to be true (unless the notifier > registration fails, which is unlikely at that point) when the > ACPI CPU thermal cooling devices are registered, so the > processor_get_max_state() result does not change while > acpi_processor_driver_init() is running. > > Change the ordering in acpi_processor_driver_init() accordingly > to prevent the max_state value from remaining 0 permanently for all > ACPI CPU cooling devices. > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") > Reported-by: Wang, Quanxian <quanxian.wang@intel.com> > Link: > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/processor_driver.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/acpi/processor_driver.c > =================================================================== > --- linux-pm.orig/drivers/acpi/processor_driver.c > +++ linux-pm/drivers/acpi/processor_driver.c > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ > if (acpi_disabled) > return 0; > > + if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > + CPUFREQ_POLICY_NOTIFIER)) { > + acpi_processor_cpufreq_init = true; > + acpi_processor_ignore_ppc_init(); > + } > + > result = driver_register(&acpi_processor_driver); > if (result < 0) > return result; > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu- > drv:dead", > NULL, acpi_soft_cpu_dead); > > - if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > - CPUFREQ_POLICY_NOTIFIER)) { > - acpi_processor_cpufreq_init = true; > - acpi_processor_ignore_ppc_init(); > - } > - > acpi_processor_throttling_init(); > return 0; > err: > Just FYI. I need some time to ramp up on the ordering here to double confirm this does not break any dependency, too many things are involved here :p. I will test the whole patch series later this week. thanks, rui
On Tue, Mar 7, 2023 at 5:55 PM Zhang, Rui <rui.zhang@intel.com> wrote: > > On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The cpufreq policy notifier in the ACPI processor driver may as > > well be registered before the driver itself, which causes > > acpi_processor_cpufreq_init to be true (unless the notifier > > registration fails, which is unlikely at that point) when the > > ACPI CPU thermal cooling devices are registered, so the > > processor_get_max_state() result does not change while > > acpi_processor_driver_init() is running. > > > > Change the ordering in acpi_processor_driver_init() accordingly > > to prevent the max_state value from remaining 0 permanently for all > > ACPI CPU cooling devices. > > > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") > > Reported-by: Wang, Quanxian <quanxian.wang@intel.com> > > Link: > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/acpi/processor_driver.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > Index: linux-pm/drivers/acpi/processor_driver.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/processor_driver.c > > +++ linux-pm/drivers/acpi/processor_driver.c > > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ > > if (acpi_disabled) > > return 0; > > > > + if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > + CPUFREQ_POLICY_NOTIFIER)) { > > + acpi_processor_cpufreq_init = true; > > + acpi_processor_ignore_ppc_init(); > > + } > > + > > result = driver_register(&acpi_processor_driver); > > if (result < 0) > > return result; > > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ > > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu- > > drv:dead", > > NULL, acpi_soft_cpu_dead); > > > > - if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > - CPUFREQ_POLICY_NOTIFIER)) { > > - acpi_processor_cpufreq_init = true; > > - acpi_processor_ignore_ppc_init(); > > - } > > - > > acpi_processor_throttling_init(); > > return 0; > > err: > > > Just FYI. > I need some time to ramp up on the ordering here to double confirm this > does not break any dependency, too many things are involved here :p. Unless I've overlooked something tricky, it should be fine, but of course verifying this independently won't hurt. > I will test the whole patch series later this week. Thank you!
On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The cpufreq policy notifier in the ACPI processor driver may as > well be registered before the driver itself, which causes > acpi_processor_cpufreq_init to be true (unless the notifier > registration fails, which is unlikely at that point) when the > ACPI CPU thermal cooling devices are registered, so the > processor_get_max_state() result does not change while > acpi_processor_driver_init() is running. > > Change the ordering in acpi_processor_driver_init() accordingly > to prevent the max_state value from remaining 0 permanently for all > ACPI CPU cooling devices. > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") > Reported-by: Wang, Quanxian <quanxian.wang@intel.com> > Link: > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> The full patch series fixes the problem but this one does not. This is because, static int cpu_has_cpufreq(unsigned int cpu) { struct cpufreq_policy *policy; if (!acpi_processor_cpufreq_init) return 0; policy = cpufreq_cpu_get(cpu); if (policy) { cpufreq_cpu_put(policy); return 1; } return 0; } Although acpi_processor_cpufreq_init is set to true with patch 1/4, but we don't have cpufreq driver registered, thus cpufreq_cpu_get() return NULL. so acpi_processor_cpufreq_init is not the only dependency here. :( thanks, rui > --- > drivers/acpi/processor_driver.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > Index: linux-pm/drivers/acpi/processor_driver.c > =================================================================== > --- linux-pm.orig/drivers/acpi/processor_driver.c > +++ linux-pm/drivers/acpi/processor_driver.c > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ > if (acpi_disabled) > return 0; > > + if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > + CPUFREQ_POLICY_NOTIFIER)) { > + acpi_processor_cpufreq_init = true; > + acpi_processor_ignore_ppc_init(); > + } > + > result = driver_register(&acpi_processor_driver); > if (result < 0) > return result; > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu- > drv:dead", > NULL, acpi_soft_cpu_dead); > > - if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > - CPUFREQ_POLICY_NOTIFIER)) { > - acpi_processor_cpufreq_init = true; > - acpi_processor_ignore_ppc_init(); > - } > - > acpi_processor_throttling_init(); > return 0; > err: > > >
On Sun, Mar 12, 2023 at 5:09 PM Zhang, Rui <rui.zhang@intel.com> wrote: > > On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The cpufreq policy notifier in the ACPI processor driver may as > > well be registered before the driver itself, which causes > > acpi_processor_cpufreq_init to be true (unless the notifier > > registration fails, which is unlikely at that point) when the > > ACPI CPU thermal cooling devices are registered, so the > > processor_get_max_state() result does not change while > > acpi_processor_driver_init() is running. > > > > Change the ordering in acpi_processor_driver_init() accordingly > > to prevent the max_state value from remaining 0 permanently for all > > ACPI CPU cooling devices. > > > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") > > Reported-by: Wang, Quanxian <quanxian.wang@intel.com> > > Link: > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The full patch series fixes the problem but this one does not. That is a correct observation, but the $subject patch fixes part of the problem (which is not addressed by the rest of the series AFAICS) and so it deserves a Fixes tag of its own IMO. I guess I should clarify that in the changelog. > This is because, > > static int cpu_has_cpufreq(unsigned int cpu) > { > struct > cpufreq_policy *policy; > > if (!acpi_processor_cpufreq_init) > return 0; > > policy = cpufreq_cpu_get(cpu); > if (policy) { > cpufreq_cpu_put(policy); > return 1; > } > return 0; > } > > Although acpi_processor_cpufreq_init is set to true with patch 1/4, but > we don't have cpufreq driver registered, thus cpufreq_cpu_get() return > NULL. > so acpi_processor_cpufreq_init is not the only dependency here. :( Right. That's why the other patches in the series are needed too. > > --- > > drivers/acpi/processor_driver.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > Index: linux-pm/drivers/acpi/processor_driver.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/processor_driver.c > > +++ linux-pm/drivers/acpi/processor_driver.c > > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ > > if (acpi_disabled) > > return 0; > > > > + if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > + CPUFREQ_POLICY_NOTIFIER)) { > > + acpi_processor_cpufreq_init = true; > > + acpi_processor_ignore_ppc_init(); > > + } > > + > > result = driver_register(&acpi_processor_driver); > > if (result < 0) > > return result; > > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ > > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu- > > drv:dead", > > NULL, acpi_soft_cpu_dead); > > > > - if (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > - CPUFREQ_POLICY_NOTIFIER)) { > > - acpi_processor_cpufreq_init = true; > > - acpi_processor_ignore_ppc_init(); > > - } > > - > > acpi_processor_throttling_init(); > > return 0; > > err: > > > > > >
On Mon, 2023-03-13 at 14:48 +0100, Rafael J. Wysocki wrote: > On Sun, Mar 12, 2023 at 5:09 PM Zhang, Rui <rui.zhang@intel.com> > wrote: > > On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The cpufreq policy notifier in the ACPI processor driver may as > > > well be registered before the driver itself, which causes > > > acpi_processor_cpufreq_init to be true (unless the notifier > > > registration fails, which is unlikely at that point) when the > > > ACPI CPU thermal cooling devices are registered, so the > > > processor_get_max_state() result does not change while > > > acpi_processor_driver_init() is running. > > > > > > Change the ordering in acpi_processor_driver_init() accordingly > > > to prevent the max_state value from remaining 0 permanently for > > > all > > > ACPI CPU cooling devices. > > > > > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") > > > Reported-by: Wang, Quanxian <quanxian.wang@intel.com> > > > Link: > > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The full patch series fixes the problem but this one does not. > > That is a correct observation, but the $subject patch fixes part of > the problem (which is not addressed by the rest of the series AFAICS) > and so it deserves a Fixes tag of its own IMO. Am I understanding this correctly that this patch helps in below case? cpufreq driver like intel_pstate is registered before we register the notifier callback in processor_driver. In this case, we are not able to catch the CPUFREQ_CREATE_POLICY notification and cpufreq should be counted as part of cooling states when registering the ACPI CPU cooling device. (acpi_processor_cpufreq_init must be set at this time) Or else, in normal case, the ACPI CPU cdev->max_state always return 0 (when t-state not available) until we receive the CPUFREQ_CREATE_POLICY notification and call thermal_cooling_device_update(), both with and without this patch. Patch 2,3,4 works on my test platform, without applying patch 1/4. thanks, rui > > I guess I should clarify that in the changelog. > > > This is because, > > > > static int cpu_has_cpufreq(unsigned int cpu) > > { > > struct > > cpufreq_policy *policy; > > > > if (!acpi_processor_cpufreq_init) > > return 0; > > > > policy = cpufreq_cpu_get(cpu); > > if (policy) { > > cpufreq_cpu_put(policy); > > return 1; > > } > > return 0; > > } > > > > Although acpi_processor_cpufreq_init is set to true with patch 1/4, > > but > > we don't have cpufreq driver registered, thus cpufreq_cpu_get() > > return > > NULL. > > so acpi_processor_cpufreq_init is not the only dependency here. :( > > Right. That's why the other patches in the series are needed too. > > > > --- > > > drivers/acpi/processor_driver.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > Index: linux-pm/drivers/acpi/processor_driver.c > > > ================================================================= > > > == > > > --- linux-pm.orig/drivers/acpi/processor_driver.c > > > +++ linux-pm/drivers/acpi/processor_driver.c > > > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ > > > if (acpi_disabled) > > > return 0; > > > > > > + if > > > (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > > + CPUFREQ_POLICY_NOTIFIER)) { > > > + acpi_processor_cpufreq_init = true; > > > + acpi_processor_ignore_ppc_init(); > > > + } > > > + > > > result = driver_register(&acpi_processor_driver); > > > if (result < 0) > > > return result; > > > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ > > > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, > > > "acpi/cpu- > > > drv:dead", > > > NULL, acpi_soft_cpu_dead); > > > > > > - if > > > (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > > - CPUFREQ_POLICY_NOTIFIER)) { > > > - acpi_processor_cpufreq_init = true; > > > - acpi_processor_ignore_ppc_init(); > > > - } > > > - > > > acpi_processor_throttling_init(); > > > return 0; > > > err: > > > > > > > > >
On Mon, Mar 13, 2023 at 3:54 PM Zhang, Rui <rui.zhang@intel.com> wrote: > > On Mon, 2023-03-13 at 14:48 +0100, Rafael J. Wysocki wrote: > > On Sun, Mar 12, 2023 at 5:09 PM Zhang, Rui <rui.zhang@intel.com> > > wrote: > > > On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The cpufreq policy notifier in the ACPI processor driver may as > > > > well be registered before the driver itself, which causes > > > > acpi_processor_cpufreq_init to be true (unless the notifier > > > > registration fails, which is unlikely at that point) when the > > > > ACPI CPU thermal cooling devices are registered, so the > > > > processor_get_max_state() result does not change while > > > > acpi_processor_driver_init() is running. > > > > > > > > Change the ordering in acpi_processor_driver_init() accordingly > > > > to prevent the max_state value from remaining 0 permanently for > > > > all > > > > ACPI CPU cooling devices. > > > > > > > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state") > > > > Reported-by: Wang, Quanxian <quanxian.wang@intel.com> > > > > Link: > > > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/ > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The full patch series fixes the problem but this one does not. > > > > That is a correct observation, but the $subject patch fixes part of > > the problem (which is not addressed by the rest of the series AFAICS) > > and so it deserves a Fixes tag of its own IMO. > > Am I understanding this correctly that this patch helps in below case? > > cpufreq driver like intel_pstate is registered before we register the > notifier callback in processor_driver. In this case, we are not able to > catch the CPUFREQ_CREATE_POLICY notification and cpufreq should be > counted as part of cooling states when registering the ACPI CPU cooling > device. (acpi_processor_cpufreq_init must be set at this time) Yes. > Or else, in normal case, the ACPI CPU cdev->max_state always return 0 > (when t-state not available) until we receive the CPUFREQ_CREATE_POLICY > notification and call thermal_cooling_device_update(), both with and > without this patch. > > Patch 2,3,4 works on my test platform, without applying patch 1/4. OK > > I guess I should clarify that in the changelog. > > > > > This is because, > > > > > > static int cpu_has_cpufreq(unsigned int cpu) > > > { > > > struct > > > cpufreq_policy *policy; > > > > > > if (!acpi_processor_cpufreq_init) > > > return 0; > > > > > > policy = cpufreq_cpu_get(cpu); > > > if (policy) { > > > cpufreq_cpu_put(policy); > > > return 1; > > > } > > > return 0; > > > } > > > > > > Although acpi_processor_cpufreq_init is set to true with patch 1/4, > > > but > > > we don't have cpufreq driver registered, thus cpufreq_cpu_get() > > > return > > > NULL. > > > so acpi_processor_cpufreq_init is not the only dependency here. :( > > > > Right. That's why the other patches in the series are needed too. > > > > > > --- > > > > drivers/acpi/processor_driver.c | 12 ++++++------ > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > Index: linux-pm/drivers/acpi/processor_driver.c > > > > ================================================================= > > > > == > > > > --- linux-pm.orig/drivers/acpi/processor_driver.c > > > > +++ linux-pm/drivers/acpi/processor_driver.c > > > > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ > > > > if (acpi_disabled) > > > > return 0; > > > > > > > > + if > > > > (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > > > + CPUFREQ_POLICY_NOTIFIER)) { > > > > + acpi_processor_cpufreq_init = true; > > > > + acpi_processor_ignore_ppc_init(); > > > > + } > > > > + > > > > result = driver_register(&acpi_processor_driver); > > > > if (result < 0) > > > > return result; > > > > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ > > > > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, > > > > "acpi/cpu- > > > > drv:dead", > > > > NULL, acpi_soft_cpu_dead); > > > > > > > > - if > > > > (!cpufreq_register_notifier(&acpi_processor_notifier_block, > > > > - CPUFREQ_POLICY_NOTIFIER)) { > > > > - acpi_processor_cpufreq_init = true; > > > > - acpi_processor_ignore_ppc_init(); > > > > - } > > > > - > > > > acpi_processor_throttling_init(); > > > > return 0; > > > > err: > > > > > > > > > > > >
Index: linux-pm/drivers/acpi/processor_driver.c =================================================================== --- linux-pm.orig/drivers/acpi/processor_driver.c +++ linux-pm/drivers/acpi/processor_driver.c @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_ if (acpi_disabled) return 0; + if (!cpufreq_register_notifier(&acpi_processor_notifier_block, + CPUFREQ_POLICY_NOTIFIER)) { + acpi_processor_cpufreq_init = true; + acpi_processor_ignore_ppc_init(); + } + result = driver_register(&acpi_processor_driver); if (result < 0) return result; @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_ cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD, "acpi/cpu-drv:dead", NULL, acpi_soft_cpu_dead); - if (!cpufreq_register_notifier(&acpi_processor_notifier_block, - CPUFREQ_POLICY_NOTIFIER)) { - acpi_processor_cpufreq_init = true; - acpi_processor_ignore_ppc_init(); - } - acpi_processor_throttling_init(); return 0; err: