Re: [PATCH 3/3] platform/x86: dasharo-acpi: Add EC fan curve support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] platform/x86: dasharo-acpi: hardcoded channel 0 in
  dasharo_shutdown()
- [Medium] platform/x86: dasharo-acpi: unlocked update in
  dasharo_curve_point_store()
- [Medium] platform/x86: dasharo-acpi: 64-bit division in
  curve_point_pwm_show()
- [Low] platform/x86: dasharo-acpi: non-static attributes in
  DASHARO_POINT_ATTRS()
- [Low] platform/x86: dasharo-acpi: negative truncation in
  curve_point_temp_store()
--

commit b06965f45c7d7bc48a913eab6b04251f21b0daf9
Author: Filip Gołaś <filip.golas@3mdeb.com>

platform/x86: dasharo-acpi: Add EC fan curve support

This commit exposes the four-point EC fan curve through hwmon
pwmN_auto_pointM_temp and pwmN_auto_pointM_pwm sysfs attributes and
persists the curve to firmware on shutdown.

> diff --git a/drivers/platform/x86/dasharo-acpi.c b/drivers/platform/x86/dasharo-acpi.c
> index 29afcf78564c5..7b21577158cb7 100644
> --- a/drivers/platform/x86/dasharo-acpi.c
> +++ b/drivers/platform/x86/dasharo-acpi.c
[ ... ]
> @@ -378,6 +380,150 @@ static const struct hwmon_chip_info dasharo_hwmon_chip_info = {
>  	.info = dasharo_hwmon_info,
>  };
>  
> +static int dasharo_curve_point_store(struct dasharo_data *data, int channel, const char *method,
> +				     unsigned int point, u64 value)
> +{
> +	u64 args[2] = { point, value };
> +	u64 ret;
> +	int err;
> +
> +	// set of temp/duty
> +	err = dasharo_call(data, method, DASHARO_FEATURE_FAN_POINTS, channel, args, 2, &ret);
> +	if (err)
> +		return err;
> +	if (ret)
> +		return -EPERM;
> +
> +	// commit
> +	err = dasharo_call(data, "CFCV", DASHARO_FEATURE_FAN_POINTS, channel, NULL, 0, &ret);
> +	if (err)
> +		return err;
> +
> +	// The EC rejected the curve
> +	return ret ? -EINVAL : 0;
> +}

[Severity: Medium]
Can concurrent sysfs accesses race across the two-step stage and commit in
dasharo_curve_point_store()?

Attributes in dasharo_groups are registered via extra_groups, so the hwmon
core does not take hwdev->lock around their show and store callbacks, and
kernfs only serializes writes per open file description.  Two writers to
different curve attributes on the same fan channel can interleave between
the stage call (SFPT/SFPD) and the commit call (CFCV):

CPU0                                    CPU1
curve_point_temp_store()
  dasharo_curve_point_store()
    dasharo_call(..., "SFPT", ...)
                                        curve_point_pwm_store()
                                          dasharo_curve_point_store()
                                            dasharo_call(..., "SFPD", ...)
    dasharo_call(..., "CFCV", ...)  <-- commits both or fails on CPU1's stage

A valid update can fail with -EINVAL because of another writer's staged
point, an invalid update can be overwritten and falsely succeed, or a
concurrent reader (GFPT/GFPD) can observe uncommitted values.  Taking
hwmon_lock(dev) around the curve callbacks would serialize these accesses.

[ ... ]

> +static ssize_t curve_point_temp_store(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct dasharo_data *data = dev_get_drvdata(dev);
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err)
> +		return err;
> +
> +	val /= MILLIDEGREE_PER_DEGREE;
> +	if (val < 0 || val > U8_MAX)
> +		return -EINVAL;

[Severity: Low]
Does dividing val by MILLIDEGREE_PER_DEGREE before checking val < 0 allow
negative inputs in [-999, -1] to pass as 0?  Integer division truncates
those negative millidegree values toward zero before the range check,
silently setting the point to 0 C instead of returning -EINVAL.

> +
> +	err = dasharo_curve_point_store(data, sattr->nr, "SFPT", sattr->index, val);
> +	return err ?: count;
> +}
> +
> +static ssize_t curve_point_pwm_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct dasharo_data *data = dev_get_drvdata(dev);
> +	u64 point = sattr->index;
> +	u64 val;
> +	int err;
> +
> +	err = dasharo_call(data, "GFPD", DASHARO_FEATURE_FAN_POINTS, sattr->nr, &point, 1, &val);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%llu\n", DIV_ROUND_CLOSEST(val * 255, 100));
> +}

[Severity: Medium]
Does DIV_ROUND_CLOSEST(val * 255, 100) fail to link on 32-bit x86 builds?
Because val is u64, DIV_ROUND_CLOSEST() performs a 64-bit unsigned division
using /, which emits an undefined reference to __udivdi3 on CONFIG_X86_32.
Using DIV_ROUND_CLOSEST_ULL() avoids the 64-bit / operator.

[ ... ]

> +#define DASHARO_POINT_ATTRS(ch, pt) \
> +	SENSOR_DEVICE_ATTR_2_RW(pwm##ch##_auto_point##pt##_temp, curve_point_temp, \
> +				(ch) - 1, (pt) - 1); \
> +	SENSOR_DEVICE_ATTR_2_RW(pwm##ch##_auto_point##pt##_pwm, curve_point_pwm, \
> +				ch - 1, pt - 1)
> +
> +#define DASHARO_POINT_ATTR_PTRS(ch, pt) \
> +	&sensor_dev_attr_pwm##ch##_auto_point##pt##_temp.dev_attr.attr, \
> +	&sensor_dev_attr_pwm##ch##_auto_point##pt##_pwm.dev_attr.attr
> +
> +DASHARO_POINT_ATTRS(1, 1);
> +DASHARO_POINT_ATTRS(1, 2);
> +DASHARO_POINT_ATTRS(1, 3);
> +DASHARO_POINT_ATTRS(1, 4);
> +DASHARO_POINT_ATTRS(2, 1);
> +DASHARO_POINT_ATTRS(2, 2);
> +DASHARO_POINT_ATTRS(2, 3);
> +DASHARO_POINT_ATTRS(2, 4);

[Severity: Low]
Should the SENSOR_DEVICE_ATTR_2_RW() definitions in DASHARO_POINT_ATTRS()
be static?  SENSOR_DEVICE_ATTR_2_RW() does not include static, so expanding
DASHARO_POINT_ATTRS() at file scope exports 16 sensor_dev_attr_pwm*
symbols into the global namespace and triggers sparse and W=1
-Wmissing-variable-declarations warnings.

[ ... ]

> @@ -413,16 +559,27 @@ static int dasharo_probe(struct platform_device *pdev)
[ ... ]
> +static void dasharo_shutdown(struct platform_device *pdev)
> +{
> +	struct dasharo_data *data = platform_get_drvdata(pdev);
> +	u64 ret;
> +
> +	/* Let the firmware persist the fan curve the EC runs */
> +	if (data->caps_found[DASHARO_FEATURE_FAN_POINTS])
> +		dasharo_call(data, "SFCS", DASHARO_FEATURE_FAN_POINTS, 0, NULL, 0, &ret);
> +}

[Severity: Medium]
Does dasharo_shutdown() leave fan curves on other channels unsaved when
more than one fan supports DASHARO_FEATURE_FAN_POINTS?

dasharo_call() passes data->capabilities[feat][channel].group and .index as
the first two ACPI arguments:

drivers/platform/x86/dasharo-acpi.c:dasharo_call() {
    ...
	obj[0].type = ACPI_TYPE_INTEGER;
	obj[0].integer.value = data->capabilities[feat][channel].group;
	obj[1].type = ACPI_TYPE_INTEGER;
	obj[1].integer.value = data->capabilities[feat][channel].index;
    ...
}

Because dasharo_shutdown() only calls SFCS for channel 0 instead of
iterating up to data->caps_found[DASHARO_FEATURE_FAN_POINTS], any curve
changes on pwm2 (channel 1) are not persisted on shutdown.  (Or if SFCS in
firmware is a global 0-argument method, calling it through dasharo_call()
passes two unexpected arguments.)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113074347.2301465-1-filip.golas@3mdeb.com?part=3