[PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Armin Wolf]

This patch series adds support for an alternative SMM calling
backend to the dell-smm-hwmon driver. The reason for this is
that on some modern machines, the legacy SMM calling interface
does not work anymore and the SMM handler can be called over
ACPI WMI instead.

The first four patches prepare the driver by allowing to
specify different SMM calling backends, and by moving most of
the DMI handling into i8k_init() so that the DMI tables can
keep their __initconst attributes (the WMI SMM backend driver
does not probe at module init time). The fifth patch does some
minor cleanup, while the next three patches implement the new
WMI SMM calling backend. The last patch adds the machine of
the user who requested and tested the changes to the fan control
whitelist.

If the driver does not detect the legacy SMM interface, either
because the machine is not whitelisted or because the SMM handler
does not react, it registers an WMI driver which will then bound
to the WMI SMM interface and do the remaining initialization.

The deprecated procfs interface is not supported when using the
WMI SMM calling backend for the following reason: the WMI driver
can potentially be instantiated multiple times while the deprectated
procfs interface cannot. This should not cause any regressions
because on machines supporting only the WMI SMM interface, the
driver would, until now, not load anyway.

All patches where tested on a Dell Inspiron 3505 and a Dell
OptiPlex 7000.

Changes since v2:
- Rework WMI response parsing
- Use #define for method number

Changes since v1:
- Cc platform driver maintainers
- Fix formating inside documentation

Armin Wolf (9):
  hwmon: (dell-smm) Prepare for multiple SMM calling backends
  hwmon: (dell-smm) Move blacklist handling to module init
  hwmon: (dell-smm) Move whitelist handling to module init
  hwmon: (dell-smm) Move DMI config handling to module init
  hwmon: (dell-smm) Move config entries out of i8k_dmi_table
  hwmon: (dell-smm) Introduce helper function for data init
  hwmon: (dell-smm) Add support for WMI SMM interface
  hwmon: (dell-smm) Document the WMI SMM interface
  hwmon: (dell-smm) Add Optiplex 7000 to fan control whitelist

 Documentation/hwmon/dell-smm-hwmon.rst |  38 +-
 drivers/hwmon/Kconfig                  |   1 +
 drivers/hwmon/dell-smm-hwmon.c         | 603 +++++++++++++++++--------
 drivers/platform/x86/wmi.c             |   1 +
 4 files changed, 453 insertions(+), 190 deletions(-)

--
2.39.2

[PATCH v3 1/9] hwmon: (dell-smm) Prepare for multiple SMM calling backends

[Armin Wolf]

Modern Dell machines support multiple ways to issue an SMM call.
Prepare support for those by introducing dell_smm_ops, which is
used by dell_smm_call() to perform a SMM call. Each SMM backend
needs to provide a dell_smm_ops structure.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 131 ++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 52 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 44aaf9b9191d..a3273780f7c3 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -69,6 +69,20 @@
 #define DELL_SMM_NO_TEMP	10
 #define DELL_SMM_NO_FANS	3

+struct smm_regs {
+	int eax;
+	int ebx;
+	int ecx;
+	int edx;
+	int esi;
+	int edi;
+};
+
+struct dell_smm_ops {
+	struct device *smm_dev;
+	int (*smm_call)(struct device *smm_dev, struct smm_regs *regs);
+};
+
 struct dell_smm_data {
 	struct mutex i8k_mutex; /* lock for sensors writes */
 	char bios_version[4];
@@ -84,6 +98,7 @@ struct dell_smm_data {
 	bool fan[DELL_SMM_NO_FANS];
 	int fan_type[DELL_SMM_NO_FANS];
 	int *fan_nominal_speed[DELL_SMM_NO_FANS];
+	const struct dell_smm_ops *ops;
 };

 struct dell_smm_cooling_data {
@@ -123,15 +138,6 @@ static uint fan_max;
 module_param(fan_max, uint, 0);
 MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");

-struct smm_regs {
-	unsigned int eax;
-	unsigned int ebx;
-	unsigned int ecx;
-	unsigned int edx;
-	unsigned int esi;
-	unsigned int edi;
-};
-
 static const char * const temp_labels[] = {
 	"CPU",
 	"GPU",
@@ -171,12 +177,8 @@ static inline const char __init *i8k_get_dmi_data(int field)
  */
 static int i8k_smm_func(void *par)
 {
-	ktime_t calltime = ktime_get();
 	struct smm_regs *regs = par;
-	int eax = regs->eax;
-	int ebx = regs->ebx;
 	unsigned char carry;
-	long long duration;

 	/* SMM requires CPU 0 */
 	if (smp_processor_id() != 0)
@@ -193,14 +195,7 @@ static int i8k_smm_func(void *par)
 		       "+S" (regs->esi),
 		       "+D" (regs->edi));

-	duration = ktime_us_delta(ktime_get(), calltime);
-	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
-		 eax, ebx, regs->eax & 0xffff, carry, duration);
-
-	if (duration > DELL_SMM_MAX_DURATION)
-		pr_warn_once("SMM call took %lld usecs!\n", duration);
-
-	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+	if (carry)
 		return -EINVAL;

 	return 0;
@@ -209,7 +204,7 @@ static int i8k_smm_func(void *par)
 /*
  * Call the System Management Mode BIOS.
  */
-static int i8k_smm(struct smm_regs *regs)
+static int i8k_smm_call(struct device *dummy, struct smm_regs *regs)
 {
 	int ret;

@@ -220,6 +215,37 @@ static int i8k_smm(struct smm_regs *regs)
 	return ret;
 }

+static const struct dell_smm_ops i8k_smm_ops = {
+	.smm_call = i8k_smm_call,
+};
+
+static int dell_smm_call(const struct dell_smm_ops *ops, struct smm_regs *regs)
+{
+	int eax = regs->eax;
+	int ebx = regs->ebx;
+	long long duration;
+	ktime_t calltime;
+	int ret;
+
+	calltime = ktime_get();
+	ret = ops->smm_call(ops->smm_dev, regs);
+	duration = ktime_us_delta(ktime_get(), calltime);
+
+	pr_debug("SMM(0x%.4x 0x%.4x) = 0x%.4x status: %d (took %7lld usecs)\n",
+		 eax, ebx, regs->eax & 0xffff, ret, duration);
+
+	if (duration > DELL_SMM_MAX_DURATION)
+		pr_warn_once("SMM call took %lld usecs!\n", duration);
+
+	if (ret < 0)
+		return ret;
+
+	if ((regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Read the fan status.
  */
@@ -233,7 +259,7 @@ static int i8k_get_fan_status(const struct dell_smm_data *data, u8 fan)
 	if (data->disallow_fan_support)
 		return -EINVAL;

-	return i8k_smm(&regs) ? : regs.eax & 0xff;
+	return dell_smm_call(data->ops, &regs) ? : regs.eax & 0xff;
 }

 /*
@@ -249,7 +275,7 @@ static int i8k_get_fan_speed(const struct dell_smm_data *data, u8 fan)
 	if (data->disallow_fan_support)
 		return -EINVAL;

-	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
+	return dell_smm_call(data->ops, &regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
 }

 /*
@@ -265,7 +291,7 @@ static int _i8k_get_fan_type(const struct dell_smm_data *data, u8 fan)
 	if (data->disallow_fan_support || data->disallow_fan_type_call)
 		return -EINVAL;

-	return i8k_smm(&regs) ? : regs.eax & 0xff;
+	return dell_smm_call(data->ops, &regs) ? : regs.eax & 0xff;
 }

 static int i8k_get_fan_type(struct dell_smm_data *data, u8 fan)
@@ -290,7 +316,7 @@ static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8
 	if (data->disallow_fan_support)
 		return -EINVAL;

-	return i8k_smm(&regs) ? : (regs.eax & 0xffff);
+	return dell_smm_call(data->ops, &regs) ? : (regs.eax & 0xffff);
 }

 /*
@@ -304,7 +330,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
 		return -EINVAL;

 	regs.eax = enable ? data->auto_fan : data->manual_fan;
-	return i8k_smm(&regs);
+	return dell_smm_call(data->ops, &regs);
 }

 /*
@@ -320,35 +346,35 @@ static int i8k_set_fan(const struct dell_smm_data *data, u8 fan, int speed)
 	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
 	regs.ebx = fan | (speed << 8);

-	return i8k_smm(&regs);
+	return dell_smm_call(data->ops, &regs);
 }

-static int __init i8k_get_temp_type(u8 sensor)
+static int __init i8k_get_temp_type(const struct dell_smm_data *data, u8 sensor)
 {
 	struct smm_regs regs = {
 		.eax = I8K_SMM_GET_TEMP_TYPE,
 		.ebx = sensor,
 	};

-	return i8k_smm(&regs) ? : regs.eax & 0xff;
+	return dell_smm_call(data->ops, &regs) ? : regs.eax & 0xff;
 }

 /*
  * Read the cpu temperature.
  */
-static int _i8k_get_temp(u8 sensor)
+static int _i8k_get_temp(const struct dell_smm_data *data, u8 sensor)
 {
 	struct smm_regs regs = {
 		.eax = I8K_SMM_GET_TEMP,
 		.ebx = sensor,
 	};

-	return i8k_smm(&regs) ? : regs.eax & 0xff;
+	return dell_smm_call(data->ops, &regs) ? : regs.eax & 0xff;
 }

-static int i8k_get_temp(u8 sensor)
+static int i8k_get_temp(const struct dell_smm_data *data, u8 sensor)
 {
-	int temp = _i8k_get_temp(sensor);
+	int temp = _i8k_get_temp(data, sensor);

 	/*
 	 * Sometimes the temperature sensor returns 0x99, which is out of range.
@@ -359,7 +385,7 @@ static int i8k_get_temp(u8 sensor)
 	 */
 	if (temp == 0x99) {
 		msleep(100);
-		temp = _i8k_get_temp(sensor);
+		temp = _i8k_get_temp(data, sensor);
 	}
 	/*
 	 * Return -ENODATA for all invalid temperatures.
@@ -375,12 +401,12 @@ static int i8k_get_temp(u8 sensor)
 	return temp;
 }

-static int __init i8k_get_dell_signature(int req_fn)
+static int __init dell_smm_get_signature(const struct dell_smm_ops *ops, int req_fn)
 {
 	struct smm_regs regs = { .eax = req_fn, };
 	int rc;

-	rc = i8k_smm(&regs);
+	rc = dell_smm_call(ops, &regs);
 	if (rc < 0)
 		return rc;

@@ -392,12 +418,12 @@ static int __init i8k_get_dell_signature(int req_fn)
 /*
  * Read the Fn key status.
  */
-static int i8k_get_fn_status(void)
+static int i8k_get_fn_status(const struct dell_smm_data *data)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_FN_STATUS, };
 	int rc;

-	rc = i8k_smm(&regs);
+	rc = dell_smm_call(data->ops, &regs);
 	if (rc < 0)
 		return rc;

@@ -416,12 +442,12 @@ static int i8k_get_fn_status(void)
 /*
  * Read the power status.
  */
-static int i8k_get_power_status(void)
+static int i8k_get_power_status(const struct dell_smm_data *data)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_POWER_STATUS, };
 	int rc;

-	rc = i8k_smm(&regs);
+	rc = dell_smm_call(data->ops, &regs);
 	if (rc < 0)
 		return rc;

@@ -464,15 +490,15 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)

 		return 0;
 	case I8K_FN_STATUS:
-		val = i8k_get_fn_status();
+		val = i8k_get_fn_status(data);
 		break;

 	case I8K_POWER_STATUS:
-		val = i8k_get_power_status();
+		val = i8k_get_power_status(data);
 		break;

 	case I8K_GET_TEMP:
-		val = i8k_get_temp(0);
+		val = i8k_get_temp(data, 0);
 		break;

 	case I8K_GET_SPEED:
@@ -539,14 +565,14 @@ static int i8k_proc_show(struct seq_file *seq, void *offset)
 	int fn_key, cpu_temp, ac_power;
 	int left_fan, right_fan, left_speed, right_speed;

-	cpu_temp	= i8k_get_temp(0);				/* 11100 µs */
+	cpu_temp	= i8k_get_temp(data, 0);			/* 11100 µs */
 	left_fan	= i8k_get_fan_status(data, I8K_FAN_LEFT);	/*   580 µs */
 	right_fan	= i8k_get_fan_status(data, I8K_FAN_RIGHT);	/*   580 µs */
 	left_speed	= i8k_get_fan_speed(data, I8K_FAN_LEFT);	/*   580 µs */
 	right_speed	= i8k_get_fan_speed(data, I8K_FAN_RIGHT);	/*   580 µs */
-	fn_key		= i8k_get_fn_status();				/*   750 µs */
+	fn_key		= i8k_get_fn_status(data);			/*   750 µs */
 	if (power_status)
-		ac_power = i8k_get_power_status();			/* 14700 µs */
+		ac_power = i8k_get_power_status(data);			/* 14700 µs */
 	else
 		ac_power = -1;

@@ -665,7 +691,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
 		switch (attr) {
 		case hwmon_temp_input:
 			/* _i8k_get_temp() is fine since we do not care about the actual value */
-			if (data->temp_type[channel] >= 0 || _i8k_get_temp(channel) >= 0)
+			if (data->temp_type[channel] >= 0 || _i8k_get_temp(data, channel) >= 0)
 				return 0444;

 			break;
@@ -747,7 +773,7 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
 	case hwmon_temp:
 		switch (attr) {
 		case hwmon_temp_input:
-			ret = i8k_get_temp(channel);
+			ret = i8k_get_temp(data, channel);
 			if (ret < 0)
 				return ret;

@@ -994,7 +1020,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
 	u8 i;

 	for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
-		data->temp_type[i] = i8k_get_temp_type(i);
+		data->temp_type[i] = i8k_get_temp_type(data, i);
 		if (data->temp_type[i] < 0)
 			continue;

@@ -1353,6 +1379,7 @@ static int __init dell_smm_probe(struct platform_device *pdev)

 	mutex_init(&data->i8k_mutex);
 	platform_set_drvdata(pdev, data);
+	data->ops = &i8k_smm_ops;

 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
 		if (!force) {
@@ -1445,8 +1472,8 @@ static int __init i8k_init(void)
 	/*
 	 * Get SMM Dell signature
 	 */
-	if (i8k_get_dell_signature(I8K_SMM_GET_DELL_SIG1) &&
-	    i8k_get_dell_signature(I8K_SMM_GET_DELL_SIG2)) {
+	if (dell_smm_get_signature(&i8k_smm_ops, I8K_SMM_GET_DELL_SIG1) &&
+	    dell_smm_get_signature(&i8k_smm_ops, I8K_SMM_GET_DELL_SIG2)) {
 		if (!force)
 			return -ENODEV;

--
2.39.2

[PATCH v3 2/9] hwmon: (dell-smm) Move blacklist handling to module init

[Armin Wolf]

Future SMM calling backends will not be able to probe during
module init, meaning the DMI tables used for backlisting broken
features would have to drop their __initconst attribute.
Prevent this by moving the blacklist handling to module init.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 63 ++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index a3273780f7c3..ccb3fcff4f60 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -90,8 +90,6 @@ struct dell_smm_data {
 	uint i8k_fan_mult;
 	uint i8k_pwm_mult;
 	uint i8k_fan_max;
-	bool disallow_fan_type_call;
-	bool disallow_fan_support;
 	unsigned int manual_fan;
 	unsigned int auto_fan;
 	int temp_type[DELL_SMM_NO_TEMP];
@@ -138,6 +136,8 @@ static uint fan_max;
 module_param(fan_max, uint, 0);
 MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");

+static bool disallow_fan_type_call, disallow_fan_support;
+
 static const char * const temp_labels[] = {
 	"CPU",
 	"GPU",
@@ -256,7 +256,7 @@ static int i8k_get_fan_status(const struct dell_smm_data *data, u8 fan)
 		.ebx = fan,
 	};

-	if (data->disallow_fan_support)
+	if (disallow_fan_support)
 		return -EINVAL;

 	return dell_smm_call(data->ops, &regs) ? : regs.eax & 0xff;
@@ -272,7 +272,7 @@ static int i8k_get_fan_speed(const struct dell_smm_data *data, u8 fan)
 		.ebx = fan,
 	};

-	if (data->disallow_fan_support)
+	if (disallow_fan_support)
 		return -EINVAL;

 	return dell_smm_call(data->ops, &regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
@@ -288,7 +288,7 @@ static int _i8k_get_fan_type(const struct dell_smm_data *data, u8 fan)
 		.ebx = fan,
 	};

-	if (data->disallow_fan_support || data->disallow_fan_type_call)
+	if (disallow_fan_support || disallow_fan_type_call)
 		return -EINVAL;

 	return dell_smm_call(data->ops, &regs) ? : regs.eax & 0xff;
@@ -313,7 +313,7 @@ static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8
 		.ebx = fan | (speed << 8),
 	};

-	if (data->disallow_fan_support)
+	if (disallow_fan_support)
 		return -EINVAL;

 	return dell_smm_call(data->ops, &regs) ? : (regs.eax & 0xffff);
@@ -326,7 +326,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
 {
 	struct smm_regs regs = { };

-	if (data->disallow_fan_support)
+	if (disallow_fan_support)
 		return -EINVAL;

 	regs.eax = enable ? data->auto_fan : data->manual_fan;
@@ -340,7 +340,7 @@ static int i8k_set_fan(const struct dell_smm_data *data, u8 fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };

-	if (data->disallow_fan_support)
+	if (disallow_fan_support)
 		return -EINVAL;

 	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
@@ -705,7 +705,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
 		}
 		break;
 	case hwmon_fan:
-		if (data->disallow_fan_support)
+		if (disallow_fan_support)
 			break;

 		switch (attr) {
@@ -715,7 +715,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types

 			break;
 		case hwmon_fan_label:
-			if (data->fan[channel] && !data->disallow_fan_type_call)
+			if (data->fan[channel] && !disallow_fan_type_call)
 				return 0444;

 			break;
@@ -731,7 +731,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
 		}
 		break;
 	case hwmon_pwm:
-		if (data->disallow_fan_support)
+		if (disallow_fan_support)
 			break;

 		switch (attr) {
@@ -1381,24 +1381,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, data);
 	data->ops = &i8k_smm_ops;

-	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
-		if (!force) {
-			dev_notice(&pdev->dev, "Disabling fan support due to BIOS bugs\n");
-			data->disallow_fan_support = true;
-		} else {
-			dev_warn(&pdev->dev, "Enabling fan support despite BIOS bugs\n");
-		}
-	}
-
-	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
-		if (!force) {
-			dev_notice(&pdev->dev, "Disabling fan type call due to BIOS bugs\n");
-			data->disallow_fan_type_call = true;
-		} else {
-			dev_warn(&pdev->dev, "Enabling fan type call despite BIOS bugs\n");
-		}
-	}
-
 	strscpy(data->bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
 		sizeof(data->bios_version));
 	strscpy(data->bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
@@ -1453,6 +1435,27 @@ static struct platform_device *dell_smm_device;
 /*
  * Probe for the presence of a supported laptop.
  */
+static void __init dell_smm_init_dmi(void)
+{
+	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
+		if (!force) {
+			pr_notice("Disabling fan support due to BIOS bugs\n");
+			disallow_fan_support = true;
+		} else {
+			pr_warn("Enabling fan support despite BIOS bugs\n");
+		}
+	}
+
+	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
+		if (!force) {
+			pr_notice("Disabling fan type call due to BIOS bugs\n");
+			disallow_fan_type_call = true;
+		} else {
+			pr_warn("Enabling fan type call despite BIOS bugs\n");
+		}
+	}
+}
+
 static int __init i8k_init(void)
 {
 	/*
@@ -1469,6 +1472,8 @@ static int __init i8k_init(void)
 			i8k_get_dmi_data(DMI_BIOS_VERSION));
 	}

+	dell_smm_init_dmi();
+
 	/*
 	 * Get SMM Dell signature
 	 */
--
2.39.2

[PATCH v3 3/9] hwmon: (dell-smm) Move whitelist handling to module init

[Armin Wolf]

Future SMM calling backends will not be able to probe during
module init, meaning the DMI tables used for whitelisting
features would have to drop their __initconst attribute.
Prevent this by moving the whitelist handling to module init.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index ccb3fcff4f60..3a792faf2369 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -90,8 +90,6 @@ struct dell_smm_data {
 	uint i8k_fan_mult;
 	uint i8k_pwm_mult;
 	uint i8k_fan_max;
-	unsigned int manual_fan;
-	unsigned int auto_fan;
 	int temp_type[DELL_SMM_NO_TEMP];
 	bool fan[DELL_SMM_NO_FANS];
 	int fan_type[DELL_SMM_NO_FANS];
@@ -138,6 +136,8 @@ MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)"

 static bool disallow_fan_type_call, disallow_fan_support;

+static unsigned int manual_fan, auto_fan;
+
 static const char * const temp_labels[] = {
 	"CPU",
 	"GPU",
@@ -329,7 +329,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
 	if (disallow_fan_support)
 		return -EINVAL;

-	regs.eax = enable ? data->auto_fan : data->manual_fan;
+	regs.eax = enable ? auto_fan : manual_fan;
 	return dell_smm_call(data->ops, &regs);
 }

@@ -741,7 +741,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types

 			break;
 		case hwmon_pwm_enable:
-			if (data->auto_fan)
+			if (auto_fan)
 				/*
 				 * There is no command for retrieve the current status
 				 * from BIOS, and userspace/firmware itself can change
@@ -1370,7 +1370,7 @@ static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
 static int __init dell_smm_probe(struct platform_device *pdev)
 {
 	struct dell_smm_data *data;
-	const struct dmi_system_id *id, *fan_control;
+	const struct dmi_system_id *id;
 	int ret;

 	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
@@ -1406,15 +1406,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;
 	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);

-	fan_control = dmi_first_match(i8k_whitelist_fan_control);
-	if (fan_control && fan_control->driver_data) {
-		const struct i8k_fan_control_data *control = fan_control->driver_data;
-
-		data->manual_fan = control->manual_fan;
-		data->auto_fan = control->auto_fan;
-		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
-	}
-
 	ret = dell_smm_init_hwmon(&pdev->dev);
 	if (ret)
 		return ret;
@@ -1437,6 +1428,9 @@ static struct platform_device *dell_smm_device;
  */
 static void __init dell_smm_init_dmi(void)
 {
+	struct i8k_fan_control_data *control;
+	const struct dmi_system_id *id;
+
 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
 		if (!force) {
 			pr_notice("Disabling fan support due to BIOS bugs\n");
@@ -1454,6 +1448,15 @@ static void __init dell_smm_init_dmi(void)
 			pr_warn("Enabling fan type call despite BIOS bugs\n");
 		}
 	}
+
+	id = dmi_first_match(i8k_whitelist_fan_control);
+	if (id && id->driver_data) {
+		control = id->driver_data;
+		manual_fan = control->manual_fan;
+		auto_fan = control->auto_fan;
+
+		pr_info("Enabling support for setting automatic/manual fan control\n");
+	}
 }

 static int __init i8k_init(void)
--
2.39.2

[PATCH v3 4/9] hwmon: (dell-smm) Move DMI config handling to module init

[Armin Wolf]

Future SMM calling backends will not be able to probe during
module init, meaning the DMI tables holding config data would
have to drop their __initconst attribute.
Prevent this by moving the config handling to module init.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 3a792faf2369..3d14a3baf8e4 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1370,7 +1370,6 @@ static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
 static int __init dell_smm_probe(struct platform_device *pdev)
 {
 	struct dell_smm_data *data;
-	const struct dmi_system_id *id;
 	int ret;

 	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
@@ -1386,21 +1385,6 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	strscpy(data->bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
 		sizeof(data->bios_machineid));

-	/*
-	 * Set fan multiplier and maximal fan speed from dmi config
-	 * Values specified in module parameters override values from dmi
-	 */
-	id = dmi_first_match(i8k_dmi_table);
-	if (id && id->driver_data) {
-		const struct i8k_config_data *conf = id->driver_data;
-
-		if (!fan_mult && conf->fan_mult)
-			fan_mult = conf->fan_mult;
-
-		if (!fan_max && conf->fan_max)
-			fan_max = conf->fan_max;
-	}
-
 	/* All options must not be 0 */
 	data->i8k_fan_mult = fan_mult ? : I8K_FAN_MULT;
 	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;
@@ -1429,6 +1413,7 @@ static struct platform_device *dell_smm_device;
 static void __init dell_smm_init_dmi(void)
 {
 	struct i8k_fan_control_data *control;
+	struct i8k_config_data *config;
 	const struct dmi_system_id *id;

 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
@@ -1449,6 +1434,20 @@ static void __init dell_smm_init_dmi(void)
 		}
 	}

+	/*
+	 * Set fan multiplier and maximal fan speed from DMI config.
+	 * Values specified in module parameters override values from DMI.
+	 */
+	id = dmi_first_match(i8k_dmi_table);
+	if (id && id->driver_data) {
+		config = id->driver_data;
+		if (!fan_mult && config->fan_mult)
+			fan_mult = config->fan_mult;
+
+		if (!fan_max && config->fan_max)
+			fan_max = config->fan_max;
+	}
+
 	id = dmi_first_match(i8k_whitelist_fan_control);
 	if (id && id->driver_data) {
 		control = id->driver_data;
--
2.39.2

[PATCH v3 5/9] hwmon: (dell-smm) Move config entries out of i8k_dmi_table

[Armin Wolf]

Currently, i8k_dmi_table contains both entries used for DMI
matching and entries used to override config options. This
does not allow for differentiating between "its safe to issue
raw SMM calls on this machine" and "its not safe to issue raw
SMM calls on this machine, but here are some config values".

Since future SMM backends will need to differentiate between
those two cases, move those config entries into a separate
table. i8k_dmi_table now serves as a general "its safe to issue
raw SMM calls" table.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 129 +++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 56 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 3d14a3baf8e4..4a2c0d1ffa5b 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1078,42 +1078,6 @@ static int __init dell_smm_init_hwmon(struct device *dev)
 	return PTR_ERR_OR_ZERO(dell_smm_hwmon_dev);
 }

-struct i8k_config_data {
-	uint fan_mult;
-	uint fan_max;
-};
-
-enum i8k_configs {
-	DELL_LATITUDE_D520,
-	DELL_PRECISION_490,
-	DELL_STUDIO,
-	DELL_XPS,
-};
-
-/*
- * Only use for machines which need some special configuration
- * in order to work correctly (e.g. if autoconfig fails on this machines).
- */
-
-static const struct i8k_config_data i8k_config_data[] __initconst = {
-	[DELL_LATITUDE_D520] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_TURBO,
-	},
-	[DELL_PRECISION_490] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_TURBO,
-	},
-	[DELL_STUDIO] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-	[DELL_XPS] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-};
-
 static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 	{
 		.ident = "Dell G5 5590",
@@ -1143,14 +1107,6 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron"),
 		},
 	},
-	{
-		.ident = "Dell Latitude D520",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D520"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
-	},
 	{
 		.ident = "Dell Latitude 2",
 		.matches = {
@@ -1172,15 +1128,6 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "MP061"),
 		},
 	},
-	{
-		.ident = "Dell Precision 490",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME,
-				  "Precision WorkStation 490"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_PRECISION_490],
-	},
 	{
 		.ident = "Dell Precision",
 		.matches = {
@@ -1201,7 +1148,6 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Studio"),
 		},
-		.driver_data = (void *)&i8k_config_data[DELL_STUDIO],
 	},
 	{
 		.ident = "Dell XPS M140",
@@ -1209,7 +1155,6 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "MXC051"),
 		},
-		.driver_data = (void *)&i8k_config_data[DELL_XPS],
 	},
 	{
 		.ident = "Dell XPS",
@@ -1223,6 +1168,78 @@ static const struct dmi_system_id i8k_dmi_table[] __initconst = {

 MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);

+/*
+ * Only use for machines which need some special configuration
+ * in order to work correctly (e.g. if autoconfig fails on this machines).
+ */
+struct i8k_config_data {
+	uint fan_mult;
+	uint fan_max;
+};
+
+enum i8k_configs {
+	DELL_LATITUDE_D520,
+	DELL_PRECISION_490,
+	DELL_STUDIO,
+	DELL_XPS,
+};
+
+static const struct i8k_config_data i8k_config_data[] __initconst = {
+	[DELL_LATITUDE_D520] = {
+		.fan_mult = 1,
+		.fan_max = I8K_FAN_TURBO,
+	},
+	[DELL_PRECISION_490] = {
+		.fan_mult = 1,
+		.fan_max = I8K_FAN_TURBO,
+	},
+	[DELL_STUDIO] = {
+		.fan_mult = 1,
+		.fan_max = I8K_FAN_HIGH,
+	},
+	[DELL_XPS] = {
+		.fan_mult = 1,
+		.fan_max = I8K_FAN_HIGH,
+	},
+};
+
+static const struct dmi_system_id i8k_config_dmi_table[] __initconst = {
+	{
+		.ident = "Dell Latitude D520",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D520"),
+		},
+		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
+	},
+	{
+		.ident = "Dell Precision 490",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME,
+				  "Precision WorkStation 490"),
+		},
+		.driver_data = (void *)&i8k_config_data[DELL_PRECISION_490],
+	},
+	{
+		.ident = "Dell Studio",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Studio"),
+		},
+		.driver_data = (void *)&i8k_config_data[DELL_STUDIO],
+	},
+	{
+		.ident = "Dell XPS M140",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MXC051"),
+		},
+		.driver_data = (void *)&i8k_config_data[DELL_XPS],
+	},
+	{ }
+};
+
 /*
  * On some machines once I8K_SMM_GET_FAN_TYPE is issued then CPU fan speed
  * randomly going up and down due to bug in Dell SMM or BIOS. Here is blacklist
@@ -1438,7 +1455,7 @@ static void __init dell_smm_init_dmi(void)
 	 * Set fan multiplier and maximal fan speed from DMI config.
 	 * Values specified in module parameters override values from DMI.
 	 */
-	id = dmi_first_match(i8k_dmi_table);
+	id = dmi_first_match(i8k_config_dmi_table);
 	if (id && id->driver_data) {
 		config = id->driver_data;
 		if (!fan_mult && config->fan_mult)
--
2.39.2

[PATCH v3 6/9] hwmon: (dell-smm) Introduce helper function for data init

[Armin Wolf]

In the future, multiple SMM calling backends will exist,
with each backend being required to initialize its data.
Introduce a helper function for this so the code necessary
to initialize dell_smm_data is not duplicated between
different backends.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 46 +++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 4a2c0d1ffa5b..2547b09929e6 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -623,6 +623,11 @@ static void __init i8k_init_procfs(struct device *dev)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);

+	strscpy(data->bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
+		sizeof(data->bios_version));
+	strscpy(data->bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
+		sizeof(data->bios_machineid));
+
 	/* Only register exit function if creation was successful */
 	if (proc_create_data("i8k", 0, NULL, &i8k_proc_ops, data))
 		devm_add_action_or_reset(dev, i8k_exit_procfs, NULL);
@@ -1078,6 +1083,26 @@ static int __init dell_smm_init_hwmon(struct device *dev)
 	return PTR_ERR_OR_ZERO(dell_smm_hwmon_dev);
 }

+static int __init dell_smm_init_data(struct device *dev, const struct dell_smm_ops *ops)
+{
+	struct dell_smm_data *data;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->i8k_mutex);
+	dev_set_drvdata(dev, data);
+
+	data->ops = ops;
+	/* All options must not be 0 */
+	data->i8k_fan_mult = fan_mult ? : I8K_FAN_MULT;
+	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;
+	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
+
+	return 0;
+}
+
 static const struct dmi_system_id i8k_dmi_table[] __initconst = {
 	{
 		.ident = "Dell G5 5590",
@@ -1386,26 +1411,11 @@ static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {

 static int __init dell_smm_probe(struct platform_device *pdev)
 {
-	struct dell_smm_data *data;
 	int ret;

-	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	mutex_init(&data->i8k_mutex);
-	platform_set_drvdata(pdev, data);
-	data->ops = &i8k_smm_ops;
-
-	strscpy(data->bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
-		sizeof(data->bios_version));
-	strscpy(data->bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
-		sizeof(data->bios_machineid));
-
-	/* All options must not be 0 */
-	data->i8k_fan_mult = fan_mult ? : I8K_FAN_MULT;
-	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;
-	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
+	ret = dell_smm_init_data(&pdev->dev, &i8k_smm_ops);
+	if (ret < 0)
+		return ret;

 	ret = dell_smm_init_hwmon(&pdev->dev);
 	if (ret)
--
2.39.2

[PATCH v3 7/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Armin Wolf]

Some Dell machines like the Dell Optiplex 7000 do not support
the legacy SMM interface, but instead expect all SMM calls
to be issued over a special WMI interface.
Add support for this interface so users can control the fans
on those machines.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/Kconfig          |   1 +
 drivers/hwmon/dell-smm-hwmon.c | 198 +++++++++++++++++++++++++++++----
 drivers/platform/x86/wmi.c     |   1 +
 3 files changed, 181 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index cf27523eed5a..76cb05db1dcf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -512,6 +512,7 @@ config SENSORS_DS1621

 config SENSORS_DELL_SMM
 	tristate "Dell laptop SMM BIOS hwmon driver"
+	depends on ACPI_WMI
 	depends on X86
 	imply THERMAL
 	help
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 2547b09929e6..d1bcfd447bb0 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -12,6 +12,7 @@

 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/acpi.h>
 #include <linux/capability.h>
 #include <linux/cpu.h>
 #include <linux/ctype.h>
@@ -34,8 +35,10 @@
 #include <linux/thermal.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <linux/wmi.h>

 #include <linux/i8k.h>
+#include <asm/unaligned.h>

 #define I8K_SMM_FN_STATUS	0x0025
 #define I8K_SMM_POWER_STATUS	0x0069
@@ -66,6 +69,9 @@
 #define I8K_POWER_AC		0x05
 #define I8K_POWER_BATTERY	0x01

+#define DELL_SMM_WMI_GUID	"F1DDEE52-063C-4784-A11E-8A06684B9B01"
+#define DELL_SMM_LEGACY_EXECUTE	0x1
+
 #define DELL_SMM_NO_TEMP	10
 #define DELL_SMM_NO_FANS	3

@@ -219,6 +225,102 @@ static const struct dell_smm_ops i8k_smm_ops = {
 	.smm_call = i8k_smm_call,
 };

+/*
+ * Call the System Management Mode BIOS over WMI.
+ */
+static int wmi_parse_register(u8 *buffer, u32 length, int *reg)
+{
+	__le32 value;
+	u32 reg_size;
+
+	if (length <= sizeof(reg_size))
+		return -ENODATA;
+
+	reg_size = get_unaligned_le32(buffer);
+	if (!reg_size || reg_size > sizeof(value))
+		return -ENOMSG;
+
+	if (length < sizeof(reg_size) + reg_size)
+		return -ENODATA;
+
+	memcpy_and_pad(&value, sizeof(value), buffer + sizeof(reg_size), reg_size, 0);
+	*reg = le32_to_cpu(value);
+
+	return (int)(reg_size + sizeof(reg_size));
+}
+
+static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
+{
+	int *registers[] = {
+		&regs->eax,
+		&regs->ebx,
+		&regs->ecx,
+		&regs->edx
+	};
+	u32 offset = 0;
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(registers); i++) {
+		if (offset >= length)
+			return -ENODATA;
+
+		ret = wmi_parse_register(buffer + offset, length - offset, registers[i]);
+		if (ret < 0)
+			return ret;
+
+		offset += ret;
+	}
+
+	if (offset != length)
+		return -ENOMSG;
+
+	return 0;
+}
+
+static int wmi_smm_call(struct device *dev, struct smm_regs *regs)
+{
+	struct wmi_device *wdev = container_of(dev, struct wmi_device, dev);
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	u32 wmi_payload[] = {
+		sizeof(regs->eax),
+		regs->eax,
+		sizeof(regs->ebx),
+		regs->ebx,
+		sizeof(regs->ecx),
+		regs->ecx,
+		sizeof(regs->edx),
+		regs->edx
+	};
+	const struct acpi_buffer in = {
+		.length = sizeof(wmi_payload),
+		.pointer = &wmi_payload,
+	};
+	union acpi_object *obj;
+	acpi_status status;
+	int ret;
+
+	status = wmidev_evaluate_method(wdev, 0x0, DELL_SMM_LEGACY_EXECUTE, &in, &out);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	obj = out.pointer;
+	if (!obj)
+		return -ENODATA;
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		ret = -ENOMSG;
+
+		goto err_free;
+	}
+
+	ret = wmi_parse_response(obj->buffer.pointer, obj->buffer.length, regs);
+
+err_free:
+	kfree(obj);
+
+	return ret;
+}
+
 static int dell_smm_call(const struct dell_smm_ops *ops, struct smm_regs *regs)
 {
 	int eax = regs->eax;
@@ -306,7 +408,7 @@ static int i8k_get_fan_type(struct dell_smm_data *data, u8 fan)
 /*
  * Read the fan nominal rpm for specific fan speed.
  */
-static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8 fan, int speed)
+static int i8k_get_fan_nominal_speed(const struct dell_smm_data *data, u8 fan, int speed)
 {
 	struct smm_regs regs = {
 		.eax = I8K_SMM_GET_NOM_SPEED,
@@ -349,7 +451,7 @@ static int i8k_set_fan(const struct dell_smm_data *data, u8 fan, int speed)
 	return dell_smm_call(data->ops, &regs);
 }

-static int __init i8k_get_temp_type(const struct dell_smm_data *data, u8 sensor)
+static int i8k_get_temp_type(const struct dell_smm_data *data, u8 sensor)
 {
 	struct smm_regs regs = {
 		.eax = I8K_SMM_GET_TEMP_TYPE,
@@ -401,7 +503,7 @@ static int i8k_get_temp(const struct dell_smm_data *data, u8 sensor)
 	return temp;
 }

-static int __init dell_smm_get_signature(const struct dell_smm_ops *ops, int req_fn)
+static int dell_smm_get_signature(const struct dell_smm_ops *ops, int req_fn)
 {
 	struct smm_regs regs = { .eax = req_fn, };
 	int rc;
@@ -986,7 +1088,7 @@ static const struct hwmon_chip_info dell_smm_chip_info = {
 	.info = dell_smm_info,
 };

-static int __init dell_smm_init_cdev(struct device *dev, u8 fan_num)
+static int dell_smm_init_cdev(struct device *dev, u8 fan_num)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
 	struct thermal_cooling_device *cdev;
@@ -1017,7 +1119,7 @@ static int __init dell_smm_init_cdev(struct device *dev, u8 fan_num)
 	return ret;
 }

-static int __init dell_smm_init_hwmon(struct device *dev)
+static int dell_smm_init_hwmon(struct device *dev)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
 	struct device *dell_smm_hwmon_dev;
@@ -1083,7 +1185,7 @@ static int __init dell_smm_init_hwmon(struct device *dev)
 	return PTR_ERR_OR_ZERO(dell_smm_hwmon_dev);
 }

-static int __init dell_smm_init_data(struct device *dev, const struct dell_smm_ops *ops)
+static int dell_smm_init_data(struct device *dev, const struct dell_smm_ops *ops)
 {
 	struct dell_smm_data *data;

@@ -1409,6 +1511,9 @@ static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
 	{ }
 };

+/*
+ * Legacy SMM backend driver.
+ */
 static int __init dell_smm_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -1434,6 +1539,47 @@ static struct platform_driver dell_smm_driver = {

 static struct platform_device *dell_smm_device;

+/*
+ * WMI SMM backend driver.
+ */
+static int dell_smm_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+	struct dell_smm_ops *ops;
+	int ret;
+
+	ops = devm_kzalloc(&wdev->dev, sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return -ENOMEM;
+
+	ops->smm_call = wmi_smm_call;
+	ops->smm_dev = &wdev->dev;
+
+	if (dell_smm_get_signature(ops, I8K_SMM_GET_DELL_SIG1) &&
+	    dell_smm_get_signature(ops, I8K_SMM_GET_DELL_SIG2))
+		return -ENODEV;
+
+	ret = dell_smm_init_data(&wdev->dev, ops);
+	if (ret < 0)
+		return ret;
+
+	return dell_smm_init_hwmon(&wdev->dev);
+}
+
+static const struct wmi_device_id dell_smm_wmi_id_table[] = {
+	{ DELL_SMM_WMI_GUID, NULL },
+	{ }
+};
+MODULE_DEVICE_TABLE(wmi, dell_smm_wmi_id_table);
+
+static struct wmi_driver dell_smm_wmi_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.id_table = dell_smm_wmi_id_table,
+	.probe = dell_smm_wmi_probe,
+};
+
 /*
  * Probe for the presence of a supported laptop.
  */
@@ -1485,33 +1631,43 @@ static void __init dell_smm_init_dmi(void)
 	}
 }

-static int __init i8k_init(void)
+static int __init dell_smm_legacy_check(void)
 {
-	/*
-	 * Get DMI information
-	 */
 	if (!dmi_check_system(i8k_dmi_table)) {
 		if (!ignore_dmi && !force)
 			return -ENODEV;

-		pr_info("not running on a supported Dell system.\n");
+		pr_info("Probing for legacy SMM handler on unsupported machine\n");
 		pr_info("vendor=%s, model=%s, version=%s\n",
 			i8k_get_dmi_data(DMI_SYS_VENDOR),
 			i8k_get_dmi_data(DMI_PRODUCT_NAME),
 			i8k_get_dmi_data(DMI_BIOS_VERSION));
 	}

-	dell_smm_init_dmi();
-
-	/*
-	 * Get SMM Dell signature
-	 */
 	if (dell_smm_get_signature(&i8k_smm_ops, I8K_SMM_GET_DELL_SIG1) &&
 	    dell_smm_get_signature(&i8k_smm_ops, I8K_SMM_GET_DELL_SIG2)) {
 		if (!force)
 			return -ENODEV;

-		pr_err("Unable to get Dell SMM signature\n");
+		pr_warn("Forcing legacy SMM calls on a possibly incompatible machine\n");
+	}
+
+	return 0;
+}
+
+static int __init i8k_init(void)
+{
+	int ret;
+
+	dell_smm_init_dmi();
+
+	ret = dell_smm_legacy_check();
+	if (ret < 0) {
+		/*
+		 * On modern machines, SMM communication happens over WMI, meaning
+		 * the SMM handler might not react to legacy SMM calls.
+		 */
+		return wmi_driver_register(&dell_smm_wmi_driver);
 	}

 	dell_smm_device = platform_create_bundle(&dell_smm_driver, dell_smm_probe, NULL, 0, NULL,
@@ -1522,8 +1678,12 @@ static int __init i8k_init(void)

 static void __exit i8k_exit(void)
 {
-	platform_device_unregister(dell_smm_device);
-	platform_driver_unregister(&dell_smm_driver);
+	if (dell_smm_device) {
+		platform_device_unregister(dell_smm_device);
+		platform_driver_unregister(&dell_smm_driver);
+	} else {
+		wmi_driver_unregister(&dell_smm_wmi_driver);
+	}
 }

 module_init(i8k_init);
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index a78ddd83cda0..0b3e63c21d26 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -106,6 +106,7 @@ MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
 static const char * const allow_duplicates[] = {
 	"05901221-D566-11D1-B2F0-00A0C9062910",	/* wmi-bmof */
 	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
+	"F1DDEE52-063C-4784-A11E-8A06684B9B01", /* dell-smm-hwmon */
 	NULL
 };

--
2.39.2

Re: [PATCH v3 7/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Armin Wolf]

Am 14.11.23 um 15:12 schrieb Pali Rohár:

> On Monday 06 November 2023 07:43:49 Armin Wolf wrote:
>> Some Dell machines like the Dell Optiplex 7000 do not support
>> the legacy SMM interface, but instead expect all SMM calls
>> to be issued over a special WMI interface.
>> Add support for this interface so users can control the fans
>> on those machines.
>>
>> Tested-by: <serverror@serverror.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/Kconfig          |   1 +
>>   drivers/hwmon/dell-smm-hwmon.c | 198 +++++++++++++++++++++++++++++----
>>   drivers/platform/x86/wmi.c     |   1 +
>>   3 files changed, 181 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index cf27523eed5a..76cb05db1dcf 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -512,6 +512,7 @@ config SENSORS_DS1621
>>
>>   config SENSORS_DELL_SMM
>>   	tristate "Dell laptop SMM BIOS hwmon driver"
>> +	depends on ACPI_WMI
>>   	depends on X86
>>   	imply THERMAL
>>   	help
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 2547b09929e6..d1bcfd447bb0 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -12,6 +12,7 @@
>>
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>> +#include <linux/acpi.h>
>>   #include <linux/capability.h>
>>   #include <linux/cpu.h>
>>   #include <linux/ctype.h>
>> @@ -34,8 +35,10 @@
>>   #include <linux/thermal.h>
>>   #include <linux/types.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/wmi.h>
>>
>>   #include <linux/i8k.h>
>> +#include <asm/unaligned.h>
>>
>>   #define I8K_SMM_FN_STATUS	0x0025
>>   #define I8K_SMM_POWER_STATUS	0x0069
>> @@ -66,6 +69,9 @@
>>   #define I8K_POWER_AC		0x05
>>   #define I8K_POWER_BATTERY	0x01
>>
>> +#define DELL_SMM_WMI_GUID	"F1DDEE52-063C-4784-A11E-8A06684B9B01"
>> +#define DELL_SMM_LEGACY_EXECUTE	0x1
>> +
>>   #define DELL_SMM_NO_TEMP	10
>>   #define DELL_SMM_NO_FANS	3
>>
>> @@ -219,6 +225,102 @@ static const struct dell_smm_ops i8k_smm_ops = {
>>   	.smm_call = i8k_smm_call,
>>   };
>>
>> +/*
>> + * Call the System Management Mode BIOS over WMI.
>> + */
>> +static int wmi_parse_register(u8 *buffer, u32 length, int *reg)
>> +{
>> +	__le32 value;
>> +	u32 reg_size;
>> +
>> +	if (length <= sizeof(reg_size))
>> +		return -ENODATA;
>> +
>> +	reg_size = get_unaligned_le32(buffer);
>> +	if (!reg_size || reg_size > sizeof(value))
>> +		return -ENOMSG;
>> +
>> +	if (length < sizeof(reg_size) + reg_size)
>> +		return -ENODATA;
>> +
>> +	memcpy_and_pad(&value, sizeof(value), buffer + sizeof(reg_size), reg_size, 0);
> Hello! In one of the patches in this patch series you changed type of
> register from unsigned 32 bit integer to signed 32 bit integers. I'm not
> sure if this change is really intended and what is the real reason for
> it (because there is no explanation for it in the commit message). But
> this memcpy_and_pad would not work correctly for signed negative values
> because it is the highest bit which indicates negative number.
>
> In my opinion, numbers and registers are unsigned. But if you have
> figure out that they has to be treated as signed with possible negative
> values then this fact has to be somehow handled.

That change was by mistake, i will send an updated patch series once Hans
has finished his review.

>> +	*reg = le32_to_cpu(value);
>> +
>> +	return (int)(reg_size + sizeof(reg_size));
>> +}
>> +
>> +static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
>> +{
>> +	int *registers[] = {
>> +		&regs->eax,
>> +		&regs->ebx,
>> +		&regs->ecx,
>> +		&regs->edx
>> +	};
>> +	u32 offset = 0;
>> +	int ret, i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(registers); i++) {
>> +		if (offset >= length)
>> +			return -ENODATA;
>> +
>> +		ret = wmi_parse_register(buffer + offset, length - offset, registers[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		offset += ret;
>> +	}
>> +
>> +	if (offset != length)
>> +		return -ENOMSG;
>> +
>> +	return 0;
>> +}
>> +
>> +static int wmi_smm_call(struct device *dev, struct smm_regs *regs)
>> +{
>> +	struct wmi_device *wdev = container_of(dev, struct wmi_device, dev);
>> +	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
>> +	u32 wmi_payload[] = {
>> +		sizeof(regs->eax),
>> +		regs->eax,
>> +		sizeof(regs->ebx),
>> +		regs->ebx,
>> +		sizeof(regs->ecx),
>> +		regs->ecx,
>> +		sizeof(regs->edx),
>> +		regs->edx
>> +	};
>> +	const struct acpi_buffer in = {
>> +		.length = sizeof(wmi_payload),
>> +		.pointer = &wmi_payload,
>> +	};
>> +	union acpi_object *obj;
>> +	acpi_status status;
>> +	int ret;
>> +
>> +	status = wmidev_evaluate_method(wdev, 0x0, DELL_SMM_LEGACY_EXECUTE, &in, &out);
>> +	if (ACPI_FAILURE(status))
>> +		return -EIO;
>> +
>> +	obj = out.pointer;
>> +	if (!obj)
>> +		return -ENODATA;
>> +
>> +	if (obj->type != ACPI_TYPE_BUFFER) {
>> +		ret = -ENOMSG;
>> +
>> +		goto err_free;
>> +	}
>> +
>> +	ret = wmi_parse_response(obj->buffer.pointer, obj->buffer.length, regs);
>> +
>> +err_free:
>> +	kfree(obj);
>> +
>> +	return ret;
>> +}
>> +
> ...
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index a78ddd83cda0..0b3e63c21d26 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -106,6 +106,7 @@ MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
>>   static const char * const allow_duplicates[] = {
>>   	"05901221-D566-11D1-B2F0-00A0C9062910",	/* wmi-bmof */
>>   	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
>> +	"F1DDEE52-063C-4784-A11E-8A06684B9B01", /* dell-smm-hwmon */
> Here you used space instead of TAB after the comma.

You are right, i will fix this in the updated patch series.

Thanks,
Armin Wolf

>>   	NULL
>>   };
>>
>> --
>> 2.39.2
>>

[PATCH v3 8/9] hwmon: (dell-smm) Document the WMI SMM interface

[Armin Wolf]

Document the WMI SMM interface so that future developers
can better understand how it works.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/dell-smm-hwmon.rst | 38 ++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index d8f1d6859b96..977263cb57a8 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -186,8 +186,7 @@ SMM Interface
 The driver uses the SMM interface to send commands to the system BIOS.
 This interface is normally used by Dell's 32-bit diagnostic program or
 on newer notebook models by the buildin BIOS diagnostics.
-The SMM is triggered by writing to the special ioports ``0xb2`` and ``0x84``,
-and may cause short hangs when the BIOS code is taking too long to
+The SMM may cause short hangs when the BIOS code is taking too long to
 execute.

 The SMM handler inside the system BIOS looks at the contents of the
@@ -210,7 +209,40 @@ The SMM handler can signal a failure by either:

 - setting the lower sixteen bits of ``eax`` to ``0xffff``
 - not modifying ``eax`` at all
-- setting the carry flag
+- setting the carry flag (legacy SMM interface only)
+
+Legacy SMM Interface
+--------------------
+
+When using the legacy SMM interface, a SMM is triggered by writing the least significant byte
+of the command code to the special ioports ``0xb2`` and ``0x84``. This interface is not
+described inside the ACPI tables and can thus only be detected by issuing a test SMM call.
+
+WMI SMM Interface
+-----------------
+
+On modern Dell machines, the SMM calls are done over ACPI WMI:
+
+::
+
+ #pragma namespace("\\\\.\\root\\dcim\\sysman\\diagnostics")
+ [WMI, Provider("Provider_DiagnosticsServices"), Dynamic, Locale("MS\\0x409"),
+  Description("RunDellDiag"), guid("{F1DDEE52-063C-4784-A11E-8A06684B9B01}")]
+ class LegacyDiags {
+  [key, read] string InstanceName;
+  [read] boolean Active;
+
+  [WmiMethodId(1), Implemented, read, write, Description("Legacy Method ")]
+  void Execute([in, out] uint32 EaxLen, [in, out, WmiSizeIs("EaxLen") : ToInstance] uint8 EaxVal[],
+               [in, out] uint32 EbxLen, [in, out, WmiSizeIs("EbxLen") : ToInstance] uint8 EbxVal[],
+               [in, out] uint32 EcxLen, [in, out, WmiSizeIs("EcxLen") : ToInstance] uint8 EcxVal[],
+               [in, out] uint32 EdxLen, [in, out, WmiSizeIs("EdxLen") : ToInstance] uint8 EdxVal[]);
+ };
+
+Some machines support only the WMI SMM interface, while some machines support both interfaces.
+The driver automatically detects which interfaces are present and will use the WMI SMM interface
+if the legacy SMM interface is not present. The WMI SMM interface is usually slower than the
+legacy SMM interface since ACPI methods need to be called in order to trigger a SMM.

 SMM command codes
 -----------------
--
2.39.2

[PATCH v3 9/9] hwmon: (dell-smm) Add Optiplex 7000 to fan control whitelist

[Armin Wolf]

A user reported that on this machine, disabling BIOS fan control
is necessary in order to change the fan speed.

Tested-by: <serverror@serverror.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index d1bcfd447bb0..bacd71043a16 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1508,6 +1508,14 @@ static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
 		},
 		.driver_data = (void *)&i8k_fan_control_data[I8K_FAN_34A3_35A3],
 	},
+	{
+		.ident = "Dell Optiplex 7000",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "OptiPlex 7000"),
+		},
+		.driver_data = (void *)&i8k_fan_control_data[I8K_FAN_34A3_35A3],
+	},
 	{ }
 };

--
2.39.2

Re: [PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Armin Wolf]

Am 06.11.23 um 07:43 schrieb Armin Wolf:

> This patch series adds support for an alternative SMM calling
> backend to the dell-smm-hwmon driver. The reason for this is
> that on some modern machines, the legacy SMM calling interface
> does not work anymore and the SMM handler can be called over
> ACPI WMI instead.
>
> The first four patches prepare the driver by allowing to
> specify different SMM calling backends, and by moving most of
> the DMI handling into i8k_init() so that the DMI tables can
> keep their __initconst attributes (the WMI SMM backend driver
> does not probe at module init time). The fifth patch does some
> minor cleanup, while the next three patches implement the new
> WMI SMM calling backend. The last patch adds the machine of
> the user who requested and tested the changes to the fan control
> whitelist.
>
> If the driver does not detect the legacy SMM interface, either
> because the machine is not whitelisted or because the SMM handler
> does not react, it registers an WMI driver which will then bound
> to the WMI SMM interface and do the remaining initialization.
>
> The deprecated procfs interface is not supported when using the
> WMI SMM calling backend for the following reason: the WMI driver
> can potentially be instantiated multiple times while the deprectated
> procfs interface cannot. This should not cause any regressions
> because on machines supporting only the WMI SMM interface, the
> driver would, until now, not load anyway.
>
> All patches where tested on a Dell Inspiron 3505 and a Dell
> OptiPlex 7000.

Any thoughts on this?

Armin Wolf

>
> Changes since v2:
> - Rework WMI response parsing
> - Use #define for method number
>
> Changes since v1:
> - Cc platform driver maintainers
> - Fix formating inside documentation
>
> Armin Wolf (9):
>    hwmon: (dell-smm) Prepare for multiple SMM calling backends
>    hwmon: (dell-smm) Move blacklist handling to module init
>    hwmon: (dell-smm) Move whitelist handling to module init
>    hwmon: (dell-smm) Move DMI config handling to module init
>    hwmon: (dell-smm) Move config entries out of i8k_dmi_table
>    hwmon: (dell-smm) Introduce helper function for data init
>    hwmon: (dell-smm) Add support for WMI SMM interface
>    hwmon: (dell-smm) Document the WMI SMM interface
>    hwmon: (dell-smm) Add Optiplex 7000 to fan control whitelist
>
>   Documentation/hwmon/dell-smm-hwmon.rst |  38 +-
>   drivers/hwmon/Kconfig                  |   1 +
>   drivers/hwmon/dell-smm-hwmon.c         | 603 +++++++++++++++++--------
>   drivers/platform/x86/wmi.c             |   1 +
>   4 files changed, 453 insertions(+), 190 deletions(-)
>
> --
> 2.39.2
>

Re: [PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Hans de Goede]

Hi Armin,

On 11/13/23 20:55, Armin Wolf wrote:
> Am 06.11.23 um 07:43 schrieb Armin Wolf:
> 
>> This patch series adds support for an alternative SMM calling
>> backend to the dell-smm-hwmon driver. The reason for this is
>> that on some modern machines, the legacy SMM calling interface
>> does not work anymore and the SMM handler can be called over
>> ACPI WMI instead.
>>
>> The first four patches prepare the driver by allowing to
>> specify different SMM calling backends, and by moving most of
>> the DMI handling into i8k_init() so that the DMI tables can
>> keep their __initconst attributes (the WMI SMM backend driver
>> does not probe at module init time). The fifth patch does some
>> minor cleanup, while the next three patches implement the new
>> WMI SMM calling backend. The last patch adds the machine of
>> the user who requested and tested the changes to the fan control
>> whitelist.
>>
>> If the driver does not detect the legacy SMM interface, either
>> because the machine is not whitelisted or because the SMM handler
>> does not react, it registers an WMI driver which will then bound
>> to the WMI SMM interface and do the remaining initialization.
>>
>> The deprecated procfs interface is not supported when using the
>> WMI SMM calling backend for the following reason: the WMI driver
>> can potentially be instantiated multiple times while the deprectated
>> procfs interface cannot. This should not cause any regressions
>> because on machines supporting only the WMI SMM interface, the
>> driver would, until now, not load anyway.
>>
>> All patches where tested on a Dell Inspiron 3505 and a Dell
>> OptiPlex 7000.
> 
> Any thoughts on this?

I was waiting for the merge window to close before
reviewing / merging patches for the next cycle.

I plan to review and hopefully merge this and your
other series sometime this week.

Regards,

Hans

Re: [PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Guenter Roeck]

On Mon, Nov 13, 2023 at 09:17:48PM +0100, Hans de Goede wrote:
> 
> I plan to review and hopefully merge this and your
> other series sometime this week.
> 

What warrants merging this series through your tree(s) instead of
through hwmon ?

Guenter

Re: [PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Hans de Goede]

Hi Guenter,

On 11/14/23 14:57, Guenter Roeck wrote:
> On Mon, Nov 13, 2023 at 09:17:48PM +0100, Hans de Goede wrote:
>>
>> I plan to review and hopefully merge this and your
>> other series sometime this week.
>>
> 
> What warrants merging this series through your tree(s) instead of
> through hwmon ?

My bad, I was not paying attention to what the patches were touching
(just a quick reply to Armin's status request), I agree that this
should be merged through the hwmon tree, sorry about the confusion.

Regards,

Hans

Re: [PATCH v3 0/9] hwmon: (dell-smm) Add support for WMI SMM interface

[Hans de Goede]

Hi,

On 11/6/23 07:43, Armin Wolf wrote:
> This patch series adds support for an alternative SMM calling
> backend to the dell-smm-hwmon driver. The reason for this is
> that on some modern machines, the legacy SMM calling interface
> does not work anymore and the SMM handler can be called over
> ACPI WMI instead.
> 
> The first four patches prepare the driver by allowing to
> specify different SMM calling backends, and by moving most of
> the DMI handling into i8k_init() so that the DMI tables can
> keep their __initconst attributes (the WMI SMM backend driver
> does not probe at module init time). The fifth patch does some
> minor cleanup, while the next three patches implement the new
> WMI SMM calling backend. The last patch adds the machine of
> the user who requested and tested the changes to the fan control
> whitelist.
> 
> If the driver does not detect the legacy SMM interface, either
> because the machine is not whitelisted or because the SMM handler
> does not react, it registers an WMI driver which will then bound
> to the WMI SMM interface and do the remaining initialization.
> 
> The deprecated procfs interface is not supported when using the
> WMI SMM calling backend for the following reason: the WMI driver
> can potentially be instantiated multiple times while the deprectated
> procfs interface cannot. This should not cause any regressions
> because on machines supporting only the WMI SMM interface, the
> driver would, until now, not load anyway.
> 
> All patches where tested on a Dell Inspiron 3505 and a Dell
> OptiPlex 7000.

Thank you for the patches.

Other then the signed int vs unsigned issue which Pali pointed
out this looks good to me, so with that fixed:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Note that the signed vs unsigned int issue needs to
be fixed in at least both struct smm_regs as well
as in the register parsing, specifically in these lines:

static int wmi_parse_register(u8 *buffer, u32 length, int *reg)

	int *registers[] = {

Also I think it might be better to use u32 instead of
"unsigned int" here. But I'll leave that choice up to you.

Regards,

Hans







> Changes since v2:
> - Rework WMI response parsing
> - Use #define for method number
> 
> Changes since v1:
> - Cc platform driver maintainers
> - Fix formating inside documentation
> 
> Armin Wolf (9):
>   hwmon: (dell-smm) Prepare for multiple SMM calling backends
>   hwmon: (dell-smm) Move blacklist handling to module init
>   hwmon: (dell-smm) Move whitelist handling to module init
>   hwmon: (dell-smm) Move DMI config handling to module init
>   hwmon: (dell-smm) Move config entries out of i8k_dmi_table
>   hwmon: (dell-smm) Introduce helper function for data init
>   hwmon: (dell-smm) Add support for WMI SMM interface
>   hwmon: (dell-smm) Document the WMI SMM interface
>   hwmon: (dell-smm) Add Optiplex 7000 to fan control whitelist
> 
>  Documentation/hwmon/dell-smm-hwmon.rst |  38 +-
>  drivers/hwmon/Kconfig                  |   1 +
>  drivers/hwmon/dell-smm-hwmon.c         | 603 +++++++++++++++++--------
>  drivers/platform/x86/wmi.c             |   1 +
>  4 files changed, 453 insertions(+), 190 deletions(-)
> 
> --
> 2.39.2
>