lkml.org 
[lkml]   [2016]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states
From
Date


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@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> 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

\
 
 \ /
  Last update: 2016-02-17 17:41    [W:0.078 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site