From bdff938d04409aba0e43e408ee47d45d1486b2ae Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Thu, 24 Mar 2022 12:09:48 -0700 Subject: [PATCH 1/9] thermal: int340x: Clean up unnecessary acpi_buffer pointer freeing It is the caller's responsibility to free only upon ACPI_SUCCESS. Signed-off-by: Davidlohr Bueso Acked-by: Zhang Rui [ rjw: Subject edits ] Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index 4954800b9850..0e7931c286ec 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -186,11 +186,11 @@ static int int3400_thermal_run_osc(acpi_handle handle, char *uuid_str, int *enab ret = *((u32 *)(context.ret.pointer + 4)); if (ret != *enable) result = -EPERM; + + kfree(context.ret.pointer); } else result = -EPERM; - kfree(context.ret.pointer); - return result; } From 9e5d3d6be66472037c4b20b7d2b43e916207ab77 Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Thu, 24 Mar 2022 12:09:49 -0700 Subject: [PATCH 2/9] thermal: int340x: Consolidate freeing of acpi_buffer pointer Introduce a single point of freeing/exit after ensuring no error in int3400_setup_gddv(). Signed-off-by: Davidlohr Bueso Acked-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- .../thermal/intel/int340x_thermal/int3400_thermal.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index 0e7931c286ec..e299873d50b8 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -508,21 +508,18 @@ static void int3400_setup_gddv(struct int3400_thermal_priv *priv) obj = buffer.pointer; if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 1 - || obj->package.elements[0].type != ACPI_TYPE_BUFFER) { - kfree(buffer.pointer); - return; - } + || obj->package.elements[0].type != ACPI_TYPE_BUFFER) + goto out_free; priv->data_vault = kmemdup(obj->package.elements[0].buffer.pointer, obj->package.elements[0].buffer.length, GFP_KERNEL); - if (!priv->data_vault) { - kfree(buffer.pointer); - return; - } + if (!priv->data_vault) + goto out_free; bin_attr_data_vault.private = priv->data_vault; bin_attr_data_vault.size = obj->package.elements[0].buffer.length; +out_free: kfree(buffer.pointer); } From ad47f8343a96cc1ebd3654a645d86c63eaeefb39 Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Thu, 24 Mar 2022 12:09:50 -0700 Subject: [PATCH 3/9] thermal: int340x: Clean up _OSC context init Now that the UUID is already sanitized by the caller, lets trivially clean up some of the context arming. Signed-off-by: Davidlohr Bueso Acked-by: Zhang Rui [ rjw: Subject edits ] Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index e299873d50b8..4c49a6c9d4df 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -169,18 +169,15 @@ static int int3400_thermal_run_osc(acpi_handle handle, char *uuid_str, int *enab acpi_status status; int result = 0; struct acpi_osc_context context = { - .uuid_str = NULL, + .uuid_str = uuid_str, .rev = 1, .cap.length = 8, + .cap.pointer = buf, }; - context.uuid_str = uuid_str; - buf[OSC_QUERY_DWORD] = 0; buf[OSC_SUPPORT_DWORD] = *enable; - context.cap.pointer = buf; - status = acpi_run_osc(handle, &context); if (ACPI_SUCCESS(status)) { ret = *((u32 *)(context.ret.pointer + 4)); From 7bb732fea3944171afa45b4a71298f822589c8a6 Mon Sep 17 00:00:00 2001 From: Hesham Almatary Date: Thu, 24 Mar 2022 10:34:43 +0000 Subject: [PATCH 4/9] thermal: hisi_termal: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr() Cleaning up the driver to use pm_sleep_ptr() macro instead of #ifdef guards is simpler and allows the compiler to remove those functions if built without CONFIG_PM_SLEEP support. Signed-off-by: Hesham Almatary Reviewed-by: Paul Cercueil Signed-off-by: Rafael J. Wysocki --- drivers/thermal/hisi_thermal.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c index 9a21ac0ceb11..b29ab09040d5 100644 --- a/drivers/thermal/hisi_thermal.c +++ b/drivers/thermal/hisi_thermal.c @@ -629,7 +629,6 @@ static int hisi_thermal_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM_SLEEP static int hisi_thermal_suspend(struct device *dev) { struct hisi_thermal_data *data = dev_get_drvdata(dev); @@ -651,15 +650,14 @@ static int hisi_thermal_resume(struct device *dev) return ret; } -#endif -static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops, +static DEFINE_SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops, hisi_thermal_suspend, hisi_thermal_resume); static struct platform_driver hisi_thermal_driver = { .driver = { .name = "hisi_thermal", - .pm = &hisi_thermal_pm_ops, + .pm = pm_sleep_ptr(&hisi_thermal_pm_ops), .of_match_table = of_hisi_thermal_match, }, .probe = hisi_thermal_probe, From b947769b8f778db130aad834257fcaca25df2edc Mon Sep 17 00:00:00 2001 From: Kant Fan Date: Fri, 25 Mar 2022 15:30:30 +0800 Subject: [PATCH 5/9] thermal: devfreq_cooling: use local ops instead of global ops Fix access illegal address problem in following condition: There are multiple devfreq cooling devices in system, some of them has EM model but others do not. Energy model ops such as state2power will append to global devfreq_cooling_ops when the cooling device with EM model is registered. It makes the cooling device without EM model also use devfreq_cooling_ops after appending when registered later by of_devfreq_cooling_register_power() or of_devfreq_cooling_register(). The IPA governor regards the cooling devices without EM model as a power actor, because they also have energy model ops, and will access illegal address at dfc->em_pd when execute cdev->ops->get_requested_power, cdev->ops->state2power or cdev->ops->power2state. Fixes: 615510fe13bd2 ("thermal: devfreq_cooling: remove old power model and use EM") Cc: 5.13+ # 5.13+ Signed-off-by: Kant Fan Reviewed-by: Lukasz Luba Signed-off-by: Rafael J. Wysocki --- drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c index 4310cb342a9f..d38a80adec73 100644 --- a/drivers/thermal/devfreq_cooling.c +++ b/drivers/thermal/devfreq_cooling.c @@ -358,21 +358,28 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df, struct thermal_cooling_device *cdev; struct device *dev = df->dev.parent; struct devfreq_cooling_device *dfc; + struct thermal_cooling_device_ops *ops; char *name; int err, num_opps; - dfc = kzalloc(sizeof(*dfc), GFP_KERNEL); - if (!dfc) + ops = kmemdup(&devfreq_cooling_ops, sizeof(*ops), GFP_KERNEL); + if (!ops) return ERR_PTR(-ENOMEM); + dfc = kzalloc(sizeof(*dfc), GFP_KERNEL); + if (!dfc) { + err = -ENOMEM; + goto free_ops; + } + dfc->devfreq = df; dfc->em_pd = em_pd_get(dev); if (dfc->em_pd) { - devfreq_cooling_ops.get_requested_power = + ops->get_requested_power = devfreq_cooling_get_requested_power; - devfreq_cooling_ops.state2power = devfreq_cooling_state2power; - devfreq_cooling_ops.power2state = devfreq_cooling_power2state; + ops->state2power = devfreq_cooling_state2power; + ops->power2state = devfreq_cooling_power2state; dfc->power_ops = dfc_power; @@ -407,8 +414,7 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df, if (!name) goto remove_qos_req; - cdev = thermal_of_cooling_device_register(np, name, dfc, - &devfreq_cooling_ops); + cdev = thermal_of_cooling_device_register(np, name, dfc, ops); kfree(name); if (IS_ERR(cdev)) { @@ -429,6 +435,8 @@ free_table: kfree(dfc->freq_table); free_dfc: kfree(dfc); +free_ops: + kfree(ops); return ERR_PTR(err); } @@ -510,11 +518,13 @@ EXPORT_SYMBOL_GPL(devfreq_cooling_em_register); void devfreq_cooling_unregister(struct thermal_cooling_device *cdev) { struct devfreq_cooling_device *dfc; + const struct thermal_cooling_device_ops *ops; struct device *dev; if (IS_ERR_OR_NULL(cdev)) return; + ops = cdev->ops; dfc = cdev->devdata; dev = dfc->devfreq->dev.parent; @@ -525,5 +535,6 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev) kfree(dfc->freq_table); kfree(dfc); + kfree(ops); } EXPORT_SYMBOL_GPL(devfreq_cooling_unregister); From 55266546f45dcc9284ca93f4fa3f6693d30ea8ce Mon Sep 17 00:00:00 2001 From: Zhang Rui Date: Thu, 19 May 2022 22:35:05 +0800 Subject: [PATCH 6/9] PM: wakeup: expose pm_wakeup_pending to modules intel_pch_thermal driver needs a long delay to cool itself (60 seconds in maximum) during suspend. When a wakeup event occures during the delay, it is better for the intel_pch_thermal driver to detect this and quit cooling because the suspend is likely to abort anyway. Thus expose pm_wakeup_pending to modules so that intel_pch_thermal driver can be aware of the wakeup events. Signed-off-by: Zhang Rui Tested-by: Sumeet Pawnikar Signed-off-by: Rafael J. Wysocki --- drivers/base/power/wakeup.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index a57d469676ca..11a4ffe91367 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -930,6 +930,7 @@ bool pm_wakeup_pending(void) return ret || atomic_read(&pm_abort_suspend) > 0; } +EXPORT_SYMBOL_GPL(pm_wakeup_pending); void pm_system_wakeup(void) { From 28708e19370013d819a511e76a0a85ae10f1be15 Mon Sep 17 00:00:00 2001 From: Zhang Rui Date: Thu, 19 May 2022 22:35:06 +0800 Subject: [PATCH 7/9] thermal: intel: pch: move cooling delay to suspend_noirq phase Move the PCH Thermal driver suspend callback to suspend_noirq to do cooling while the system is more quiescent. Signed-off-by: Zhang Rui Tested-by: Sumeet Pawnikar Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_pch_thermal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c index 527c91f5960b..c0f651b5905d 100644 --- a/drivers/thermal/intel/intel_pch_thermal.c +++ b/drivers/thermal/intel/intel_pch_thermal.c @@ -193,6 +193,7 @@ 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; @@ -455,7 +456,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 +496,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, .resume = intel_pch_thermal_resume, }; From 92923028e9795055debc460243cc246cc9284cd1 Mon Sep 17 00:00:00 2001 From: Zhang Rui Date: Thu, 19 May 2022 22:35:07 +0800 Subject: [PATCH 8/9] thermal: intel: pch: enhance overheat handling 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. At the same time, as the cooling delay can be much longer and many wakeup events (ACPI Power Button press, USB mouse move, etc) becomes valid in the suspend_noirq phase, add detection of wakeup event so that the driver does not delay blindly when the system suspend is likely to abort soon. This patch 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 Tested-by: Sumeet Pawnikar Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_pch_thermal.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c index c0f651b5905d..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."); @@ -197,7 +197,7 @@ static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp) 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 */ @@ -234,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, @@ -246,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); From bd30d075eedce159fd5a42e772b903facaa3c47c Mon Sep 17 00:00:00 2001 From: Zhang Rui Date: Thu, 19 May 2022 22:35:08 +0800 Subject: [PATCH 9/9] thermal: intel: pch: improve the cooling delay log Previously, during suspend, intel_pch_thermal driver logs for every cooling iteration, about the current PCH temperature and number of cooling iterations that have been tried, like below [ 100.955526] intel_pch_thermal 0000:00:14.2: CPU-PCH current temp [53C] higher than the threshold temp [50C], sleep 1 times for 100 ms duration [ 101.064156] intel_pch_thermal 0000:00:14.2: CPU-PCH current temp [53C] higher than the threshold temp [50C], sleep 2 times for 100 ms duration After changing the default delay_cnt to 600, in practice, it is common to see tens of the above messages if the system is suspended when PCH overheats. Thus, change this log message from dev_warn to dev_dbg because it is only useful when we want to check the temperature trend. At the same time, there is always a one-line message given by the driver with the patch applied, with below four possibilities. 1. PCH is cool, no cooling delay needed [ 1791.902853] intel_pch_thermal 0000:00:12.0: CPU-PCH is cool [48C] 2. PCH overheats and becomes cool after the cooling delays [ 1475.511617] intel_pch_thermal 0000:00:12.0: CPU-PCH is cool [49C] after 30700 ms delay 3. PCH still overheats after the overall cooling timeout [ 2250.157487] intel_pch_thermal 0000:00:12.0: CPU-PCH is hot [60C] after 60000 ms delay. S0ix might fail 4. PCH aborts cooling because of wakeup event detected during the delay [ 1933.639509] intel_pch_thermal 0000:00:12.0: Wakeup event detected, abort cooling Signed-off-by: Zhang Rui Tested-by: Sumeet Pawnikar Signed-off-by: Rafael J. Wysocki --- drivers/thermal/intel/intel_pch_thermal.c | 31 +++++++++++++++-------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c index b7b32e2f5ae2..c1fa2b29b153 100644 --- a/drivers/thermal/intel/intel_pch_thermal.c +++ b/drivers/thermal/intel/intel_pch_thermal.c @@ -197,7 +197,7 @@ static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp) static int pch_wpt_suspend(struct pch_thermal_device *ptd) { u8 tsel; - int pch_delay_cnt = 1; + int pch_delay_cnt = 0; u16 pch_thr_temp, pch_cur_temp; /* Shutdown the thermal sensor if it is not enabled by BIOS */ @@ -233,29 +233,38 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd) * temperature stays above threshold, notify the warning message * which helps to indentify the reason why S0ix entry was rejected. */ - while (pch_delay_cnt <= delay_cnt) { + while (pch_delay_cnt < delay_cnt) { if (pch_cur_temp < pch_thr_temp) break; - if (pm_wakeup_pending()) - break; + if (pm_wakeup_pending()) { + dev_warn(&ptd->pdev->dev, "Wakeup event detected, abort cooling\n"); + return 0; + } - dev_warn(&ptd->pdev->dev, + pch_delay_cnt++; + dev_dbg(&ptd->pdev->dev, "CPU-PCH current temp [%dC] higher than the threshold temp [%dC], sleep %d times for %d ms duration\n", pch_cur_temp, pch_thr_temp, pch_delay_cnt, delay_timeout); msleep(delay_timeout); /* Read the PCH current temperature for next cycle. */ pch_cur_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP)); - pch_delay_cnt++; } 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); - else - dev_info(&ptd->pdev->dev, - "CPU-PCH is cool [%dC], continue to suspend\n", pch_cur_temp); + "CPU-PCH is hot [%dC] after %d ms delay. S0ix might fail\n", + pch_cur_temp, pch_delay_cnt * delay_timeout); + else { + if (pch_delay_cnt) + dev_info(&ptd->pdev->dev, + "CPU-PCH is cool [%dC] after %d ms delay\n", + pch_cur_temp, pch_delay_cnt * delay_timeout); + else + dev_info(&ptd->pdev->dev, + "CPU-PCH is cool [%dC]\n", + pch_cur_temp); + } return 0; }