Re: [PATCH] intel_ips: quieten "power or thermal limit exceeded" messages

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 27 Aug 2010 19:21:44 -0700
Joe Perches <joe@perches.com> wrote:

> On Fri, 2010-08-27 at 20:12 -0300, Cesar Eduardo Barros wrote:
> > Em 27-08-2010 04:39, Joe Perches escreveu:
> > > On Thu, 2010-08-26 at 22:38 -0300, Cesar Eduardo Barros wrote:
> > >> - The first "MCP power limit exceeded" seems very bogus.
> > >> - What do you mean, core_power_limit is zero?
> > > I added a logging message whenever the turbo limits change
> > > and logging messages for power/temp on MCH for completeness.
> > > Maybe this will show something useful like when/how
> > > CPU power limit gets set to 0.
> 
> > Running with it right now, did not help much:
> > 
> > $ dmesg | fgrep 'intel ips'
> > intel ips 0000:00:1f.6: Warning: CPU TDP doesn't match expected value 
> > (found 25, expected 35)
> > intel ips 0000:00:1f.6: PCI INT C -> GSI 18 (level, low) -> IRQ 18
> > intel ips 0000:00:1f.6: IPS driver initialized, MCP temp limit 65535
> > intel ips 0000:00:1f.6: MCP power limit 65535 exceeded: cpu:8058 + 
> > mch:23392829
> > intel ips 0000:00:1f.6: CPU power limit 0 exceeded: 5675
> > intel ips 0000:00:1f.6: CPU power limit 0 exceeded: 6369
> 
> I believe all these limits should always have non-zero values.
> So I still think you've hardware problems, but I suppose it
> could be the driver not reading the right registers or some
> such.  It seems odd that the driver never printed a logging
> message for either of the polling or irq methods to read the
> device cpu and thermal limits.
> 
> Jesse or any Intel folk, can you verify or suggest anything
> better?
> 
> If cpu_power_limit, or any _limit, is not set perhaps changing
> the test style to verify limit and adding a printed_once alert
> for each 0 value limit.  At least that'd shut up the continuous
> logging but at least give a notification message.
> 
> if (limit) {
> 	if (measured_val > limit)
> 		dev_info(foo)
> } else
> 	dev_alert_once()
> 
> Maybe something like this:
> 
>  drivers/platform/x86/intel_ips.c |  160 +++++++++++++++++++++++++++++++++-----
>  1 files changed, 140 insertions(+), 20 deletions(-)

Yes, we do need something like this.  It turns out the BIOS can
optionally program several of these values, and we need to sanity check
them.  If they're not valid (e.g. a core power limit of 0 or MCP temp
limit of 0xffff) we need to use the default values in the limit structs.

I think the programmed limits are valid if they're nonzero and less
than one of the available default limits.  If they're not valid, we
should just use the default values.  I was thinking something like the
below for MCP, but Joe you may want to just update your patch instead
since it's more complete.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
index 9024480..ec72e80 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -662,6 +662,17 @@ static bool mch_exceeded(struct ips_driver *ips)
 	return ret;
 }
 
+static void clamp_mcp_temp_limit(struct ips_driver *ips)
+{
+	/*
+	 * BIOS may or may not program an MCP limit.  Clamp it to the
+	 * lowest available value.
+	 */
+	if (ips->mcp_temp_limit < ips->core_temp_limit ||
+	    ips->mcp_temp_limit < ips->mch_temp_limit)
+		ips->mcp_temp_limit = min(ips->core_temp_limit, ips->mch_temp_limit);
+}
+
 /**
  * update_turbo_limits - get various limits & settings from regs
  * @ips: IPS driver struct
@@ -686,6 +697,7 @@ static void update_turbo_limits(struct ips_driver *ips)
 	ips->mcp_temp_limit = thm_readw(THM_PTL);
 	ips->mcp_power_limit = thm_readw(THM_MPPC);
 
+	clamp_mcp_temp_limit(ips);
 	/* Ignore BIOS CPU vs GPU pref */
 }
 
@@ -1155,6 +1167,7 @@ static irqreturn_t ips_irq_handler(int irq, void *arg)
 				STS_PTL_SHIFT;
 			ips->mcp_power_limit = (tc1 & STS_PPL_MASK) >>
 				STS_PPL_SHIFT;
+			clamp_mcp_temp_limit(ips);
 			spin_unlock(&ips->turbo_status_lock);
 
 			thm_writeb(THM_SEC, SEC_ACK);