| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 17 Feb 2016 16:10:11 +0000
From: Sudeep Holla <sudeep.holla@....com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Sudeep Holla <sudeep.holla@....com>, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-ia64@...r.kernel.org,
x86@...nel.org, Al Stone <al.stone@...aro.org>,
Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
Mahesh Sivasubramanian <msivasub@...eaurora.org>,
Ashwin Chaugule <ashwin.chaugule@...aro.org>,
Prashanth Prakash <pprakash@...eaurora.org>
Subject: Re: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power
Idle(LPI) states
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@...ysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@....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
Powered by blists - more mailing lists