[PATCH 0/3] platform/x86: dasharo-acpi: Add fan control

[PATCH 0/3] platform/x86: dasharo-acpi: Add fan control

[Filip Gołaś]

Add the capability to directly control fan PWM and configure the fan
curves. The initial version of the driver only supported monitoring
the fans.

Tested on a NovaCustom V560TU laptop with a pre-release
Dasharo firmware implementing the relevant methods.

Filip Gołaś (3):
  platform/x86: dasharo-acpi: Extract dasharo_call() from
    dasharo_read_channel()
  platform/x86: dasharo-acpi: Add fan mode and PWM write support
  platform/x86: dasharo-acpi: Add EC fan curve support

 drivers/platform/x86/dasharo-acpi.c | 317 ++++++++++++++++++++++++----
 1 file changed, 279 insertions(+), 38 deletions(-)

-- 
2.56.0

[PATCH 1/3] platform/x86: dasharo-acpi: Extract dasharo_call() from dasharo_read_channel()

[Filip Gołaś]

Move the ACPI method evaluation out of dasharo_read_channel() into a new
dasharo_call() helper, which additionally accepts up to two extra integer
arguments after the channel's group and index. This allows the upcoming
write and fan curve support to reuse the same code path.

No functional change intended.

Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
---
 drivers/platform/x86/dasharo-acpi.c | 40 +++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/dasharo-acpi.c b/drivers/platform/x86/dasharo-acpi.c
index dfea00cdde18..13d97eb43511 100644
--- a/drivers/platform/x86/dasharo-acpi.c
+++ b/drivers/platform/x86/dasharo-acpi.c
@@ -93,13 +93,18 @@ static int dasharo_get_feature_cap_count(struct dasharo_data *data, enum dasharo
 	return count;
 }
 
-static int dasharo_read_channel(struct dasharo_data *data, char *method, enum dasharo_feature feat, int channel, long *value)
+/*
+ * Evaluate a DASH method taking the group and index of a channel, followed by
+ * up to two more integer arguments, and returning an integer.
+ */
+static int dasharo_call(struct dasharo_data *data, const char *method, enum dasharo_feature feat,
+			int channel, const u64 *args, unsigned int n_args, u64 *ret)
 {
 	struct acpi_object_list obj_list;
-	union acpi_object obj[2];
+	union acpi_object obj[4];
 	acpi_handle handle;
 	acpi_status status;
-	u64 val;
+	unsigned int i;
 
 	if (feat >= ARRAY_SIZE(data->capabilities))
 		return -EINVAL;
@@ -107,20 +112,35 @@ static int dasharo_read_channel(struct dasharo_data *data, char *method, enum da
 	if (channel >= data->caps_found[feat])
 		return -EINVAL;
 
+	if (n_args > ARRAY_SIZE(obj) - 2) // 2 fixed args always at obj[0] and obj[1]
+		return -EINVAL;
+
 	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;
-	obj_list.count = 2;
-	obj_list.pointer = &obj[0];
+	for (i = 0; i < n_args; ++i) {
+		obj[2 + i].type = ACPI_TYPE_INTEGER;
+		obj[2 + i].integer.value = args[i];
+	}
+	obj_list.count = 2 + n_args;
+	obj_list.pointer = obj;
 
 	handle = ACPI_HANDLE(&data->pdev->dev);
-	status = acpi_evaluate_integer(handle, method, &obj_list, &val);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
+	status = acpi_evaluate_integer(handle, (acpi_string)method, &obj_list, ret);
+	return ACPI_FAILURE(status) ? -ENODEV : 0;
+}
 
-	*value = val;
-	return 0;
+static int dasharo_read_channel(struct dasharo_data *data, const char *method,
+				enum dasharo_feature feat, int channel, long *value)
+{
+	u64 val;
+	int ret;
+
+	ret = dasharo_call(data, method, feat, channel, NULL, 0, &val);
+	if (!ret)
+		*value = val;
+	return ret;
 }
 
 static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
-- 
2.56.0

Re: [PATCH 1/3] platform/x86: dasharo-acpi: Extract dasharo_call() from dasharo_read_channel()

[sashiko-bot]

> Move the ACPI method evaluation out of dasharo_read_channel() into a new
> dasharo_call() helper, which additionally accepts up to two extra integer
> arguments after the channel's group and index. This allows the upcoming
> write and fan curve support to reuse the same code path.
> 
> No functional change intended.
> 
> Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH 2/3] platform/x86: dasharo-acpi: Add fan mode and PWM write support

[Filip Gołaś]

Allow switching fans between automatic control by the EC and manual
control via pwmN_enable (1=manual, 2=automatic), and setting the duty
cycle via pwmN. The EC refuses duty cycle writes in automatic mode.

Fan mode control is only exposed for fans on which the firmware reports
support for it.

Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
---
 drivers/platform/x86/dasharo-acpi.c | 117 +++++++++++++++++++++-------
 1 file changed, 90 insertions(+), 27 deletions(-)

diff --git a/drivers/platform/x86/dasharo-acpi.c b/drivers/platform/x86/dasharo-acpi.c
index 13d97eb43511..29afcf78564c 100644
--- a/drivers/platform/x86/dasharo-acpi.c
+++ b/drivers/platform/x86/dasharo-acpi.c
@@ -15,6 +15,8 @@ enum dasharo_feature {
 	DASHARO_FEATURE_TEMPERATURE = 0,
 	DASHARO_FEATURE_FAN_PWM,
 	DASHARO_FEATURE_FAN_TACH,
+	DASHARO_FEATURE_FAN_POINTS,
+	DASHARO_FEATURE_FAN_MODE,
 	DASHARO_FEATURE_MAX
 };
 
@@ -54,6 +56,16 @@ static const char * const dasharo_group_names[DASHARO_FEATURE_MAX][MAX_GROUPS_PE
 		[DASHARO_FAN_GPU] = "GPU",
 		[DASHARO_FAN_CHASSIS] = "Chassis",
 	},
+	[DASHARO_FEATURE_FAN_MODE] = {
+		[DASHARO_FAN_CPU] = "CPU",
+		[DASHARO_FAN_GPU] = "GPU",
+		[DASHARO_FAN_CHASSIS] = "Chassis",
+	},
+	[DASHARO_FEATURE_FAN_POINTS] = {
+		[DASHARO_FAN_CPU] = "CPU",
+		[DASHARO_FAN_GPU] = "GPU",
+		[DASHARO_FAN_CHASSIS] = "Chassis",
+	},
 };
 
 struct dasharo_capability {
@@ -143,6 +155,21 @@ static int dasharo_read_channel(struct dasharo_data *data, const char *method,
 	return ret;
 }
 
+static int dasharo_write_channel(struct dasharo_data *data, const char *method,
+				 enum dasharo_feature feat, int channel, long value)
+{
+	u64 arg = value;
+	u64 ret;
+	int err;
+
+	err = dasharo_call(data, method, feat, channel, &arg, 1, &ret);
+	if (err)
+		return err;
+
+	/* Non-zero status from the method means the EC refused the write */
+	return ret ? -EPERM : 0;
+}
+
 static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, long *val)
 {
@@ -162,6 +189,14 @@ static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			*val = value;
 		break;
 	case hwmon_pwm:
+		if (attr == hwmon_pwm_enable) {
+			/* hwmon: 1 = manual, 2 = automatic */
+			ret = dasharo_read_channel(data, "GFMD", DASHARO_FEATURE_FAN_MODE,
+						   channel, &value);
+			if (!ret)
+				*val = value ? 1 : 2;
+			break;
+		}
 		ret = dasharo_read_channel(data, "GFDC", DASHARO_FEATURE_FAN_PWM, channel, &value);
 		if (!ret)
 			*val = value;
@@ -174,6 +209,31 @@ static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 	return ret;
 }
 
+static int dasharo_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long val)
+{
+	struct dasharo_data *data = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EINVAL;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		if (val != 1 && val != 2)
+			return -EINVAL;
+		return dasharo_write_channel(data, "SFMD", DASHARO_FEATURE_FAN_MODE,
+					     channel, val == 1);
+	case hwmon_pwm_input:
+		if (val < 0 || val > 255)
+			return -EINVAL;
+		/* Refused by the EC unless in manual mode */
+		return dasharo_write_channel(data, "SFDC", DASHARO_FEATURE_FAN_PWM,
+					     channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int dasharo_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
 				     u32 attr, int channel, const char **str)
 {
@@ -210,9 +270,11 @@ static umode_t dasharo_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_t
 			return 0444;
 		break;
 	case hwmon_pwm:
-		if (channel < data->caps_found[DASHARO_FEATURE_FAN_PWM])
-			return 0444;
-		break;
+		if (channel >= data->caps_found[DASHARO_FEATURE_FAN_PWM])
+			break;
+		if (attr == hwmon_pwm_enable)
+			return channel < data->caps_found[DASHARO_FEATURE_FAN_MODE] ? 0644 : 0;
+		return data->caps_found[DASHARO_FEATURE_FAN_MODE] ? 0644 : 0444;
 	case hwmon_fan:
 		if (channel < data->caps_found[DASHARO_FEATURE_FAN_TACH])
 			return 0444;
@@ -228,6 +290,7 @@ static const struct hwmon_ops dasharo_hwmon_ops = {
 	.is_visible = dasharo_hwmon_is_visible,
 	.read_string = dasharo_hwmon_read_string,
 	.read = dasharo_hwmon_read,
+	.write = dasharo_hwmon_write,
 };
 
 // Max 24 capabilities per feature
@@ -283,30 +346,30 @@ static const struct hwmon_channel_info * const dasharo_hwmon_info[] = {
 		HWMON_T_INPUT | HWMON_T_LABEL,
 		HWMON_T_INPUT | HWMON_T_LABEL),
 	HWMON_CHANNEL_INFO(pwm,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT,
-		HWMON_PWM_INPUT),
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+		HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
 	NULL
 };
 
-- 
2.56.0

Re: [PATCH 2/3] platform/x86: dasharo-acpi: Add fan mode and PWM write support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] platform/x86: dasharo-acpi: broken per-channel fan mode
  mapping and visibility
--

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

platform/x86: dasharo-acpi: Add fan mode and PWM write support

This commit adds hwmon support for switching fans between automatic and
manual control via pwmN_enable and setting PWM duty cycles via pwmN.

> diff --git a/drivers/platform/x86/dasharo-acpi.c b/drivers/platform/x86/dasharo-acpi.c
> index 13d97eb43511f..29afcf78564c5 100644
> --- a/drivers/platform/x86/dasharo-acpi.c
> +++ b/drivers/platform/x86/dasharo-acpi.c
[ ... ]
> @@ -162,6 +189,14 @@ static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			*val = value;
>  		break;
>  	case hwmon_pwm:
> +		if (attr == hwmon_pwm_enable) {
> +			/* hwmon: 1 = manual, 2 = automatic */
> +			ret = dasharo_read_channel(data, "GFMD", DASHARO_FEATURE_FAN_MODE,
> +						   channel, &value);
> +			if (!ret)
> +				*val = value ? 1 : 2;
> +			break;
> +		}
>  		ret = dasharo_read_channel(data, "GFDC", DASHARO_FEATURE_FAN_PWM, channel, &value);
>  		if (!ret)
>  			*val = value;
> @@ -174,6 +209,31 @@ static int dasharo_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  	return ret;
>  }
>  
> +static int dasharo_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long val)
> +{
> +	struct dasharo_data *data = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EINVAL;
> +
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		if (val != 1 && val != 2)
> +			return -EINVAL;
> +		return dasharo_write_channel(data, "SFMD", DASHARO_FEATURE_FAN_MODE,
> +					     channel, val == 1);
[ ... ]
> @@ -210,9 +270,11 @@ static umode_t dasharo_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_t
>  			return 0444;
>  		break;
>  	case hwmon_pwm:
> -		if (channel < data->caps_found[DASHARO_FEATURE_FAN_PWM])
> -			return 0444;
> -		break;
> +		if (channel >= data->caps_found[DASHARO_FEATURE_FAN_PWM])
> +			break;
> +		if (attr == hwmon_pwm_enable)
> +			return channel < data->caps_found[DASHARO_FEATURE_FAN_MODE] ? 0644 : 0;
> +		return data->caps_found[DASHARO_FEATURE_FAN_MODE] ? 0644 : 0444;

[Severity: Medium]
Can indexing DASHARO_FEATURE_FAN_MODE directly with the hwmon_pwm channel
number operate on the wrong fan when only a subset of PWM fans supports
mode control?

dasharo_fill_feature_caps() packs capabilities for each feature densely
from index 0 across groups, skipping groups with no capabilities.  If an
earlier fan supports DASHARO_FEATURE_FAN_PWM without
DASHARO_FEATURE_FAN_MODE while a later fan supports both,
data->capabilities[DASHARO_FEATURE_FAN_MODE][0] refers to the later fan
while channel 0 of DASHARO_FEATURE_FAN_PWM refers to the earlier one.
dasharo_hwmon_is_visible() will then expose pwm1_enable on the earlier
fan instead of the later one, and GFMD/SFMD will read and switch the mode
of a different fan than GFDC/SFDC.

Also, for hwmon_pwm_input, dasharo_hwmon_is_visible() checks
data->caps_found[DASHARO_FEATURE_FAN_MODE] globally rather than checking
whether the given channel supports DASHARO_FEATURE_FAN_MODE, exposing
pwmN as 0644 instead of 0444 on fans without mode control.

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

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

[Filip Gołaś]

Expose the four-point fan curve run by the EC as pwmN_auto_pointM_temp
and pwmN_auto_pointM_pwm, so that userspace can read and change it.
The EC validates the curve on every write and keeps running the
previous one if the new curve is invalid.

On shutdown, ask the firmware to store the current curve, so that it is
restored on the next boot.

Fan curve control is only exposed for fans on which the firmware reports
support for it.

Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
---
 drivers/platform/x86/dasharo-acpi.c | 160 +++++++++++++++++++++++++++-
 1 file changed, 159 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dasharo-acpi.c b/drivers/platform/x86/dasharo-acpi.c
index 29afcf78564c..7b21577158cb 100644
--- a/drivers/platform/x86/dasharo-acpi.c
+++ b/drivers/platform/x86/dasharo-acpi.c
@@ -6,6 +6,8 @@
 #include <linux/acpi.h>
 #include <linux/array_size.h>
 #include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/kstrtox.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
@@ -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;
+}
+
+static ssize_t curve_point_temp_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, "GFPT", DASHARO_FEATURE_FAN_POINTS, sattr->nr, &point, 1, &val);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%llu\n", val * MILLIDEGREE_PER_DEGREE);
+}
+
+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;
+
+	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));
+}
+
+static ssize_t curve_point_pwm_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;
+
+	if (val < 0 || val > 255)
+		return -EINVAL;
+
+	err = dasharo_curve_point_store(data, sattr->nr, "SFPD", sattr->index,
+				  DIV_ROUND_CLOSEST(val * 100, 255));
+	return err ?: count;
+}
+
+#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);
+
+static struct attribute *dasharo_point_attrs[] = {
+	DASHARO_POINT_ATTR_PTRS(1, 1),
+	DASHARO_POINT_ATTR_PTRS(1, 2),
+	DASHARO_POINT_ATTR_PTRS(1, 3),
+	DASHARO_POINT_ATTR_PTRS(1, 4),
+	DASHARO_POINT_ATTR_PTRS(2, 1),
+	DASHARO_POINT_ATTR_PTRS(2, 2),
+	DASHARO_POINT_ATTR_PTRS(2, 3),
+	DASHARO_POINT_ATTR_PTRS(2, 4),
+	NULL
+};
+
+static umode_t dasharo_curve_point_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct dasharo_data *data = dev_get_drvdata(dev);
+	struct device_attribute *dattr = container_of(attr, struct device_attribute, attr);
+	int channel = to_sensor_dev_attr_2(dattr)->nr;
+
+	return channel < data->caps_found[DASHARO_FEATURE_FAN_POINTS] ? attr->mode : 0;
+}
+
+static const struct attribute_group dasharo_point_group = {
+	.attrs = dasharo_point_attrs,
+	.is_visible = dasharo_curve_point_is_visible,
+};
+
+static const struct attribute_group *dasharo_groups[] = {
+	&dasharo_point_group,
+	NULL
+};
+
 static void dasharo_fill_feature_caps(struct dasharo_data *data, enum dasharo_feature feat)
 {
 	struct dasharo_capability *cap;
@@ -413,16 +559,27 @@ static int dasharo_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 	data->pdev = pdev;
+	platform_set_drvdata(pdev, data);
 
 	for (unsigned int i = 0; i < DASHARO_FEATURE_MAX; ++i)
 		dasharo_fill_feature_caps(data, i);
 
 	hwmon = devm_hwmon_device_register_with_info(&pdev->dev, "dasharo_acpi", data,
-						     &dasharo_hwmon_chip_info, NULL);
+						     &dasharo_hwmon_chip_info, dasharo_groups);
 
 	return PTR_ERR_OR_ZERO(hwmon);
 }
 
+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);
+}
+
 static const struct acpi_device_id dasharo_device_ids[] = {
 	{"DSHR0001", 0},
 	{}
@@ -435,6 +592,7 @@ static struct platform_driver dasharo_driver = {
 		.acpi_match_table = dasharo_device_ids,
 	},
 	.probe = dasharo_probe,
+	.shutdown = dasharo_shutdown,
 };
 module_platform_driver(dasharo_driver);
 
-- 
2.56.0

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

[sashiko-bot]

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