[PATCH v2 0/2] thermal: int340x: fix tcc offset on resume

[PATCH v2 0/2] thermal: int340x: fix tcc offset on resume

[Antoine Tenart]

Hi,

This is a follow-up of [1]. As discussed in the initial thread, the fix
is now split into two commits: the two commits combined are fixing the
issue in a nice way for 5.15, but only the first one is meant to be
applied on stable kernels (to avoid conflicts).

The issue this is fixing is described in patch 1. Tl;dr: a
suspend/resume cycle can end-up in setting a wrong tcc offset value,
which can lead to machines shutting down because of overheating (when
they should not).

I've stripped the Acked-by/Tested-by flags from v1 as the patch was
split. But the end result for 5.15+ kernels should be the same.

Thanks,
Antoine

[1] https://lore.kernel.org/linux-pm/20210908161632.15520-1-atenart@kernel.org/T/

Antoine Tenart (2):
  thermal: int340x: do not set a wrong tcc offset on resume
  thermal: int340x: improve the tcc offset saving for suspend/resume

 .../intel/int340x_thermal/int3401_thermal.c   |  8 +++-
 .../processor_thermal_device.c                | 39 ++++++++++++++-----
 .../processor_thermal_device.h                |  1 +
 .../processor_thermal_device_pci.c            | 18 ++++++++-
 .../processor_thermal_device_pci_legacy.c     |  8 +++-
 5 files changed, 62 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Antoine Tenart]

After upgrading to Linux 5.13.3 I noticed my laptop would shutdown due
to overheat (when it should not). It turned out this was due to commit
fe6a6de6692e ("thermal/drivers/int340x/processor_thermal: Fix tcc setting").

What happens is this drivers uses a global variable to keep track of the
tcc offset (tcc_offset_save) and uses it on resume. The issue is this
variable is initialized to 0, but is only set in
tcc_offset_degree_celsius_store, i.e. when the tcc offset is explicitly
set by userspace. If that does not happen, the resume path will set the
offset to 0 (in my case the h/w default being 3, the offset would become
too low after a suspend/resume cycle).

The issue did not arise before commit fe6a6de6692e, as the function
setting the offset would return if the offset was 0. This is no longer
the case (rightfully).

Fix this by not applying the offset if it wasn't saved before, reverting
back to the old logic. A better approach will come later, but this will
be easier to apply to stable kernels.

The logic to restore the offset after a resume was there long before
commit fe6a6de6692e, but as a value of 0 was considered invalid I'm
referencing the commit that made the issue possible in the Fixes tag
instead.

Fixes: fe6a6de6692e ("thermal/drivers/int340x/processor_thermal: Fix tcc setting")
Cc: stable@vger.kernel.org
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 .../thermal/intel/int340x_thermal/processor_thermal_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index 0f0038af2ad4..fb64acfd5e07 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -107,7 +107,7 @@ static int tcc_offset_update(unsigned int tcc)
 	return 0;
 }
 
-static unsigned int tcc_offset_save;
+static int tcc_offset_save = -1;
 
 static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
@@ -352,7 +352,8 @@ int proc_thermal_resume(struct device *dev)
 	proc_dev = dev_get_drvdata(dev);
 	proc_thermal_read_ppcc(proc_dev);
 
-	tcc_offset_update(tcc_offset_save);
+	if (tcc_offset_save >= 0)
+		tcc_offset_update(tcc_offset_save);
 
 	return 0;
 }
-- 
2.31.1

[PATCH v2 2/2] thermal: int340x: improve the tcc offset saving for suspend/resume

[Antoine Tenart]

When the driver resumes, the tcc offset is set back to its previous
value. But this only works if the value was user defined as otherwise
the offset isn't saved. This asymmetric logic is harder to maintain and
introduced some issues.

Improve the logic by saving the tcc offset in a suspend op, so the right
value is always restored after a resume.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 .../intel/int340x_thermal/int3401_thermal.c   |  8 ++++-
 .../processor_thermal_device.c                | 36 ++++++++++++++-----
 .../processor_thermal_device.h                |  1 +
 .../processor_thermal_device_pci.c            | 18 +++++++++-
 .../processor_thermal_device_pci_legacy.c     |  8 ++++-
 5 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int3401_thermal.c b/drivers/thermal/intel/int340x_thermal/int3401_thermal.c
index acebc8ba94e2..217786fba185 100644
--- a/drivers/thermal/intel/int340x_thermal/int3401_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3401_thermal.c
@@ -44,15 +44,21 @@ static int int3401_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int int3401_thermal_suspend(struct device *dev)
+{
+	return proc_thermal_suspend(dev);
+}
 static int int3401_thermal_resume(struct device *dev)
 {
 	return proc_thermal_resume(dev);
 }
 #else
+#define int3401_thermal_suspend NULL
 #define int3401_thermal_resume NULL
 #endif
 
-static SIMPLE_DEV_PM_OPS(int3401_proc_thermal_pm, NULL, int3401_thermal_resume);
+static SIMPLE_DEV_PM_OPS(int3401_proc_thermal_pm, int3401_thermal_suspend,
+			 int3401_thermal_resume);
 
 static struct platform_driver int3401_driver = {
 	.probe = int3401_add,
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index fb64acfd5e07..a8d98f1bd6c6 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -68,8 +68,7 @@ static const struct attribute_group power_limit_attribute_group = {
 	.name = "power_limits"
 };
 
-static ssize_t tcc_offset_degree_celsius_show(struct device *dev,
-			       struct device_attribute *attr, char *buf)
+static int tcc_get_offset(void)
 {
 	u64 val;
 	int err;
@@ -78,8 +77,20 @@ static ssize_t tcc_offset_degree_celsius_show(struct device *dev,
 	if (err)
 		return err;
 
-	val = (val >> 24) & 0x3f;
-	return sprintf(buf, "%d\n", (int)val);
+	return (val >> 24) & 0x3f;
+}
+
+static ssize_t tcc_offset_degree_celsius_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	int tcc;
+
+	tcc = tcc_get_offset();
+	if (tcc < 0)
+		return tcc;
+
+	return sprintf(buf, "%d\n", tcc);
 }
 
 static int tcc_offset_update(unsigned int tcc)
@@ -107,8 +118,6 @@ static int tcc_offset_update(unsigned int tcc)
 	return 0;
 }
 
-static int tcc_offset_save = -1;
-
 static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -131,8 +140,6 @@ static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
 	if (err)
 		return err;
 
-	tcc_offset_save = tcc;
-
 	return count;
 }
 
@@ -345,6 +352,18 @@ void proc_thermal_remove(struct proc_thermal_device *proc_priv)
 }
 EXPORT_SYMBOL_GPL(proc_thermal_remove);
 
+static int tcc_offset_save = -1;
+
+int proc_thermal_suspend(struct device *dev)
+{
+	tcc_offset_save = tcc_get_offset();
+	if (tcc_offset_save < 0)
+		dev_warn(dev, "failed to save offset (%d)\n", tcc_offset_save);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(proc_thermal_suspend);
+
 int proc_thermal_resume(struct device *dev)
 {
 	struct proc_thermal_device *proc_dev;
@@ -352,6 +371,7 @@ int proc_thermal_resume(struct device *dev)
 	proc_dev = dev_get_drvdata(dev);
 	proc_thermal_read_ppcc(proc_dev);
 
+	/* Do not update if saving failed */
 	if (tcc_offset_save >= 0)
 		tcc_offset_update(tcc_offset_save);
 
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
index 5a1cfe4864f1..c1d8de6dc3d1 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
@@ -83,6 +83,7 @@ void proc_thermal_mbox_remove(struct pci_dev *pdev);
 int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16 cmd_id, u32 cmd_data, u32 *cmd_resp);
 int proc_thermal_add(struct device *dev, struct proc_thermal_device *priv);
 void proc_thermal_remove(struct proc_thermal_device *proc_priv);
+int proc_thermal_suspend(struct device *dev);
 int proc_thermal_resume(struct device *dev);
 int proc_thermal_mmio_add(struct pci_dev *pdev,
 			  struct proc_thermal_device *proc_priv,
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
index 11dd2e825f4f..b4bcd3fe9eb2 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
@@ -314,6 +314,20 @@ static void proc_thermal_pci_remove(struct pci_dev *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int proc_thermal_pci_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct proc_thermal_device *proc_priv;
+	struct proc_thermal_pci *pci_info;
+
+	proc_priv = pci_get_drvdata(pdev);
+	pci_info = proc_priv->priv_data;
+
+	if (!pci_info->no_legacy)
+		return proc_thermal_suspend(dev);
+
+	return 0;
+}
 static int proc_thermal_pci_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -335,10 +349,12 @@ static int proc_thermal_pci_resume(struct device *dev)
 	return 0;
 }
 #else
+#define proc_thermal_pci_suspend NULL
 #define proc_thermal_pci_resume NULL
 #endif
 
-static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm, NULL, proc_thermal_pci_resume);
+static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm, proc_thermal_pci_suspend,
+			 proc_thermal_pci_resume);
 
 static const struct pci_device_id proc_thermal_pci_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, ADL_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci_legacy.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci_legacy.c
index f5fc1791b11e..4571a1a53b84 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci_legacy.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci_legacy.c
@@ -107,15 +107,21 @@ static void proc_thermal_pci_remove(struct pci_dev *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int proc_thermal_pci_suspend(struct device *dev)
+{
+	return proc_thermal_suspend(dev);
+}
 static int proc_thermal_pci_resume(struct device *dev)
 {
 	return proc_thermal_resume(dev);
 }
 #else
+#define proc_thermal_pci_suspend NULL
 #define proc_thermal_pci_resume NULL
 #endif
 
-static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm, NULL, proc_thermal_pci_resume);
+static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm, proc_thermal_pci_suspend,
+			 proc_thermal_pci_resume);
 
 static const struct pci_device_id proc_thermal_pci_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, BDW_THERMAL, 0) },
-- 
2.31.1

Re: [PATCH v2 0/2] thermal: int340x: fix tcc offset on resume

[Srinivas Pandruvada]

On Thu, 2021-09-09 at 10:56 +0200, Antoine Tenart wrote:
> Hi,
> 
> This is a follow-up of [1]. As discussed in the initial thread, the fix
> is now split into two commits: the two commits combined are fixing the
> issue in a nice way for 5.15, but only the first one is meant to be
> applied on stable kernels (to avoid conflicts).
> 
> The issue this is fixing is described in patch 1. Tl;dr: a
> suspend/resume cycle can end-up in setting a wrong tcc offset value,
> which can lead to machines shutting down because of overheating (when
> they should not).
> 
> I've stripped the Acked-by/Tested-by flags from v1 as the patch was
> split. But the end result for 5.15+ kernels should be the same.
> 
> Thanks,
> Antoine
> 
> [1] 
> https://lore.kernel.org/linux-pm/20210908161632.15520-1-atenart@kernel.org/T/
> 
> Antoine Tenart (2):
>   thermal: int340x: do not set a wrong tcc offset on resume
>   thermal: int340x: improve the tcc offset saving for suspend/resume
> 
>  .../intel/int340x_thermal/int3401_thermal.c   |  8 +++-
>  .../processor_thermal_device.c                | 39 ++++++++++++++-----
>  .../processor_thermal_device.h                |  1 +
>  .../processor_thermal_device_pci.c            | 18 ++++++++-
>  .../processor_thermal_device_pci_legacy.c     |  8 +++-
>  5 files changed, 62 insertions(+), 12 deletions(-)
> 
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Tested-by: Srinivas Pandruvada <srinivas.pI andruvada@linux.intel.com>

Re: [PATCH v2 0/2] thermal: int340x: fix tcc offset on resume

[Daniel Lezcano]

Applied, thanks

On 09/09/2021 10:56, Antoine Tenart wrote:
> Hi,
> 
> This is a follow-up of [1]. As discussed in the initial thread, the fix
> is now split into two commits: the two commits combined are fixing the
> issue in a nice way for 5.15, but only the first one is meant to be
> applied on stable kernels (to avoid conflicts).
> 
> The issue this is fixing is described in patch 1. Tl;dr: a
> suspend/resume cycle can end-up in setting a wrong tcc offset value,
> which can lead to machines shutting down because of overheating (when
> they should not).
> 
> I've stripped the Acked-by/Tested-by flags from v1 as the patch was
> split. But the end result for 5.15+ kernels should be the same.
> 
> Thanks,
> Antoine
> 
> [1] https://lore.kernel.org/linux-pm/20210908161632.15520-1-atenart@kernel.org/T/
> 
> Antoine Tenart (2):
>   thermal: int340x: do not set a wrong tcc offset on resume
>   thermal: int340x: improve the tcc offset saving for suspend/resume
> 
>  .../intel/int340x_thermal/int3401_thermal.c   |  8 +++-
>  .../processor_thermal_device.c                | 39 ++++++++++++++-----
>  .../processor_thermal_device.h                |  1 +
>  .../processor_thermal_device_pci.c            | 18 ++++++++-
>  .../processor_thermal_device_pci_legacy.c     |  8 +++-
>  5 files changed, 62 insertions(+), 12 deletions(-)
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Srinivas Pandruvada]

Hi Daniel,

This patch is important. Can we send for 5.15 rc release?

I see the previous version of this patch is applied to linux-next.
But this series is better as it splits into two patches. The first one
can be easily backported and will fix the problem. The second one is an
improvement.

Thanks,
Srinivas

On Thu, 2021-09-09 at 10:56 +0200, Antoine Tenart wrote:
> After upgrading to Linux 5.13.3 I noticed my laptop would shutdown
> due
> to overheat (when it should not). It turned out this was due to
> commit
> fe6a6de6692e ("thermal/drivers/int340x/processor_thermal: Fix tcc
> setting").
> 
> What happens is this drivers uses a global variable to keep track of
> the
> tcc offset (tcc_offset_save) and uses it on resume. The issue is this
> variable is initialized to 0, but is only set in
> tcc_offset_degree_celsius_store, i.e. when the tcc offset is
> explicitly
> set by userspace. If that does not happen, the resume path will set
> the
> offset to 0 (in my case the h/w default being 3, the offset would
> become
> too low after a suspend/resume cycle).
> 
> The issue did not arise before commit fe6a6de6692e, as the function
> setting the offset would return if the offset was 0. This is no
> longer
> the case (rightfully).
> 
> Fix this by not applying the offset if it wasn't saved before,
> reverting
> back to the old logic. A better approach will come later, but this
> will
> be easier to apply to stable kernels.
> 
> The logic to restore the offset after a resume was there long before
> commit fe6a6de6692e, but as a value of 0 was considered invalid I'm
> referencing the commit that made the issue possible in the Fixes tag
> instead.
> 
> Fixes: fe6a6de6692e ("thermal/drivers/int340x/processor_thermal: Fix
> tcc setting")
> Cc: stable@vger.kernel.org
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>
> ---
>  .../thermal/intel/int340x_thermal/processor_thermal_device.c | 5
> +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> index 0f0038af2ad4..fb64acfd5e07 100644
> ---
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> +++
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> @@ -107,7 +107,7 @@ static int tcc_offset_update(unsigned int tcc)
>         return 0;
>  }
>  
> -static unsigned int tcc_offset_save;
> +static int tcc_offset_save = -1;
>  
>  static ssize_t tcc_offset_degree_celsius_store(struct device *dev,
>                                 struct device_attribute *attr, const
> char *buf,
> @@ -352,7 +352,8 @@ int proc_thermal_resume(struct device *dev)
>         proc_dev = dev_get_drvdata(dev);
>         proc_thermal_read_ppcc(proc_dev);
>  
> -       tcc_offset_update(tcc_offset_save);
> +       if (tcc_offset_save >= 0)
> +               tcc_offset_update(tcc_offset_save);
>  
>         return 0;
>  }

Re: [PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Daniel Lezcano]

On 24/09/2021 18:27, Srinivas Pandruvada wrote:
> Hi Daniel,
> 
> This patch is important. Can we send for 5.15 rc release?
> 
> I see the previous version of this patch is applied to linux-next.
> But this series is better as it splits into two patches. The first one
> can be easily backported and will fix the problem. The second one is an
> improvement.

Yes, it is in the pipe.

I've applied the patch 1/2 to the fixes branch and the patch 2/2 will
land in the next branch as soon as the next -rc is released with the fix
and merged to the next branch.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Srinivas Pandruvada]

On Fri, 2021-09-24 at 19:40 +0200, Daniel Lezcano wrote:
> On 24/09/2021 18:27, Srinivas Pandruvada wrote:
> > Hi Daniel,
> > 
> > This patch is important. Can we send for 5.15 rc release?
> > 
> > I see the previous version of this patch is applied to linux-next.
> > But this series is better as it splits into two patches. The first
> > one
> > can be easily backported and will fix the problem. The second one
> > is an
> > improvement.
> 
> Yes, it is in the pipe.
> 
> I've applied the patch 1/2 to the fixes branch and the patch 2/2 will
> land in the next branch as soon as the next -rc is released with the
> fix
> and merged to the next branch.

Thanks Daniel.

-Srinivas

> 
> 

Re: [PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Antoine Tenart]

Hello Daniel,

Quoting Daniel Lezcano (2021-09-24 19:40:13)
> 
> I've applied the patch 1/2 to the fixes branch and the patch 2/2 will
> land in the next branch as soon as the next -rc is released with the fix
> and merged to the next branch.

I don't see it in thermal/next even though patch 1 has made it. Not sure
if patch 2 has slipped through the cracks or wasn't pushed yet. If it's
the later, please ignore this mail.

Thanks!
Antoine

Re: [PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Daniel Lezcano]

On 20/10/2021 15:38, Antoine Tenart wrote:
> Hello Daniel,
> 
> Quoting Daniel Lezcano (2021-09-24 19:40:13)
>>
>> I've applied the patch 1/2 to the fixes branch and the patch 2/2 will
>> land in the next branch as soon as the next -rc is released with the fix
>> and merged to the next branch.
> 
> I don't see it in thermal/next even though patch 1 has made it. Not sure
> if patch 2 has slipped through the cracks or wasn't pushed yet. If it's
> the later, please ignore this mail.

Indeed, I thougth I picked it but it wasn't.

Thanks for the head up, it is applied now.

  -- D.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 1/2] thermal: int340x: do not set a wrong tcc offset on resume

[Antoine Tenart]

Quoting Daniel Lezcano (2021-10-21 11:47:50)
> On 20/10/2021 15:38, Antoine Tenart wrote:
> > Quoting Daniel Lezcano (2021-09-24 19:40:13)
> >>
> >> I've applied the patch 1/2 to the fixes branch and the patch 2/2 will
> >> land in the next branch as soon as the next -rc is released with the fix
> >> and merged to the next branch.
> > 
> > I don't see it in thermal/next even though patch 1 has made it. Not sure
> > if patch 2 has slipped through the cracks or wasn't pushed yet. If it's
> > the later, please ignore this mail.
> 
> Indeed, I thougth I picked it but it wasn't.
> 
> Thanks for the head up, it is applied now.

Thanks!