Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cb827fce-197e-1f33-0127-ba5d49679d65@linaro.org>
Date:   Fri, 12 Aug 2022 15:17:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 5/5] thermal/core: Move the mutex inside the
 thermal_zone_device_update() function
Content-Language: en-US
To:     Marek Szyprowski <m.szyprowski@samsung.com>, rafael@kernel.org,
        Krzysztof Kozlowski <krzk@kernel.org>
Cc:     rui.zhang@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Amit Kucheria <amitk@kernel.org>,
        'Linux Samsung SOC' <linux-samsung-soc@vger.kernel.org>
References: <20220805153834.2510142-1-daniel.lezcano@linaro.org>
 <20220805153834.2510142-5-daniel.lezcano@linaro.org>
 <CGME20220812103956eucas1p1849443855b3537c3f5be2891fa50a50b@eucas1p1.samsung.com>
 <f1fb1d84-85de-f3c5-0212-fcf0e9c0ccd2@samsung.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <f1fb1d84-85de-f3c5-0212-fcf0e9c0ccd2@samsung.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@linaro.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