[PATCH] Thermal: x86 package temp thermal crash

[PATCH] Thermal: x86 package temp thermal crash

[Srinivas Pandruvada]

On systems with no package MSR support this caused crash as there
is a bug in the logic to check presence of DTHERM and PTS feature
together. Added a change so that when there is no PTS support, module
doesn't get loaded. Even if some CPU comes online with the PTS
feature disabled, and other CPUs has this support, this patch
will still prevent such MSR accesses.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reported-by: Daniel Walker <dwalker@fifo99.com>
---
 drivers/thermal/x86_pkg_temp_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
index 5de56f6..034604c 100644
--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -511,7 +511,7 @@ static int get_core_online(unsigned int cpu)
 
 	/* Check if there is already an instance for this package */
 	if (!phdev) {
-		if (!cpu_has(c, X86_FEATURE_DTHERM) &&
+		if (!cpu_has(c, X86_FEATURE_DTHERM) ||
 					!cpu_has(c, X86_FEATURE_PTS))
 			return -ENODEV;
 		if (pkg_temp_thermal_device_add(cpu))
@@ -562,7 +562,7 @@ static struct notifier_block pkg_temp_thermal_notifier __refdata = {
 };
 
 static const struct x86_cpu_id __initconst pkg_temp_thermal_ids[] = {
-	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTHERM },
+	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_PTS },
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids);
-- 
1.7.11.7

Re: [PATCH] Thermal: x86 package temp thermal crash

[Zhang Rui]

On Thu, 2013-07-11 at 09:50 -0700, Srinivas Pandruvada wrote:
> On systems with no package MSR support this caused crash as there
> is a bug in the logic to check presence of DTHERM and PTS feature
> together. Added a change so that when there is no PTS support, module
> doesn't get loaded. Even if some CPU comes online with the PTS
> feature disabled, and other CPUs has this support, this patch
> will still prevent such MSR accesses.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Reported-by: Daniel Walker <dwalker@fifo99.com>

applied to thermal -next.

thanks,
rui
> ---
>  drivers/thermal/x86_pkg_temp_thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
> index 5de56f6..034604c 100644
> --- a/drivers/thermal/x86_pkg_temp_thermal.c
> +++ b/drivers/thermal/x86_pkg_temp_thermal.c
> @@ -511,7 +511,7 @@ static int get_core_online(unsigned int cpu)
>  
>  	/* Check if there is already an instance for this package */
>  	if (!phdev) {
> -		if (!cpu_has(c, X86_FEATURE_DTHERM) &&
> +		if (!cpu_has(c, X86_FEATURE_DTHERM) ||
>  					!cpu_has(c, X86_FEATURE_PTS))
>  			return -ENODEV;
>  		if (pkg_temp_thermal_device_add(cpu))
> @@ -562,7 +562,7 @@ static struct notifier_block pkg_temp_thermal_notifier __refdata = {
>  };
>  
>  static const struct x86_cpu_id __initconst pkg_temp_thermal_ids[] = {
> -	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTHERM },
> +	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_PTS },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids);

[PATCH] Thermal: x86 package temp thermal crash

[Srinivas Pandruvada]

On some older platform this resulted in general protection fault,
for accessing MSRs, which are not present.
Added change so that the module is loaded, only when the CPU has
PTS feature. The previous check has an issue when DTS sensor is
present but PTS is not supported.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Reported-by: Daniel Walker <dwalker@fifo99.com>
---
 drivers/thermal/x86_pkg_temp_thermal.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
index 5de56f6..cfb6b7d 100644
--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -506,14 +506,10 @@ static int pkg_temp_thermal_device_remove(unsigned int cpu)
 
 static int get_core_online(unsigned int cpu)
 {
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
 
 	/* Check if there is already an instance for this package */
 	if (!phdev) {
-		if (!cpu_has(c, X86_FEATURE_DTHERM) &&
-					!cpu_has(c, X86_FEATURE_PTS))
-			return -ENODEV;
 		if (pkg_temp_thermal_device_add(cpu))
 			return -ENODEV;
 	} else {
@@ -562,7 +558,7 @@ static struct notifier_block pkg_temp_thermal_notifier __refdata = {
 };
 
 static const struct x86_cpu_id __initconst pkg_temp_thermal_ids[] = {
-	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTHERM },
+	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_PTS },
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids);
-- 
1.7.10.4

Re: [PATCH] Thermal: x86 package temp thermal crash

[Daniel Walker]

On Wed, Jul 10, 2013 at 10:49:23PM -0700, Srinivas Pandruvada wrote:
> On some older platform this resulted in general protection fault,
> for accessing MSRs, which are not present.
> Added change so that the module is loaded, only when the CPU has
> PTS feature. The previous check has an issue when DTS sensor is
> present but PTS is not supported.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Reported-by: Daniel Walker <dwalker@fifo99.com>
> ---


Although I should note that my platform is not old by any means.. So
your comments above aren't exactly correct.

Tested-By: Daniel Walker <dwalker@fifo99.com>

Re: [PATCH] Thermal: x86 package temp thermal crash

[Srinivas Pandruvada]

Thanks Daniel.

I will correct the comment and resubmit patch. Sorry for the pain caused 
by me.

Regards,
Srinivas

On 07/11/2013 08:59 AM, Daniel Walker wrote:
> On Wed, Jul 10, 2013 at 10:49:23PM -0700, Srinivas Pandruvada wrote:
>> On some older platform this resulted in general protection fault,
>> for accessing MSRs, which are not present.
>> Added change so that the module is loaded, only when the CPU has
>> PTS feature. The previous check has an issue when DTS sensor is
>> present but PTS is not supported.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> Reported-by: Daniel Walker <dwalker@fifo99.com>
>> ---
>
> Although I should note that my platform is not old by any means.. So
> your comments above aren't exactly correct.
>
> Tested-By: Daniel Walker <dwalker@fifo99.com>
>
>