From 1170433e6611402b869c583fa1fbfd85106ff066 Mon Sep 17 00:00:00 2001 From: Dmitry Osipenko Date: Thu, 9 Jul 2020 20:35:32 +0300 Subject: cpuidle: tegra: Correctly handle result of arm_cpuidle_simple_enter() The enter() callback of CPUIDLE drivers returns index of the entered idle state on success or a negative value on failure. The negative value could any negative value, i.e. it doesn't necessarily needs to be a error code. That's because CPUIDLE core only cares about the fact of failure and not about the reason of the enter() failure. Like every other enter() callback, the arm_cpuidle_simple_enter() returns the entered idle-index on success. Unlike some of other drivers, it never fails. It happened that TEGRA_C1=index=err=0 in the code of cpuidle-tegra driver, and thus, there is no problem for the cpuidle-tegra driver created by the typo in the code which assumes that the arm_cpuidle_simple_enter() returns a error code. The arm_cpuidle_simple_enter() also may return a -ENODEV error if CPU_IDLE is disabled in a kernel's config, but all CPUIDLE drivers are disabled if CPU_IDLE is disabled, including the cpuidle-tegra driver. So we can't ever see the error code from arm_cpuidle_simple_enter() today. Of course the code may get some changes in the future and then the typo may transform into a real bug, so let's correct the typo! The tegra_cpuidle_state_enter() is now changed to make it return the entered idle-index on success and negative error code on fail, which puts it on par with the arm_cpuidle_simple_enter(), making code consistent in regards to the error handling. This patch fixes a minor typo in the code, it doesn't fix any bugs. Signed-off-by: Dmitry Osipenko Reviewed-by: Jon Hunter Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle-tegra.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c index a12fb141875a..e8956706a291 100644 --- a/drivers/cpuidle/cpuidle-tegra.c +++ b/drivers/cpuidle/cpuidle-tegra.c @@ -172,7 +172,7 @@ static int tegra_cpuidle_coupled_barrier(struct cpuidle_device *dev) static int tegra_cpuidle_state_enter(struct cpuidle_device *dev, int index, unsigned int cpu) { - int ret; + int err; /* * CC6 state is the "CPU cluster power-off" state. In order to @@ -183,9 +183,9 @@ static int tegra_cpuidle_state_enter(struct cpuidle_device *dev, * CPU cores, GIC and L2 cache). */ if (index == TEGRA_CC6) { - ret = tegra_cpuidle_coupled_barrier(dev); - if (ret) - return ret; + err = tegra_cpuidle_coupled_barrier(dev); + if (err) + return err; } local_fiq_disable(); @@ -194,15 +194,15 @@ static int tegra_cpuidle_state_enter(struct cpuidle_device *dev, switch (index) { case TEGRA_C7: - ret = tegra_cpuidle_c7_enter(); + err = tegra_cpuidle_c7_enter(); break; case TEGRA_CC6: - ret = tegra_cpuidle_cc6_enter(cpu); + err = tegra_cpuidle_cc6_enter(cpu); break; default: - ret = -EINVAL; + err = -EINVAL; break; } @@ -210,7 +210,7 @@ static int tegra_cpuidle_state_enter(struct cpuidle_device *dev, tegra_pm_clear_cpu_in_lp2(); local_fiq_enable(); - return ret; + return err ?: index; } static int tegra_cpuidle_adjust_state_index(int index, unsigned int cpu) @@ -236,21 +236,27 @@ static int tegra_cpuidle_enter(struct cpuidle_device *dev, int index) { unsigned int cpu = cpu_logical_map(dev->cpu); - int err; + int ret; index = tegra_cpuidle_adjust_state_index(index, cpu); if (dev->states_usage[index].disable) return -1; if (index == TEGRA_C1) - err = arm_cpuidle_simple_enter(dev, drv, index); + ret = arm_cpuidle_simple_enter(dev, drv, index); else - err = tegra_cpuidle_state_enter(dev, index, cpu); + ret = tegra_cpuidle_state_enter(dev, index, cpu); - if (err && (err != -EINTR || index != TEGRA_CC6)) - pr_err_once("failed to enter state %d err: %d\n", index, err); + if (ret < 0) { + if (ret != -EINTR || index != TEGRA_CC6) + pr_err_once("failed to enter state %d err: %d\n", + index, ret); + index = -1; + } else { + index = ret; + } - return err ? -1 : index; + return index; } static int tegra114_enter_s2idle(struct cpuidle_device *dev, -- cgit v1.2.3 From 653f68b6ecd15cf147817bb01c2b7d02f4bffc8e Mon Sep 17 00:00:00 2001 From: Chen Yu Date: Wed, 19 Aug 2020 11:23:54 +0800 Subject: ACPI: processor: Print more information when acpi_processor_evaluate_cst() fails Some platforms have bogus _CST which might cause unexpectd behavior in the CPU idle driver. Some bogus _CST might be unable to be disassembled by acpica-tools due to broken format. Print extra log if the _CST extraction/verification failes. This can be used to help narrow down why the CPU idle driver fails to behave as expected. Suggested-by: Zhang Rui Signed-off-by: Chen Yu [ rjw: Subject and changelog edits ] Signed-off-by: Rafael J. Wysocki --- drivers/acpi/acpi_processor.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index b51ddf3bb616..412a9725cc1e 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -798,22 +798,34 @@ int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu, memset(&cx, 0, sizeof(cx)); element = &cst->package.elements[i]; - if (element->type != ACPI_TYPE_PACKAGE) + if (element->type != ACPI_TYPE_PACKAGE) { + acpi_handle_info(handle, "_CST C%d type(%x) is not package, skip...\n", + i, element->type); continue; + } - if (element->package.count != 4) + if (element->package.count != 4) { + acpi_handle_info(handle, "_CST C%d package count(%d) is not 4, skip...\n", + i, element->package.count); continue; + } obj = &element->package.elements[0]; - if (obj->type != ACPI_TYPE_BUFFER) + if (obj->type != ACPI_TYPE_BUFFER) { + acpi_handle_info(handle, "_CST C%d package element[0] type(%x) is not buffer, skip...\n", + i, obj->type); continue; + } reg = (struct acpi_power_register *)obj->buffer.pointer; obj = &element->package.elements[1]; - if (obj->type != ACPI_TYPE_INTEGER) + if (obj->type != ACPI_TYPE_INTEGER) { + acpi_handle_info(handle, "_CST C[%d] package element[1] type(%x) is not integer, skip...\n", + i, obj->type); continue; + } cx.type = obj->integer.value; /* @@ -850,6 +862,8 @@ int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu, cx.entry_method = ACPI_CSTATE_HALT; snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT"); } else { + acpi_handle_info(handle, "_CST C%d declares FIXED_HARDWARE C-state but not supported in hardware, skip...\n", + i); continue; } } else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { @@ -857,6 +871,8 @@ int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu, snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI IOPORT 0x%x", cx.address); } else { + acpi_handle_info(handle, "_CST C%d space_id(%x) neither FIXED_HARDWARE nor SYSTEM_IO, skip...\n", + i, reg->space_id); continue; } @@ -864,14 +880,20 @@ int acpi_processor_evaluate_cst(acpi_handle handle, u32 cpu, cx.valid = 1; obj = &element->package.elements[2]; - if (obj->type != ACPI_TYPE_INTEGER) + if (obj->type != ACPI_TYPE_INTEGER) { + acpi_handle_info(handle, "_CST C%d package element[2] type(%x) not integer, skip...\n", + i, obj->type); continue; + } cx.latency = obj->integer.value; obj = &element->package.elements[3]; - if (obj->type != ACPI_TYPE_INTEGER) + if (obj->type != ACPI_TYPE_INTEGER) { + acpi_handle_info(handle, "_CST C%d package element[3] type(%x) not integer, skip...\n", + i, obj->type); continue; + } memcpy(&info->states[++last_index], &cx, sizeof(cx)); } -- cgit v1.2.3 From 10942019040c5557556ec22aae0f771b2a1a1a6d Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Tue, 1 Sep 2020 16:28:58 +0200 Subject: firmware: psci: Extend psci_set_osi_mode() to allow reset to PC mode The current user (cpuidle-psci) of psci_set_osi_mode() only needs to enable the PSCI OSI mode. Although, as subsequent changes shows, there is a need to be able to reset back into the PSCI PC mode. Therefore, let's extend psci_set_osi_mode() to take a bool as in-parameter, to let the user indicate whether to enable OSI or to switch back to PC mode. Reviewed-by: Sudeep Holla Signed-off-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle-psci-domain.c | 2 +- drivers/firmware/psci/psci.c | 12 +++++++----- include/linux/psci.h | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c index b6e9649ab0da..b6ab0415f450 100644 --- a/drivers/cpuidle/cpuidle-psci-domain.c +++ b/drivers/cpuidle/cpuidle-psci-domain.c @@ -278,7 +278,7 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) goto remove_pd; /* Try to enable OSI mode. */ - ret = psci_set_osi_mode(); + ret = psci_set_osi_mode(true); if (ret) { pr_warn("failed to enable OSI mode: %d\n", ret); psci_pd_remove_topology(np); diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index 92013ecc2d9e..00af99b6f97c 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -151,12 +151,15 @@ static u32 psci_get_version(void) return invoke_psci_fn(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0); } -int psci_set_osi_mode(void) +int psci_set_osi_mode(bool enable) { + unsigned long suspend_mode; int err; - err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, - PSCI_1_0_SUSPEND_MODE_OSI, 0, 0); + suspend_mode = enable ? PSCI_1_0_SUSPEND_MODE_OSI : + PSCI_1_0_SUSPEND_MODE_PC; + + err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, suspend_mode, 0, 0); return psci_to_linux_errno(err); } @@ -546,8 +549,7 @@ static int __init psci_1_0_init(struct device_node *np) pr_info("OSI mode supported.\n"); /* Default to PC mode. */ - invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, - PSCI_1_0_SUSPEND_MODE_PC, 0, 0); + psci_set_osi_mode(false); } return 0; diff --git a/include/linux/psci.h b/include/linux/psci.h index 14ad9b9ebcd6..2a1bfb890e58 100644 --- a/include/linux/psci.h +++ b/include/linux/psci.h @@ -18,7 +18,7 @@ bool psci_tos_resident_on(int cpu); int psci_cpu_suspend_enter(u32 state); bool psci_power_state_is_valid(u32 state); -int psci_set_osi_mode(void); +int psci_set_osi_mode(bool enable); bool psci_has_osi_support(void); struct psci_operations { -- cgit v1.2.3 From 70c179b49870929ca183421935415622d30875b5 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Tue, 1 Sep 2020 16:28:59 +0200 Subject: cpuidle: psci: Allow PM domain to be initialized even if no OSI mode If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain topology with the genpd providers isn't initialized. This is perfectly fine from cpuidle-psci point of view. However, since the PM domain topology in the DTS files is a description of the HW, no matter of whether the PSCI OSI mode is supported or not, other consumers besides the CPUs may rely on it. Therefore, let's always allow the initialization of the PM domain topology to succeed, independently of whether the PSCI OSI mode is supported. Consequentially we need to track if we succeed to enable the OSI mode, as to know when a domain idlestate can be selected. Note that, CPU devices are still not being attached to the PM domain topology, unless the PSCI OSI mode is supported. Acked-by: Sudeep Holla Signed-off-by: Ulf Hansson Signed-off-by: Rafael J. Wysocki --- drivers/cpuidle/cpuidle-psci-domain.c | 59 ++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c index b6ab0415f450..4a031c62f92a 100644 --- a/drivers/cpuidle/cpuidle-psci-domain.c +++ b/drivers/cpuidle/cpuidle-psci-domain.c @@ -105,7 +105,7 @@ static void psci_pd_free_states(struct genpd_power_state *states, kfree(states); } -static int psci_pd_init(struct device_node *np) +static int psci_pd_init(struct device_node *np, bool use_osi) { struct generic_pm_domain *pd; struct psci_pd_provider *pd_provider; @@ -135,11 +135,16 @@ static int psci_pd_init(struct device_node *np) pd->free_states = psci_pd_free_states; pd->name = kbasename(pd->name); - pd->power_off = psci_pd_power_off; pd->states = states; pd->state_count = state_count; pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN; + /* Allow power off when OSI has been successfully enabled. */ + if (use_osi) + pd->power_off = psci_pd_power_off; + else + pd->flags |= GENPD_FLAG_ALWAYS_ON; + /* Use governor for CPU PM domains if it has some states to manage. */ pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL; @@ -190,7 +195,7 @@ static void psci_pd_remove(void) } } -static int psci_pd_init_topology(struct device_node *np, bool add) +static int psci_pd_init_topology(struct device_node *np) { struct device_node *node; struct of_phandle_args child, parent; @@ -203,9 +208,7 @@ static int psci_pd_init_topology(struct device_node *np, bool add) child.np = node; child.args_count = 0; - - ret = add ? of_genpd_add_subdomain(&parent, &child) : - of_genpd_remove_subdomain(&parent, &child); + ret = of_genpd_add_subdomain(&parent, &child); of_node_put(parent.np); if (ret) { of_node_put(node); @@ -216,14 +219,20 @@ static int psci_pd_init_topology(struct device_node *np, bool add) return 0; } -static int psci_pd_add_topology(struct device_node *np) +static bool psci_pd_try_set_osi_mode(void) { - return psci_pd_init_topology(np, true); -} + int ret; -static void psci_pd_remove_topology(struct device_node *np) -{ - psci_pd_init_topology(np, false); + if (!psci_has_osi_support()) + return false; + + ret = psci_set_osi_mode(true); + if (ret) { + pr_warn("failed to enable OSI mode: %d\n", ret); + return false; + } + + return true; } static void psci_cpuidle_domain_sync_state(struct device *dev) @@ -244,14 +253,14 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; struct device_node *node; + bool use_osi; int ret = 0, pd_count = 0; if (!np) return -ENODEV; - /* Currently limit the hierarchical topology to be used in OSI mode. */ - if (!psci_has_osi_support()) - return 0; + /* If OSI mode is supported, let's try to enable it. */ + use_osi = psci_pd_try_set_osi_mode(); /* * Parse child nodes for the "#power-domain-cells" property and @@ -261,7 +270,7 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) if (!of_find_property(node, "#power-domain-cells", NULL)) continue; - ret = psci_pd_init(node); + ret = psci_pd_init(node, use_osi); if (ret) goto put_node; @@ -270,30 +279,24 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) /* Bail out if not using the hierarchical CPU topology. */ if (!pd_count) - return 0; + goto no_pd; /* Link genpd masters/subdomains to model the CPU topology. */ - ret = psci_pd_add_topology(np); + ret = psci_pd_init_topology(np); if (ret) goto remove_pd; - /* Try to enable OSI mode. */ - ret = psci_set_osi_mode(true); - if (ret) { - pr_warn("failed to enable OSI mode: %d\n", ret); - psci_pd_remove_topology(np); - goto remove_pd; - } - pr_info("Initialized CPU PM domain topology\n"); return 0; put_node: of_node_put(node); remove_pd: - if (pd_count) - psci_pd_remove(); + psci_pd_remove(); pr_err("failed to create CPU PM domains ret=%d\n", ret); +no_pd: + if (use_osi) + psci_set_osi_mode(false); return ret; } -- cgit v1.2.3 From f49735f4978f479b0de4f50ab217d5a56bc83c55 Mon Sep 17 00:00:00 2001 From: Lina Iyer Date: Tue, 22 Sep 2020 12:34:16 -0600 Subject: cpuidle: record state entry rejection statistics CPUs may fail to enter the chosen idle state if there was a pending interrupt, causing the cpuidle driver to return an error value. Record that and export it via sysfs along with the other idle state statistics. This could prove useful in understanding behavior of the governor and the system during usecases that involve multiple CPUs. Signed-off-by: Lina Iyer [ rjw: Changelog and documentation edits ] Signed-off-by: Rafael J. Wysocki --- Documentation/admin-guide/pm/cpuidle.rst | 9 +++++++++ drivers/cpuidle/cpuidle.c | 1 + drivers/cpuidle/sysfs.c | 3 +++ include/linux/cpuidle.h | 1 + 4 files changed, 14 insertions(+) diff --git a/Documentation/admin-guide/pm/cpuidle.rst b/Documentation/admin-guide/pm/cpuidle.rst index a96a423e3779..830868e526f4 100644 --- a/Documentation/admin-guide/pm/cpuidle.rst +++ b/Documentation/admin-guide/pm/cpuidle.rst @@ -528,6 +528,10 @@ object corresponding to it, as follows: Total number of times the hardware has been asked by the given CPU to enter this idle state. +``rejected`` + Total number of times a request to enter this idle state on the given + CPU was rejected. + The :file:`desc` and :file:`name` files both contain strings. The difference between them is that the name is expected to be more concise, while the description may be longer and it may contain white space or special characters. @@ -572,6 +576,11 @@ particular case. For these reasons, the only reliable way to find out how much time has been spent by the hardware in different idle states supported by it is to use idle state residency counters in the hardware, if available. +Generally, an interrupt received when trying to enter an idle state causes the +idle state entry request to be rejected, in which case the ``CPUIdle`` driver +may return an error code to indicate that this was the case. The :file:`usage` +and :file:`rejected` files report the number of times the given idle state +was entered successfully or rejected, respectively. .. _cpu-pm-qos: diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 6c7e5621cf9a..0ed5030b89d6 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -307,6 +307,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, } } else { dev->last_residency_ns = 0; + dev->states_usage[index].rejected++; } return entered_state; diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 091d1caceb41..53ec9585ccd4 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -256,6 +256,7 @@ define_show_state_time_function(exit_latency) define_show_state_time_function(target_residency) define_show_state_function(power_usage) define_show_state_ull_function(usage) +define_show_state_ull_function(rejected) define_show_state_str_function(name) define_show_state_str_function(desc) define_show_state_ull_function(above) @@ -312,6 +313,7 @@ define_one_state_ro(latency, show_state_exit_latency); define_one_state_ro(residency, show_state_target_residency); define_one_state_ro(power, show_state_power_usage); define_one_state_ro(usage, show_state_usage); +define_one_state_ro(rejected, show_state_rejected); define_one_state_ro(time, show_state_time); define_one_state_rw(disable, show_state_disable, store_state_disable); define_one_state_ro(above, show_state_above); @@ -325,6 +327,7 @@ static struct attribute *cpuidle_state_default_attrs[] = { &attr_residency.attr, &attr_power.attr, &attr_usage.attr, + &attr_rejected.attr, &attr_time.attr, &attr_disable.attr, &attr_above.attr, diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 6175c77bf25e..ed0da0e58e8b 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -38,6 +38,7 @@ struct cpuidle_state_usage { u64 time_ns; unsigned long long above; /* Number of times it's been too deep */ unsigned long long below; /* Number of times it's been too shallow */ + unsigned long long rejected; /* Number of times idle entry was rejected */ #ifdef CONFIG_SUSPEND unsigned long long s2idle_usage; unsigned long long s2idle_time; /* in US */ -- cgit v1.2.3