Re: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states
From: Sudeep Holla
Date: Wed Feb 17 2016 - 11:10:37 EST
On 16/02/16 20:46, Rafael J. Wysocki wrote:
On Wednesday, December 02, 2015 02:10:46 PM Sudeep Holla wrote:
ACPI 6.0 introduced an optional object _LPI that provides an alternate
method to describe Low Power Idle states. It defines the local power
states for each node in a hierarchical processor topology. The OSPM can
use _LPI object to select a local power state for each level of processor
hierarchy in the system. They used to produce a composite power state
request that is presented to the platform by the OSPM.
Since multiple processors affect the idle state for any non-leaf hierarchy
node, coordination of idle state requests between the processors is
required. ACPI supports two different coordination schemes: Platform
coordinated and OS initiated.
This patch adds initial support for Platform coordination scheme of LPI.
Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
My first point here would be the same as for [4/5]: please don't introduce
new Kconfig options if you don't have to (and you clearly don't have to in this
case, because it all can be made work without new Kconfig options).
Right, this was kind of defensive approach initially so as to not break
anything on x86, rather add anything new to x86 code path. I have
removed it now.
---
drivers/acpi/Kconfig | 3 +
drivers/acpi/bus.c | 8 +-
drivers/acpi/processor_driver.c | 2 +-
drivers/acpi/processor_idle.c | 440 +++++++++++++++++++++++++++++++++++-----
include/acpi/processor.h | 30 ++-
include/linux/acpi.h | 4 +
6 files changed, 435 insertions(+), 52 deletions(-)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 12873f0b8141..89a2d9b81feb 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -51,6 +51,9 @@ config ARCH_MIGHT_HAVE_ACPI_PDC
config ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
bool
+config ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
Not needed.
Done
+ bool
+
config ACPI_GENERIC_GSI
bool
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index a212cefae524..2e9e2e3fde6a 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -301,6 +301,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
EXPORT_SYMBOL(acpi_run_osc);
bool osc_sb_apei_support_acked;
+bool osc_pc_lpi_support_acked;
Maybe call it osc_pc_lpi_support_confirmed. And add a comment describing what
"PC LPI" means (a pointer to the relevant spec section might be useful too).
Agreed and done
[...]
@@ -943,24 +906,407 @@ static inline void acpi_processor_cstate_first_run_checks(void)
static inline int disabled_by_idle_boot_param(void) { return 0; }
static inline void acpi_processor_cstate_first_run_checks(void) { }
-static int acpi_processor_get_power_info(struct acpi_processor *pr)
+static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
{
return -ENODEV;
}
-
Unrelated whitespace change.
Removed
[...]
+
+ /* There must be at least 4 elements = 3 elements + 1 package */
+ if (!lpi || lpi->type != ACPI_TYPE_PACKAGE || lpi->package.count < 4) {
+ pr_info("not enough elements in _LPI\n");
pr_debug()? Plus maybe point to the LPI object in question in that message?
+ ret = -EFAULT;
-ENXIO? -EFAULT has a specific meaning which doesn't apply here.
Both done
+
+ /* TODO this long list is looking insane now
+ * need a cleaner and saner way to read the elements
+ */
Well, I'm not sure about the above comment. The code is what it is, anyone
can draw their own conclusions. :-)
Well that's stray leftover comment. When I started adding LPI changes, I
initially thought they may be better way to do that, but I have now
realized it's only way :). I will remove it
I would consider storing the whole buffer instead of trying to decode it
upfront, though. You need to flatten it etc going forward, so maybe
decode it at that time?
OK, I will look at this now.
Also I'm not sure about silent discarding things that look defective. I would
at least add a debug message to wherever this is done and maybe we can try
to fix up or fall back to some sane defaults at least in some cases?
Agreed.
[...]
+ info = kcalloc(max_leaf_depth + 1, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ phandle = pr->handle;
phandle is easily confised with DT phandles. I'd avoind using that for an ACPI
object handle variable name.
I changed it to pr_ahandle now(i.e processor acpi handle)
Well, what if it turns out that LPI is missing? Shouldn't we fall back to using
_CST then?
Of course, the problem here is that the presence of LPI has to be checked for
all CPUs in the system before deciding to fall back (in which case all of them
should be using _CST if present).
I agree, do we do similar check for _CST too ?
+ dev->cpu = pr->id;
+ if (pr->flags.has_lpi)
+ return acpi_processor_ffh_lpi_probe(pr->id);
+ else
+ return acpi_processor_setup_cpuidle_cx(pr, dev);
Fallback here too maybe?
Fixed
+}
+
+static int acpi_processor_get_power_info(struct acpi_processor *pr)
+{
+ int ret = 0;
+
+ ret = acpi_processor_get_cstate_info(pr);
+ if (ret)
+ ret = acpi_processor_get_lpi_info(pr);
Shouldn't that be done in the reverse order?
Yes, again kind of defensive approach I took to avoid any change, but
will change it.
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 50f2423d31fa..f3006831d427 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -39,6 +39,7 @@
#define ACPI_CSTATE_SYSTEMIO 0
#define ACPI_CSTATE_FFH 1
#define ACPI_CSTATE_HALT 2
+#define ACPI_CSTATE_INTEGER 3
What does this mean?
Ah bad naming, I introduced this to communicate to flattening algo that
it's a simple number that needs to used as is. Based on you comment
above, saving buffer and decoding later might remove the need of it.
--
Regards,
Sudeep