Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend

From: "Chen, Yu C" <yu.c.chen@intel.com>
To: "mingo@kernel.org" <mingo@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Zhang, Rui" <rui.zhang@intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"pavel@ucw.cz" <pavel@ucw.cz>, "x86@kernel.org" <x86@kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>,
	"mingo@redhat.com" <mingo@redhat.com>
Subject: Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
Date: Mon, 17 Aug 2015 11:43:02 +0000	[thread overview]
Message-ID: <1439811991.15722.26.camel@localhost> (raw)
In-Reply-To: <20150817101115.GA27204@gmail.com>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1144 bytes --]

Hi, Ingo, thanks for your review,
On Mon, 2015-08-17 at 12:11 +0200, Ingo Molnar wrote:
> So what your changelog fails to mention:
> 
>  - You only add this code to the 64-bit kernel. Are 32-bit kernels not affected?
Yes, 32-bit kernel should also do the save/restore operation. 
I'll adjust them to 64/32-bit common path. 
> 
>  - the MSR read is done unconditionally. Is MSR_IA32_THERM_CONTROL available
>    architecturally and readable (and has sensible values) on all 64-bit capable
>    x86 CPUs that run this code path?
MSR_IA32_THERM_CONTROL is available on Intel Pentium 4, Xeon, Pentium M and later
processors, so I think not all the 64/32-bit capable x86 CPUs have this
register. Maybe codes like the following would be more reasonable?

save:
ctxt->clock_modulation_saved = !rdmsrl_safe(MSR_IA32_THERM_CONTROL,
	&ctxt->clock_modulation);

restore:
if (ctxt->clock_modulation_saved)
	wrmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);

Thanks a lot.

Best Regards,
Yu
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éÝ¶\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dÊ‡Ú™ë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥