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: <f1fb1d84-85de-f3c5-0212-fcf0e9c0ccd2@samsung.com>
Date:   Fri, 12 Aug 2022 12:39:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0)
        Gecko/20100101 Thunderbird/91.12.0
Subject: Re: [PATCH 5/5] thermal/core: Move the mutex inside the
 thermal_zone_device_update() function
Content-Language: en-US
To:     Daniel Lezcano <daniel.lezcano@linaro.org>, 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>
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20220805153834.2510142-5-daniel.lezcano@linaro.org>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20220805153834.2510142-1-daniel.lezcano@linaro.org>
        <20220805153834.2510142-5-daniel.lezcano@linaro.org>
        <CGME20220812103956eucas1p1849443855b3537c3f5be2891fa50a50b@eucas1p1.samsung.com>
Precedence: bulk

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland