Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220505015814.3727692-1-rui.zhang@intel.com> <20220505015814.3727692-3-rui.zhang@intel.com>
In-Reply-To: <20220505015814.3727692-3-rui.zhang@intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 17 May 2022 17:02:14 +0200
Message-ID: <CAJZ5v0ikGkBqT1FNxGv3Y2guMRhb0su+qioGee=eo22NS+xErQ@mail.gmail.com>
Subject: Re: [PATCH 2/7] thermal: intel: pch: enhance overheat handling
To:     Zhang Rui <rui.zhang@intel.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>, kvalo@kernel.org,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Linux PM <linux-pm@vger.kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        linux-rtc@vger.kernel.org,
        "open list:NETWORKING DRIVERS (WIRELESS)" 
        <linux-wireless@vger.kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        merez@codeaurora.org, mat.jonczyk@o2.pl,
        Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>,
        Len Brown <len.brown@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, May 5, 2022 at 3:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> Commit ef63b043ac86 ("thermal: intel: pch: fix S0ix failure due to PCH
> temperature above threshold") introduces delay loop mechanism that allows
> PCH temperature to go down below threshold during suspend so it won't
> block S0ix. And the default overall delay timeout is 1 second.
>
> However, in practice, we found that the time it takes to cool the PCH down
> below threshold highly depends on the initial PCH temperature when the
> delay starts, as well as the ambient temperature.
> And in some cases, the 1 second delay is not sufficient. As a result, the
> system stays in a shallower power state like PCx instead of S0ix, and
> drains the battery power, without user' notice.
>
> To make sure S0ix is not blocked by the PCH overheating, we
> 1. expand the default overall timeout to 60 seconds.
> 2. make sure the temperature is below threshold rather than equal to it.
> 3. move the delay to .suspend_noirq phase instead, in order to
>    a) do cooling delay with a more quiescent system
>    b) be aware of wakeup events during the long delay, because some wakeup
>       events (ACPI Power button Press, USB mouse, etc) become valid only
>       in .suspend_noirq phase and later.
>
> This may introduce longer suspend time, but only in the cases when the
> system overheats and Linux used to enter a shallower S2idle state, say,
> PCx instead of S0ix.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
> ---
>  drivers/thermal/intel/intel_pch_thermal.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
> index 527c91f5960b..b7b32e2f5ae2 100644
> --- a/drivers/thermal/intel/intel_pch_thermal.c
> +++ b/drivers/thermal/intel/intel_pch_thermal.c
> @@ -70,8 +70,8 @@ static unsigned int delay_timeout = 100;
>  module_param(delay_timeout, int, 0644);
>  MODULE_PARM_DESC(delay_timeout, "amount of time delay for each iteration.");
>
> -/* Number of iterations for cooling delay, 10 counts by default for now */
> -static unsigned int delay_cnt = 10;
> +/* Number of iterations for cooling delay, 600 counts by default for now */
> +static unsigned int delay_cnt = 600;
>  module_param(delay_cnt, int, 0644);
>  MODULE_PARM_DESC(delay_cnt, "total number of iterations for time delay.");
>
> @@ -193,10 +193,11 @@ static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp)
>         return 0;
>  }
>
> +/* Cool the PCH when it's overheat in .suspend_noirq phase */
>  static int pch_wpt_suspend(struct pch_thermal_device *ptd)
>  {
>         u8 tsel;
> -       u8 pch_delay_cnt = 1;
> +       int pch_delay_cnt = 1;
>         u16 pch_thr_temp, pch_cur_temp;
>
>         /* Shutdown the thermal sensor if it is not enabled by BIOS */
> @@ -233,7 +234,10 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
>          * which helps to indentify the reason why S0ix entry was rejected.
>          */
>         while (pch_delay_cnt <= delay_cnt) {
> -               if (pch_cur_temp <= pch_thr_temp)
> +               if (pch_cur_temp < pch_thr_temp)
> +                       break;
> +
> +               if (pm_wakeup_pending())
>                         break;
>
>                 dev_warn(&ptd->pdev->dev,
> @@ -245,7 +249,7 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
>                 pch_delay_cnt++;
>         }
>
> -       if (pch_cur_temp > pch_thr_temp)
> +       if (pch_cur_temp >= pch_thr_temp)
>                 dev_warn(&ptd->pdev->dev,
>                         "CPU-PCH is hot [%dC] even after delay, continue to suspend. S0ix might fail\n",
>                         pch_cur_temp);
> @@ -455,7 +459,7 @@ static void intel_pch_thermal_remove(struct pci_dev *pdev)
>         pci_disable_device(pdev);
>  }
>
> -static int intel_pch_thermal_suspend(struct device *device)
> +static int intel_pch_thermal_suspend_noirq(struct device *device)
>  {
>         struct pch_thermal_device *ptd = dev_get_drvdata(device);
>
> @@ -495,7 +499,7 @@ static const struct pci_device_id intel_pch_thermal_id[] = {
>  MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
>
>  static const struct dev_pm_ops intel_pch_pm_ops = {
> -       .suspend = intel_pch_thermal_suspend,
> +       .suspend_noirq = intel_pch_thermal_suspend_noirq,

IMO it would be better to put this change into a separate patch and
reorder the other changes after this one.  It is valid by itself.

>         .resume = intel_pch_thermal_resume,
>  };
>
> --