Re: [PATCH v2 2/2] thermal: hwmon: Remove hwmon class device along with its parent

[Lukasz Luba <lukasz.luba@arm.com>]

On 8/4/26 21:11, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current code creates one hwmon device per thermal zone type and that
> device is registered under the first thermal zone of the given type.
> 
> That turns out to be problematic when the thermal zone holding the
> hwmon device is removed.
> 
> For example, say that there are two ACPI thermal zones on a system
> 
>   /sys/devices/virtual/thermal/thermal_zone0/
>   /sys/devices/virtual/thermal/thermal_zone1/
> 
> The current code registers a hwmon class device for thermal_zone0 only:
> 
>   /sys/devices/virtual/thermal/thermal_zone0/hwmon0/
> 
> because the type is "acpitz" for both of them, but it adds a sysfs
> attribute that belongs to thermal_zone1 under it:
> 
>   /sys/devices/virtual/thermal/thermal_zone0/hwmon0/temp2_input
> 
> There is also
> 
>   /sys/devices/virtual/thermal/thermal_zone0/hwmon0/temp1_input
> 
> which belongs to thermal_zone0.
> 
> When thermal_zone0 is removed, say because the ACPI thermal driver is
> unbound from the underlying platform device, thermal_remove_hwmon_sysfs()
> skips the removal of hwmon0 because of the temp2_input attribute
> belonging to thermal_zone1 which effectively prevents thermal_zone0
> removal from making progress.
> 
> Address this by making thermal_remove_hwmon_sysfs() remove the entire
> hwmon class device interface for the given thermal zone type when the
> thermal zone device holding it is removed.
> 
> To prevent races with thermal_add_hwmon_sysfs() that may interfere
> with this, carry out the entire addition and removal of hwmon sysfs
> interfaces for thermal zones under thermal_hwmon_list_lock.
> 
> Also adjust the layout of the labels in thermal_add_hwmon_sysfs() to
> the current kernel coding style to align with the new "unlock" label.
> 
> Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gmail.com/
> Fixes: f6b6b52ef7a5 ("thermal_hwmon: Pass the originating device down to hwmon_device_register_with_info")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2:
>     * Rebase on top of the new [1/2]
> 
> ---
>   drivers/thermal/thermal_hwmon.c |   89 +++++++++++++++-------------------------
>   1 file changed, 35 insertions(+), 54 deletions(-)
> 
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
> @@ -95,34 +95,12 @@ thermal_hwmon_lookup_by_type(const struc
>   	struct thermal_hwmon_device *hwmon;
>   	char type[THERMAL_NAME_LENGTH];
>   
> -	mutex_lock(&thermal_hwmon_list_lock);
>   	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
>   		strscpy(type, tz->type);
>   		strreplace(type, '-', '_');
> -		if (!strcmp(hwmon->type, type)) {
> -			mutex_unlock(&thermal_hwmon_list_lock);
> +		if (!strcmp(hwmon->type, type))
>   			return hwmon;
> -		}
>   	}
> -	mutex_unlock(&thermal_hwmon_list_lock);
> -
> -	return NULL;
> -}
> -
> -/* Find the temperature input matching a given thermal zone */
> -static struct thermal_hwmon_temp *
> -thermal_hwmon_lookup_temp(const struct thermal_hwmon_device *hwmon,
> -			  const struct thermal_zone_device *tz)
> -{
> -	struct thermal_hwmon_temp *temp;
> -
> -	mutex_lock(&thermal_hwmon_list_lock);
> -	list_for_each_entry(temp, &hwmon->tz_list, hwmon_node)
> -		if (temp->tz == tz) {
> -			mutex_unlock(&thermal_hwmon_list_lock);
> -			return temp;
> -		}
> -	mutex_unlock(&thermal_hwmon_list_lock);
>   
>   	return NULL;
>   }
> @@ -138,7 +116,9 @@ int thermal_add_hwmon_sysfs(struct therm
>   	struct thermal_hwmon_device *hwmon;
>   	struct thermal_hwmon_temp *temp;
>   	int new_hwmon_device = 1;
> -	int result;
> +	int result = 0;
> +
> +	mutex_lock(&thermal_hwmon_list_lock);
>   
>   	hwmon = thermal_hwmon_lookup_by_type(tz);
>   	if (hwmon) {
> @@ -147,8 +127,10 @@ int thermal_add_hwmon_sysfs(struct therm
>   	}
>   
>   	hwmon = kzalloc_obj(*hwmon);
> -	if (!hwmon)
> -		return -ENOMEM;
> +	if (!hwmon) {
> +		result = -ENOMEM;
> +		goto unlock;
> +	}
>   
>   	INIT_LIST_HEAD(&hwmon->tz_list);
>   	strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> @@ -196,24 +178,24 @@ int thermal_add_hwmon_sysfs(struct therm
>   		temp->temp_crit_present = true;
>   	}
>   
> -	mutex_lock(&thermal_hwmon_list_lock);
>   	if (new_hwmon_device)
>   		list_add_tail(&hwmon->node, &thermal_hwmon_list);
>   	list_add_tail(&temp->hwmon_node, &hwmon->tz_list);
> -	mutex_unlock(&thermal_hwmon_list_lock);
>   
> -	return 0;
> +	goto unlock;
>   
> - unregister_input:
> +unregister_input:
>   	device_remove_file(hwmon->device, &temp->temp_input.attr);
> - free_temp_mem:
> +free_temp_mem:
>   	kfree(temp);
> - unregister_name:
> +unregister_name:
>   	if (new_hwmon_device)
>   		hwmon_device_unregister(hwmon->device);
> - free_mem:
> +free_mem:
>   	if (new_hwmon_device)
>   		kfree(hwmon);
> +unlock:
> +	mutex_unlock(&thermal_hwmon_list_lock);
>   
>   	return result;
>   }
> @@ -222,8 +204,11 @@ EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysf
>   
>   void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
>   {
> +	struct thermal_hwmon_temp *temp, *entry;
>   	struct thermal_hwmon_device *hwmon;
> -	struct thermal_hwmon_temp *temp;
> +	bool unregister;
> +
> +	guard(mutex)(&thermal_hwmon_list_lock);
>   
>   	hwmon = thermal_hwmon_lookup_by_type(tz);
>   	if (unlikely(!hwmon)) {
> @@ -232,29 +217,25 @@ void thermal_remove_hwmon_sysfs(struct t
>   		return;
>   	}
>   
> -	temp = thermal_hwmon_lookup_temp(hwmon, tz);
> -	if (unlikely(!temp)) {
> -		/* Should never happen... */
> -		dev_dbg(&tz->device, "temperature input lookup failed!\n");
> -		return;
> -	}
> +	unregister = hwmon->device->parent == &tz->device;
>   
> -	device_remove_file(hwmon->device, &temp->temp_input.attr);
> -	if (temp->temp_crit_present)
> -		device_remove_file(hwmon->device, &temp->temp_crit.attr);
> +	list_for_each_entry_safe_reverse(temp, entry, &hwmon->tz_list, hwmon_node) {
> +		if (!unregister && temp->tz != tz)
> +			continue;
>   
> -	mutex_lock(&thermal_hwmon_list_lock);
> -	list_del(&temp->hwmon_node);
> -	kfree(temp);
> -	if (!list_empty(&hwmon->tz_list)) {
> -		mutex_unlock(&thermal_hwmon_list_lock);
> -		return;
> +		device_remove_file(hwmon->device, &temp->temp_input.attr);
> +		if (temp->temp_crit_present)
> +			device_remove_file(hwmon->device, &temp->temp_crit.attr);
> +
> +		list_del(&temp->hwmon_node);
> +		kfree(temp);
>   	}
> -	list_del(&hwmon->node);
> -	mutex_unlock(&thermal_hwmon_list_lock);
>   
> -	hwmon_device_unregister(hwmon->device);
> -	kfree(hwmon);
> +	if (unregister) {
> +		list_del(&hwmon->node);
> +		hwmon_device_unregister(hwmon->device);
> +		kfree(hwmon);
> +	}
>   }
>   EXPORT_SYMBOL_GPL(thermal_remove_hwmon_sysfs);
>   
> 
> 
> 


Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>