Re: [PATCH v3 2/6] thermal: add hardware-binning trip-point filtering support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] thermal: dead code and masked errors in
  thermal_of_populate_trip() bin parsing
- [Low] thermal: duplicated devres logic in
  devm_thermal_of_zone_register_with_bin()
--

commit 587b57fa5fb124fbca43f4241cacd250c0901807
Author: Amit Kucheria <amit.kucheria@oss.qualcomm.com>

thermal: add hardware-binning trip-point filtering support

This commit adds Device Tree trip-point filtering and per-bin temperature
selection via the thermal-hw-bin and temperature-bin properties, along with
devm_thermal_of_zone_register_with_bin() for platform drivers to supply
decoded fuse masks at zone registration time.

> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 9e96fb1b033e9..57500e565560c 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -57,21 +57,168 @@ static int thermal_of_get_trip_type(struct device_node *np,

[ ... ]

> +static bool thermal_of_trip_is_supported(struct device_node *np,
> +					 const struct thermal_hw_bin_info *hw)
> +{
> +	unsigned int levels, ngroups, i;
> +	int count;
> +
> +	if (!hw)
> +		return true;
> +
> +	levels = hw->supported_hw_bin_count;
> +
> +	count = of_property_count_u32_elems(np, "thermal-hw-bin");
> +	if (count == -EINVAL)
> +		return true;
> +	if (count < 0) {
> +		pr_warn_once("%pOF: error reading thermal-hw-bin: %d\n", np, count);
> +		return false;
> +	}
> +	if (count == 0)
> +		return true;
> +
> +	if (count % levels) {
> +		pr_warn("%pOF: thermal-hw-bin length %d not a multiple of supported_hw_bin_count %u; disabling trip\n",
> +			np, count, levels);
> +		return false;
> +	}
> +
> +	ngroups = count / levels;
> +	for (i = 0; i < ngroups; i++)
> +		if (thermal_of_match_hw_bin_group(np, hw, levels, i))
> +			return true;
> +
> +	return false;
> +}

[ ... ]

> +static int thermal_of_trip_read_temperature(struct device_node *np,
> +					    const struct thermal_hw_bin_info *hw,
> +					    s32 *temp)
> +{

[ ... ]

> +	levels = hw->supported_hw_bin_count;
> +	count_hw = of_property_count_u32_elems(np, "thermal-hw-bin");
> +	if (count_hw <= 0 || count_hw % levels)
> +		return -EINVAL;
> +
> +	ngroups = count_hw / levels;
> +	if (count_temp != (int)ngroups)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ngroups; i++) {
> +		if (thermal_of_match_hw_bin_group(np, hw, levels, i)) {
> +			u32 utmp;
> +
> +			if (of_property_read_u32_index(np, "temperature-bin",
> +						       i, &utmp))
> +				return -EINVAL;
> +			*temp = (s32)utmp;
> +			return 0;
> +		}
> +	}
> +
> +	pr_warn("%pOF: no thermal-hw-bin sub-group matched for temperature-bin\n", np);
> +	return -ENODEV;
>  }
>  
>  static int thermal_of_populate_trip(struct device_node *np,
> -				    struct thermal_trip *trip)
> +				    struct thermal_trip *trip,
> +				    const struct thermal_hw_bin_info *hw)
>  {
>  	u32 hysteresis;
>  	s32 temperature;
>  	int ret;
>  
> -	ret = of_property_read_s32(np, "temperature", &temperature);
> -	if (ret < 0) {
> -		pr_err("missing temperature property\n");
> -		return ret;
> +	if (!thermal_of_trip_is_supported(np, hw)) {
> +		pr_debug("%pOF: trip skipped: no thermal-hw-bin match\n", np);
> +		return -ENODEV;
>  	}
> +
> +	ret = thermal_of_trip_read_temperature(np, hw, &temperature);

[Severity: Low]
Does calling thermal_of_trip_is_supported() before
thermal_of_trip_read_temperature() leave the no-match path in
thermal_of_trip_read_temperature() unreachable and mask malformed
Device Tree properties?

Whenever thermal_of_trip_read_temperature() reaches its sub-group loop
with a non-NULL hw, thermal_of_trip_is_supported() has already verified
that count_hw % levels == 0 and that a sub-group matches, so the second
loop repeats the same property reads and the trailing warning and -ENODEV
return in thermal_of_trip_read_temperature() cannot be reached.

Because thermal_of_trip_is_supported() returns false on count < 0 or
count % levels != 0, a temperature-bin trip with a malformed
thermal-hw-bin property returns -ENODEV and is silently skipped by
thermal_of_trips_init() instead of failing registration with -EINVAL, and
a count_temp != ngroups mismatch is likewise hidden when no sub-group
matches.  In addition, of_property_count_u32_elems() returns -EINVAL both
when thermal-hw-bin is absent and when its byte length is not a multiple
of 4, so returning true on count == -EINVAL (without checking
of_property_present()) or count == 0 enables a scalar-temperature trip
with a truncated or empty thermal-hw-bin property on all hardware.

[ ... ]

> @@ -510,6 +679,46 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
>  }
>  EXPORT_SYMBOL_GPL(devm_thermal_of_zone_register);
>  
> +/**
> + * devm_thermal_of_zone_register_with_bin - register a thermal zone with hardware binning info
> + *
> + * Like devm_thermal_of_zone_register(), but passes @hw_bin_info to the trip-point
> + * parser so that trips can be filtered and per-bin temperatures selected at
> + * zone registration time.  Use this when fuse values are read before zone
> + * registration (e.g. in a platform driver's probe function).
> + *
> + * @dev:         device structure pointer to sensor
> + * @id:          the sensor identifier
> + * @data:        private data stored in the thermal zone 'devdata' field
> + * @ops:         ops structure associated with the sensor
> + * @hw_bin_info: hardware binning info for trip filtering (may be NULL)
> + */
> +struct thermal_zone_device *
> +devm_thermal_of_zone_register_with_bin(struct device *dev, int id, void *data,
> +				       const struct thermal_zone_device_ops *ops,
> +				       const struct thermal_hw_bin_info *hw_bin_info)
> +{
> +	struct thermal_zone_device **ptr, *tzd;
> +
> +	ptr = devres_alloc(devm_thermal_of_zone_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	tzd = thermal_of_zone_register_with_bin(dev->of_node, id, data,
> +						ops, hw_bin_info);
> +	if (IS_ERR(tzd)) {
> +		devres_free(ptr);
> +		return tzd;
> +	}
> +
> +	*ptr = tzd;
> +	devres_add(dev, ptr);
> +
> +	return tzd;
> +}
> +EXPORT_SYMBOL_GPL(devm_thermal_of_zone_register_with_bin);

[Severity: Low]
This isn't a bug, but devm_thermal_of_zone_register_with_bin() duplicates
the devres handling of devm_thermal_of_zone_register(); having
devm_thermal_of_zone_register() call
devm_thermal_of_zone_register_with_bin(dev, sensor_id, data, ops, NULL)
directly would avoid the duplication and the extra static
thermal_of_zone_register() wrapper.

Also, the kernel-doc comment above places the function description before
the @argument list and omits a Return: section, which causes W=2
kernel-doc warnings.  In thermal_of_trip_read_temperature(), the missing
hw warning uses pr_warn() on every trip node whereas hardware-binning.rst
states that it warns once.  Finally, the new
F: Documentation/driver-api/thermal/hardware-binning.rst line in
MAINTAINERS is already covered by F: Documentation/driver-api/thermal/.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-thermal_bin-v3-0-878df6eef8bc@oss.qualcomm.com?part=2