Updated throttling fix patchkit

Updated throttling fix patchkit

[Andi Kleen]

Here's an updated patchkit to fix ACPI thermal zone throttling and some
related problems. See the individual patches for more details.

Changes to the earlier version:

- Address earlier review comments
- Fix package index computation
- Shut up Intel IPS driver

This is ready for merging now.

-Andi

[PATCH 1/5] ACPI: ec: Do request_region outside WARN()

[Andi Kleen]

WARN() is not supposed to have side effects, so move the request_regions
outside.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/acpi/ec.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index b19a18d..3268dcf 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -812,10 +812,10 @@ static int acpi_ec_add(struct acpi_device *device)
 		first_ec = ec;
 	device->driver_data = ec;
 
-	WARN(!request_region(ec->data_addr, 1, "EC data"),
-	     "Could not request EC data io port 0x%lx", ec->data_addr);
-	WARN(!request_region(ec->command_addr, 1, "EC cmd"),
-	     "Could not request EC cmd io port 0x%lx", ec->command_addr);
+	ret = !!request_region(ec->data_addr, 1, "EC data");
+	WARN(!ret, "Could not request EC data io port 0x%lx", ec->data_addr);
+	ret = !!request_region(ec->command_addr, 1, "EC cmd");
+	WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
 
 	pr_info(PREFIX "GPE = 0x%lx, I/O: command/status = 0x%lx, data = 0x%lx\n",
 			  ec->gpe, ec->command_addr, ec->data_addr);
-- 
1.7.7

Re: [PATCH 1/5] ACPI: ec: Do request_region outside WARN()

[Len Brown]

Applied.

thanks,
Len Brown, Intel Open Source Technology Center

On 02/06/2012 11:17 AM, Andi Kleen wrote:

> WARN() is not supposed to have side effects, so move the request_regions
> outside.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  drivers/acpi/ec.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index b19a18d..3268dcf 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -812,10 +812,10 @@ static int acpi_ec_add(struct acpi_device *device)
>  		first_ec = ec;
>  	device->driver_data = ec;
>  
> -	WARN(!request_region(ec->data_addr, 1, "EC data"),
> -	     "Could not request EC data io port 0x%lx", ec->data_addr);
> -	WARN(!request_region(ec->command_addr, 1, "EC cmd"),
> -	     "Could not request EC cmd io port 0x%lx", ec->command_addr);
> +	ret = !!request_region(ec->data_addr, 1, "EC data");
> +	WARN(!ret, "Could not request EC data io port 0x%lx", ec->data_addr);
> +	ret = !!request_region(ec->command_addr, 1, "EC cmd");
> +	WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
>  
>  	pr_info(PREFIX "GPE = 0x%lx, I/O: command/status = 0x%lx, data = 0x%lx\n",
>  			  ec->gpe, ec->command_addr, ec->data_addr);

[PATCH 2/5] ACPI: Make ACPI interrupt threaded

[Andi Kleen]

Some ACPI interrupt actions may need to wait, and it's easiest to
have a thread context for this. So turn the ACPI interrupt
into a threaded interrupt.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/acpi/osl.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 412a1e0..f3f6ff7 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -595,7 +595,8 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 
 	acpi_irq_handler = handler;
 	acpi_irq_context = context;
-	if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) {
+	if (request_threaded_irq(irq, NULL, acpi_irq, IRQF_SHARED, "acpi", 
+				 acpi_irq)) {
 		printk(KERN_ERR PREFIX "SCI (IRQ%d) allocation failed\n", irq);
 		acpi_irq_handler = NULL;
 		return AE_NOT_ACQUIRED;
-- 
1.7.7

[PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Andi Kleen]

On Intel CPUs the processor typically uses the highest frequency
set by any logical CPU. When the system overheats
Linux first forces the frequency to the lowest available one
to lower the temperature.

However this was done only per logical CPU, which means all
logical CPUs in a package would need to go through this before
the frequency is actually lowered.

Worse this delay actually prevents real throttling, because
the real throttle code only proceeds when the lowest frequency
is already reached.

So when a throttle event happens force the lowest frequency
for all CPUs in the package where it happened. The per CPU
state is now kept per package, not per logical CPU. An alternative
would be to do it per cpufreq unit, but since we want to bring
down the temperature of the complete chip it's better
to do it for all.

In principle it may even make sense to do it for all CPUs,
but I kept it on the package for now.

With this change the frequency is actually lowered, which
in terms also allows real throttling to proceed.

I also removed an unnecessary per cpu variable initialization.

v2: Fix package mapping
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/acpi/processor_thermal.c |   45 +++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 3b599ab..b149b6e 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -57,6 +57,27 @@ ACPI_MODULE_NAME("processor_thermal");
 static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
 static unsigned int acpi_thermal_cpufreq_is_init = 0;
 
+#define reduction_pctg(cpu) \
+	per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
+
+/*
+ * Emulate "per package data" using per cpu data (which should be really provided 
+ * elsewhere)
+ *
+ * Note we can lose a CPU on cpu hotunplug, in this case we forget the state
+ * temporarily. Fortunately that's not a big issue here (I hope)
+ */
+static int phys_package_first_cpu(int cpu)
+{
+	int i;
+	int id = topology_physical_package_id(cpu);
+
+	for_each_online_cpu (i)
+		if (topology_physical_package_id(i) == id)
+			return i;
+	return 0;
+}
+
 static int cpu_has_cpufreq(unsigned int cpu)
 {
 	struct cpufreq_policy policy;
@@ -76,7 +97,7 @@ static int acpi_thermal_cpufreq_notifier(struct notifier_block *nb,
 
 	max_freq = (
 	    policy->cpuinfo.max_freq *
-	    (100 - per_cpu(cpufreq_thermal_reduction_pctg, policy->cpu) * 20)
+	    (100 - reduction_pctg(policy->cpu) * 20)
 	) / 100;
 
 	cpufreq_verify_within_limits(policy, 0, max_freq);
@@ -102,16 +123,28 @@ static int cpufreq_get_cur_state(unsigned int cpu)
 	if (!cpu_has_cpufreq(cpu))
 		return 0;
 
-	return per_cpu(cpufreq_thermal_reduction_pctg, cpu);
+	return reduction_pctg(cpu);
 }
 
 static int cpufreq_set_cur_state(unsigned int cpu, int state)
 {
+	int i;
+
 	if (!cpu_has_cpufreq(cpu))
 		return 0;
 
-	per_cpu(cpufreq_thermal_reduction_pctg, cpu) = state;
-	cpufreq_update_policy(cpu);
+	reduction_pctg(cpu) = state;
+
+	/* 
+	 * Update all the CPUs in the same package because they all
+	 * contribute to the temperature and often share the same
+	 * frequency.
+	 */
+	for_each_online_cpu (i) {
+		if (topology_physical_package_id(i) == 
+		    topology_physical_package_id(cpu))
+			cpufreq_update_policy(i);
+	}
 	return 0;
 }
 
@@ -119,10 +152,6 @@ void acpi_thermal_cpufreq_init(void)
 {
 	int i;
 
-	for (i = 0; i < nr_cpu_ids; i++)
-		if (cpu_present(i))
-			per_cpu(cpufreq_thermal_reduction_pctg, i) = 0;
-
 	i = cpufreq_register_notifier(&acpi_thermal_cpufreq_notifier_block,
 				      CPUFREQ_POLICY_NOTIFIER);
 	if (!i)
-- 
1.7.7

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Matthew Garrett]

On Mon, Feb 06, 2012 at 08:17:11AM -0800, Andi Kleen wrote:
> +#define reduction_pctg(cpu) \
> +	per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))

I don't like using percentages here - we end up with the potential for 
several percentages to end up mapping to the same P state. I've sent a 
patch that replaces the percentage code with just stepping through P 
states instead. But otherwise, yes, this seems sensible. An open 
question is whether we should be doing the same on _PPC notifications. 
There's some vague evidence that Windows does.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Andi Kleen]

On Mon, Feb 06, 2012 at 04:31:06PM +0000, Matthew Garrett wrote:
> On Mon, Feb 06, 2012 at 08:17:11AM -0800, Andi Kleen wrote:
> > +#define reduction_pctg(cpu) \
> > +	per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
> 
> I don't like using percentages here - we end up with the potential for

The code was using percentages before, i merely moved the accounting
to be per package. I don't disagree with changing them, but it 
should be probably a separate patch.

-Andi

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Len Brown]

On 02/06/2012 11:31 AM, Matthew Garrett wrote:

> On Mon, Feb 06, 2012 at 08:17:11AM -0800, Andi Kleen wrote:
>> +#define reduction_pctg(cpu) \
>> +	per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
> 
> I don't like using percentages here - we end up with the potential for 
> several percentages to end up mapping to the same P state.


Does it matter?

> I've sent a

> patch that replaces the percentage code with just stepping through P 
> states instead. But otherwise, yes, this seems sensible. An open 
> question is whether we should be doing the same on _PPC notifications. 
> There's some vague evidence that Windows does.

If you stepped by P-states, then you behave entirely differently
on a machine with many P-states vs a machine with few P-states.

There is code floating about that exposes every 100 MHz step on SNB
and later as a P-state -- you can have quite a few...

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Matthew Garrett]

On Mon, Feb 13, 2012 at 06:30:33PM -0500, Len Brown wrote:
> On 02/06/2012 11:31 AM, Matthew Garrett wrote:
> 
> > On Mon, Feb 06, 2012 at 08:17:11AM -0800, Andi Kleen wrote:
> >> +#define reduction_pctg(cpu) \
> >> +	per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
> > 
> > I don't like using percentages here - we end up with the potential for 
> > several percentages to end up mapping to the same P state.
> 
> 
> Does it matter?

If you step through multiple percentages that map to the same P state, 
yes. On the other hand, re-reading the specification, it seems that this 
is the behaviour envisaged in the polling equation. I guess we'll stick 
with that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Len Brown]

Applied (w/ whitespace cleanups)

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Len Brown]

Andi,
Can you re-base this on top of my release tree?

Your fix is important, but conflicted with mjg's
more invasive percentage -> frequency change.

I opted to take mjg's patch 1st b/c I think that
your fix is important enough to integrate any time
in the release cycle - so it can go on top.

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Matthew Garrett]

On Fri, Mar 30, 2012 at 06:05:29AM -0400, Len Brown wrote:
> Andi,
> Can you re-base this on top of my release tree?
> 
> Your fix is important, but conflicted with mjg's
> more invasive percentage -> frequency change.

Sorry, I think it's probably better not to take my patch here - the spec 
uses percentages rather than frequencies. I'm not convinced that that 
makes sense, but there's some chance that some behaviour may end up 
depending on that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/5] ACPI: Do cpufreq clamping for throttling per package v2

[Len Brown]

On 03/30/2012 07:46 AM, Matthew Garrett wrote:

> On Fri, Mar 30, 2012 at 06:05:29AM -0400, Len Brown wrote:
>> Andi,
>> Can you re-base this on top of my release tree?
>>
>> Your fix is important, but conflicted with mjg's
>> more invasive percentage -> frequency change.
> 
> Sorry, I think it's probably better not to take my patch here - the spec 
> uses percentages rather than frequencies. I'm not convinced that that 
> makes sense, but there's some chance that some behaviour may end up 
> depending on that.
> 


Ok Matthew, I've dropped your patch and pulled in Andi's.

thanks,
-Len

[PATCH 5/5] Disable MCP limit exceeded messages from Intel IPS driver

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

On a system on the thermal limit these are quite noisy and flood the logs.
Better would be a counter anyways. But given that we don't even have
anything for normal throttling this doesn't seem to be urgent either.

Cc: jbarnes@virtuousgeek.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/platform/x86/intel_ips.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
index 809a3ae..84bba52 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -620,25 +620,16 @@ static bool mcp_exceeded(struct ips_driver *ips)
 	bool ret = false;
 	u32 temp_limit;
 	u32 avg_power;
-	const char *msg = "MCP limit exceeded: ";
 
 	spin_lock_irqsave(&ips->turbo_status_lock, flags);
 
 	temp_limit = ips->mcp_temp_limit * 100;
-	if (ips->mcp_avg_temp > temp_limit) {
-		dev_info(&ips->dev->dev,
-			"%sAvg temp %u, limit %u\n", msg, ips->mcp_avg_temp,
-			temp_limit);
+	if (ips->mcp_avg_temp > temp_limit)
 		ret = true;
-	}
 
 	avg_power = ips->cpu_avg_power + ips->mch_avg_power;
-	if (avg_power > ips->mcp_power_limit) {
-		dev_info(&ips->dev->dev,
-			"%sAvg power %u, limit %u\n", msg, avg_power,
-			ips->mcp_power_limit);
+	if (avg_power > ips->mcp_power_limit)
 		ret = true;
-	}
 
 	spin_unlock_irqrestore(&ips->turbo_status_lock, flags);
 
-- 
1.7.7

Re: [PATCH 5/5] Disable MCP limit exceeded messages from Intel IPS driver

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

On Mon,  6 Feb 2012 08:17:12 -0800
Andi Kleen <andi@firstfloor.org> wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> On a system on the thermal limit these are quite noisy and flood the logs.
> Better would be a counter anyways. But given that we don't even have
> anything for normal throttling this doesn't seem to be urgent either.
> 
> Cc: jbarnes@virtuousgeek.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Yeah looks fine.  I think I queued a message reduction patch awhile
back too... not sure if mjg saw it or not.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] Disable MCP limit exceeded messages from Intel IPS driver

[Len Brown]

Applied.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/5] ACPI: EC: Add a limited number of repeats after false EC interrupts

[Matthew Garrett]

On Mon, Feb 06, 2012 at 08:17:10AM -0800, Andi Kleen wrote:
> My Acer laptop has a large number of false EC interrupts
> (interrupts when the EC indexed data register protocol is in the wrong
> state, expecting input when we should send output or vice versa)
> It seems the hardware triggers the interrupt before it actually
> sets the right status in the register.

Our EC code is, at this point, a layer of hacks piled on top of other 
hacks. We have various patches that fix some machines and break others 
and a lack of a detailed description of what the driver actually does 
and where it deviates from the specification (and why).

I mention this not because I object to adding more hacks to the pile, 
but because at some point we're really going to need to bite the bullet 
and figure out how Windows deals with this hardware and what we're doing 
differently. That probably means adding ec emulation to qemu.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/5] ACPI: EC: Add a limited number of repeats after false EC interrupts

[Andi Kleen]

> Our EC code is, at this point, a layer of hacks piled on top of other 
> hacks. We have various patches that fix some machines and break others 
> and a lack of a detailed description of what the driver actually does 
> and where it deviates from the specification (and why).

The moving to an interrupt thread will enable a lot of cleanups
in fact. A lot of the existing code could be simplified now that
you can sleep. A lot of the nasty code was simply a workaround
for that.

I don't have plans to work on this however, since my laptop
works now (sorry). But I believe the patchkit is a good base
for further cleanups.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: Updated throttling fix patchkit

[Matthew Garrett]

On Mon, Feb 06, 2012 at 08:17:07AM -0800, Andi Kleen wrote:
> Here's an updated patchkit to fix ACPI thermal zone throttling and some
> related problems. See the individual patches for more details.

ACK to the set (with the provisos noted on a couple of the individual 
patches). It may need some rebasing depending on whether Len merges my 
other patch or not.

-- 
Matthew Garrett | mjg59@srcf.ucam.org