| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cb827fce-197e-1f33-0127-ba5d49679d65@linaro.org>
Date: Fri, 12 Aug 2022 15:17:30 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Marek Szyprowski <m.szyprowski@...sung.com>, rafael@...nel.org,
Krzysztof Kozlowski <krzk@...nel.org>
Cc: rui.zhang@...el.com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, Amit Kucheria <amitk@...nel.org>,
'Linux Samsung SOC' <linux-samsung-soc@...r.kernel.org>
Subject: Re: [PATCH 5/5] thermal/core: Move the mutex inside the
thermal_zone_device_update() function
Hi Marek,
thanks for reporting.
Are you ok to check if the patch I've sent fixes the issue on your
platform ?
On 12/08/2022 12:39, Marek Szyprowski wrote:
> Hi Daniel,
>
> On 05.08.2022 17:38, Daniel Lezcano wrote:
>> All the different calls inside the thermal_zone_device_update()
>> function take the mutex.
>>
>> The previous changes move the mutex out of the different functions,
>> like the throttling ops. Now that the mutexes are all at the same
>> level in the call stack for the thermal_zone_device_update() function,
>> they can be moved inside this one.
>>
>> That has the benefit of:
>>
>> 1. Simplify the code by not having a plethora of places where the lock is taken
>>
>> 2. Probably closes more race windows because releasing the lock from
>> one line to another can give the opportunity to the thermal zone to change
>> its state in the meantime. For example, the thermal zone can be
>> enabled right after checking it is disabled.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>
> This patch landed recently in linux next-20220811 as commit ca48ad71717d
> ("thermal/core: Move the mutex inside the thermal_zone_device_update()
> function"). Unfortunately it triggers a warning on Samsung ARM/ARM64
> Exynos-based systems during the system suspend/resume cycle:
>
> Restarting tasks ... done.
> random: crng reseeded on system resumption
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1374 at drivers/thermal/thermal_core.c:452
> thermal_zone_device_is_enabled+0x58/0x5c
> Modules linked in: cmac bnep hci_uart btbcm btintel bluetooth s5p_csis
> s5p_fimc exynos4_is_common v4l2_fwnode v4l2_async ecdh_generic ecc
> s5p_mfc brcmfmac cfg80211 s5p_jpeg videobuf2_dma_contig videobuf2_memops
> brcmutil v4l2_mem2mem videobuf2_v4l2 videobuf2_common videodev mc
> CPU: 1 PID: 1374 Comm: rtcwake Not tainted 5.18.0-02136-gca48ad71717d #12560
> Hardware name: Samsung Exynos (Flattened Device Tree)
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x58/0x70
> dump_stack_lvl from __warn+0x230/0x234
> __warn from warn_slowpath_fmt+0xac/0xb4
> warn_slowpath_fmt from thermal_zone_device_is_enabled+0x58/0x5c
> thermal_zone_device_is_enabled from thermal_pm_notify+0x84/0xe8
> thermal_pm_notify from blocking_notifier_call_chain+0x6c/0x94
> blocking_notifier_call_chain from pm_suspend+0x2e8/0x428
> pm_suspend from state_store+0x68/0xc8
> state_store from kernfs_fop_write_iter+0x108/0x1b0
> kernfs_fop_write_iter from vfs_write+0x268/0x50c
> vfs_write from ksys_write+0x54/0xc8
> ksys_write from ret_fast_syscall+0x0/0x1c
> Exception stack(0xf0fa9fa8 to 0xf0fa9ff0)
> ...
> irq event stamp: 49341
> hardirqs last enabled at (49349): [<c019d3f0>] __up_console_sem+0x50/0x60
> hardirqs last disabled at (49358): [<c019d3dc>] __up_console_sem+0x3c/0x60
> softirqs last enabled at (49336): [<c0101808>] __do_softirq+0x4c8/0x5e8
> softirqs last disabled at (49331): [<c01305b8>] irq_exit+0x1cc/0x200
> ---[ end trace 0000000000000000 ]---
> PM: suspend exit
>
>
> It looks that either the exynos_thermal driver or the framework has to
> be somehow adjusted to avoid the above issue, but I didn't check the
> details in the code yet.
>
>
>> ---
>> drivers/thermal/thermal_core.c | 32 +++++--------
>> drivers/thermal/thermal_core.h | 2 +
>> drivers/thermal/thermal_helpers.c | 75 ++++++++++++++++++-------------
>> drivers/thermal/thermal_sysfs.c | 6 ++-
>> 4 files changed, 62 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 9d554f97e081..60110ac53e23 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -297,24 +297,18 @@ static void thermal_zone_device_set_polling(struct thermal_zone_device *tz,
>>
>> static void monitor_thermal_zone(struct thermal_zone_device *tz)
>> {
>> - mutex_lock(&tz->lock);
>> -
>> if (tz->mode != THERMAL_DEVICE_ENABLED)
>> thermal_zone_device_set_polling(tz, 0);
>> else if (tz->passive)
>> thermal_zone_device_set_polling(tz, tz->passive_delay_jiffies);
>> else if (tz->polling_delay_jiffies)
>> thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
>> -
>> - mutex_unlock(&tz->lock);
>> }
>>
>> static void handle_non_critical_trips(struct thermal_zone_device *tz, int trip)
>> {
>> - mutex_lock(&tz->lock);
>> tz->governor ? tz->governor->throttle(tz, trip) :
>> def_governor->throttle(tz, trip);
>> - mutex_unlock(&tz->lock);
>> }
>>
>> void thermal_zone_device_critical(struct thermal_zone_device *tz)
>> @@ -382,7 +376,7 @@ static void update_temperature(struct thermal_zone_device *tz)
>> {
>> int temp, ret;
>>
>> - ret = thermal_zone_get_temp(tz, &temp);
>> + ret = __thermal_zone_get_temp(tz, &temp);
>> if (ret) {
>> if (ret != -EAGAIN)
>> dev_warn(&tz->device,
>> @@ -391,10 +385,8 @@ static void update_temperature(struct thermal_zone_device *tz)
>> return;
>> }
>>
>> - mutex_lock(&tz->lock);
>> tz->last_temperature = tz->temperature;
>> tz->temperature = temp;
>> - mutex_unlock(&tz->lock);
>>
>> trace_thermal_temperature(tz);
>>
>> @@ -457,15 +449,9 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_disable);
>>
>> int thermal_zone_device_is_enabled(struct thermal_zone_device *tz)
>> {
>> - enum thermal_device_mode mode;
>> -
>> - mutex_lock(&tz->lock);
>> -
>> - mode = tz->mode;
>> + lockdep_assert_held(&tz->lock);
>>
>> - mutex_unlock(&tz->lock);
>> -
>> - return mode == THERMAL_DEVICE_ENABLED;
>> + return tz->mode == THERMAL_DEVICE_ENABLED;
>> }
>>
>> void thermal_zone_device_update(struct thermal_zone_device *tz,
>> @@ -473,9 +459,6 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>> {
>> int count;
>>
>> - if (!thermal_zone_device_is_enabled(tz))
>> - return;
>> -
>> if (atomic_read(&in_suspend))
>> return;
>>
>> @@ -483,9 +466,14 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>> "'get_temp' ops set\n", __func__))
>> return;
>>
>> + mutex_lock(&tz->lock);
>> +
>> + if (!thermal_zone_device_is_enabled(tz))
>> + goto out;
>> +
>> update_temperature(tz);
>>
>> - thermal_zone_set_trips(tz);
>> + __thermal_zone_set_trips(tz);
>>
>> tz->notify_event = event;
>>
>> @@ -493,6 +481,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,
>> handle_thermal_trip(tz, count);
>>
>> monitor_thermal_zone(tz);
>> +out:
>> + mutex_unlock(&tz->lock);
>> }
>> EXPORT_SYMBOL_GPL(thermal_zone_device_update);
>>
>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>> index 2241d2dce017..1571917bd3c8 100644
>> --- a/drivers/thermal/thermal_core.h
>> +++ b/drivers/thermal/thermal_core.h
>> @@ -112,6 +112,8 @@ int thermal_build_list_of_policies(char *buf);
>>
>> /* Helpers */
>> void thermal_zone_set_trips(struct thermal_zone_device *tz);
>> +void __thermal_zone_set_trips(struct thermal_zone_device *tz);
>> +int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>>
>> /* sysfs I/F */
>> int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
>> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
>> index 690890f054a3..702c70bdca48 100644
>> --- a/drivers/thermal/thermal_helpers.c
>> +++ b/drivers/thermal/thermal_helpers.c
>> @@ -64,27 +64,17 @@ get_thermal_instance(struct thermal_zone_device *tz,
>> }
>> EXPORT_SYMBOL(get_thermal_instance);
>>
>> -/**
>> - * thermal_zone_get_temp() - returns the temperature of a thermal zone
>> - * @tz: a valid pointer to a struct thermal_zone_device
>> - * @temp: a valid pointer to where to store the resulting temperature.
>> - *
>> - * When a valid thermal zone reference is passed, it will fetch its
>> - * temperature and fill @temp.
>> - *
>> - * Return: On success returns 0, an error code otherwise
>> - */
>> -int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>> +int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>> {
>> int ret = -EINVAL;
>> int count;
>> int crit_temp = INT_MAX;
>> enum thermal_trip_type type;
>>
>> + lockdep_assert_held(&tz->lock);
>> +
>> if (!tz || IS_ERR(tz) || !tz->ops->get_temp)
>> - goto exit;
>> -
>> - mutex_lock(&tz->lock);
>> + return -EINVAL;
>>
>> ret = tz->ops->get_temp(tz, temp);
>>
>> @@ -107,35 +97,42 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>> *temp = tz->emul_temperature;
>> }
>>
>> - mutex_unlock(&tz->lock);
>> -exit:
>> return ret;
>> }
>> -EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>>
>> /**
>> - * thermal_zone_set_trips - Computes the next trip points for the driver
>> - * @tz: a pointer to a thermal zone device structure
>> + * thermal_zone_get_temp() - returns the temperature of a thermal zone
>> + * @tz: a valid pointer to a struct thermal_zone_device
>> + * @temp: a valid pointer to where to store the resulting temperature.
>> *
>> - * The function computes the next temperature boundaries by browsing
>> - * the trip points. The result is the closer low and high trip points
>> - * to the current temperature. These values are passed to the backend
>> - * driver to let it set its own notification mechanism (usually an
>> - * interrupt).
>> + * When a valid thermal zone reference is passed, it will fetch its
>> + * temperature and fill @temp.
>> *
>> - * It does not return a value
>> + * Return: On success returns 0, an error code otherwise
>> */
>> -void thermal_zone_set_trips(struct thermal_zone_device *tz)
>> +int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&tz->lock);
>> + ret = __thermal_zone_get_temp(tz, temp);
>> + mutex_unlock(&tz->lock);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>> +
>> +void __thermal_zone_set_trips(struct thermal_zone_device *tz)
>> {
>> int low = -INT_MAX;
>> int high = INT_MAX;
>> int trip_temp, hysteresis;
>> int i, ret;
>>
>> - mutex_lock(&tz->lock);
>> -
>> + lockdep_assert_held(&tz->lock);
>> +
>> if (!tz->ops->set_trips || !tz->ops->get_trip_hyst)
>> - goto exit;
>> + return;
>>
>> for (i = 0; i < tz->num_trips; i++) {
>> int trip_low;
>> @@ -154,7 +151,7 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz)
>>
>> /* No need to change trip points */
>> if (tz->prev_low_trip == low && tz->prev_high_trip == high)
>> - goto exit;
>> + return;
>>
>> tz->prev_low_trip = low;
>> tz->prev_high_trip = high;
>> @@ -169,8 +166,24 @@ void thermal_zone_set_trips(struct thermal_zone_device *tz)
>> ret = tz->ops->set_trips(tz, low, high);
>> if (ret)
>> dev_err(&tz->device, "Failed to set trips: %d\n", ret);
>> +}
>>
>> -exit:
>> +/**
>> + * thermal_zone_set_trips - Computes the next trip points for the driver
>> + * @tz: a pointer to a thermal zone device structure
>> + *
>> + * The function computes the next temperature boundaries by browsing
>> + * the trip points. The result is the closer low and high trip points
>> + * to the current temperature. These values are passed to the backend
>> + * driver to let it set its own notification mechanism (usually an
>> + * interrupt).
>> + *
>> + * It does not return a value
>> + */
>> +void thermal_zone_set_trips(struct thermal_zone_device *tz)
>> +{
>> + mutex_lock(&tz->lock);
>> + __thermal_zone_set_trips(tz);
>> mutex_unlock(&tz->lock);
>> }
>>
>> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
>> index 3c513561d346..f094f7cbc455 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -49,7 +49,11 @@ static ssize_t
>> mode_show(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> struct thermal_zone_device *tz = to_thermal_zone(dev);
>> - int enabled = thermal_zone_device_is_enabled(tz);
>> + int enabled;
>> +
>> + mutex_lock(&tz->lock);
>> + enabled = thermal_zone_device_is_enabled(tz);
>> + mutex_unlock(&tz->lock);
>>
>> return sprintf(buf, "%s\n", enabled ? "enabled" : "disabled");
>> }
>
> Best regards
--
<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
Powered by blists - more mailing lists