[PATCH v2 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[PATCH v2 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[Denis Pauk]

Lock by a ACPI mutex that is required for support ASUS MAXIMUS VII HERO 
motherboard. In other case, all methods are returned zero instead of real 
values. Code uses acpi mutex before any IO operations in case when such 
acpi mutex is known.

Patch series adds additional check for chip ID, and if method returned 
zero, all calls by acpi_wmi are disabled. 

@Andy Shevchenko: 
>> Do you need string_helpers.h after this change?
It was not required in the original patch, as it was included as part of 
some other header and I have left includes without changes.

I have a little bit changed conditionals in "add MAXIMUS VII HERO", code 
can change access variable several times: 
* By the default, access is set to direct, 
* after code has checked that wmi methods can return something useful,
* and as the last step code has checked that mutext is existed and set 
  access back to direct.

But as for me, it should be less confusing.

What do you think?

---

Changes in v2:
- Fix commit message.
- Remove default acpi_board_ANY and use NULL instead.
- Use chip ID everywhere.
- Use an anonymous union for mutexes.
- Use temporary status varibale in acpi calls.
---

Denis Pauk (3):
  hwmon: (nct6775) Use lock function pointers in nct6775_data
  hwmon: (nct6775) Implement custom lock by ACPI mutex
  hwmon: (nct6775) add MAXIMUS VII HERO

 drivers/hwmon/nct6775.c | 364 +++++++++++++++++++++++++++++-----------
 1 file changed, 263 insertions(+), 101 deletions(-)


base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
-- 
2.33.0

[PATCH v2 1/3] hwmon: (nct6775) Use lock function pointers in nct6775_data

[Denis Pauk]

Prepare for platform specific callbacks usage:
* Use nct6775 lock function pointers in struct nct6775_data instead
  direct calls.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>

---
Changes in v2:
- Fix commit message.
---
 drivers/hwmon/nct6775.c | 195 +++++++++++++++++++++++++++++-----------
 1 file changed, 143 insertions(+), 52 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 93dca471972e..049c42ea66bb 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -1326,6 +1326,8 @@ struct nct6775_data {
 	/* nct6775_*() callbacks  */
 	u16 (*read_value)(struct nct6775_data *data, u16 reg);
 	int (*write_value)(struct nct6775_data *data, u16 reg, u16 value);
+	int (*lock)(struct nct6775_data *data);
+	void (*unlock)(struct nct6775_data *data, struct device *dev);
 };
 
 struct sensor_device_template {
@@ -1918,12 +1920,26 @@ static void nct6775_update_pwm_limits(struct device *dev)
 	}
 }
 
+static int nct6775_lock(struct nct6775_data *data)
+{
+	mutex_lock(&data->update_lock);
+
+	return 0;
+}
+
+static void nct6775_unlock(struct nct6775_data *data, struct device *dev)
+{
+	mutex_unlock(&data->update_lock);
+}
+
 static struct nct6775_data *nct6775_update_device(struct device *dev)
 {
 	struct nct6775_data *data = dev_get_drvdata(dev);
-	int i, j;
+	int i, j, err;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return data;
 
 	if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
 	    || !data->valid) {
@@ -2011,7 +2027,7 @@ static struct nct6775_data *nct6775_update_device(struct device *dev)
 		data->valid = true;
 	}
 
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return data;
 }
 
@@ -2043,11 +2059,15 @@ store_in_reg(struct device *dev, struct device_attribute *attr, const char *buf,
 	err = kstrtoul(buf, 10, &val);
 	if (err < 0)
 		return err;
-	mutex_lock(&data->update_lock);
+
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->in[nr][index] = in_to_reg(val, nr);
 	data->write_value(data, data->REG_IN_MINMAX[index - 1][nr],
 			  data->in[nr][index]);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2127,14 +2147,17 @@ store_beep(struct device *dev, struct device_attribute *attr, const char *buf,
 	if (val > 1)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	if (val)
 		data->beeps |= (1ULL << nr);
 	else
 		data->beeps &= ~(1ULL << nr);
 	data->write_value(data, data->REG_BEEP[regindex],
 			  (data->beeps >> (regindex << 3)) & 0xff);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2183,14 +2206,17 @@ store_temp_beep(struct device *dev, struct device_attribute *attr,
 	bit = data->BEEP_BITS[nr + TEMP_ALARM_BASE];
 	regindex = bit >> 3;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	if (val)
 		data->beeps |= (1ULL << bit);
 	else
 		data->beeps &= ~(1ULL << bit);
 	data->write_value(data, data->REG_BEEP[regindex],
 			  (data->beeps >> (regindex << 3)) & 0xff);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return count;
 }
@@ -2284,7 +2310,10 @@ store_fan_min(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	if (!data->has_fan_div) {
 		/* NCT6776F or NCT6779D; we know this is a 13 bit register */
 		if (!val) {
@@ -2357,7 +2386,7 @@ store_fan_min(struct device *dev, struct device_attribute *attr,
 
 write_min:
 	data->write_value(data, data->REG_FAN_MIN[nr], data->fan_min[nr]);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return count;
 }
@@ -2390,13 +2419,16 @@ store_fan_pulses(struct device *dev, struct device_attribute *attr,
 	if (val > 4)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->fan_pulses[nr] = val & 3;
 	reg = data->read_value(data, data->REG_FAN_PULSES[nr]);
 	reg &= ~(0x03 << data->FAN_PULSE_SHIFT[nr]);
 	reg |= (val & 3) << data->FAN_PULSE_SHIFT[nr];
 	data->write_value(data, data->REG_FAN_PULSES[nr], reg);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return count;
 }
@@ -2494,11 +2526,14 @@ store_temp(struct device *dev, struct device_attribute *attr, const char *buf,
 	if (err < 0)
 		return err;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->temp[index][nr] = LM75_TEMP_TO_REG(val);
 	nct6775_write_temp(data, data->reg_temp[index][nr],
 			   data->temp[index][nr]);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2527,10 +2562,13 @@ store_temp_offset(struct device *dev, struct device_attribute *attr,
 
 	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->temp_offset[nr] = val;
 	data->write_value(data, data->REG_TEMP_OFFSET[nr], val);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return count;
 }
@@ -2563,7 +2601,9 @@ store_temp_type(struct device *dev, struct device_attribute *attr,
 	if (val != 1 && val != 3 && val != 4)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
 
 	data->temp_type[nr] = val;
 	vbit = 0x02 << nr;
@@ -2584,7 +2624,7 @@ store_temp_type(struct device *dev, struct device_attribute *attr,
 	data->write_value(data, data->REG_VBAT, vbat);
 	data->write_value(data, data->REG_DIODE, diode);
 
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2704,14 +2744,17 @@ store_pwm_mode(struct device *dev, struct device_attribute *attr,
 		return count;
 	}
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->pwm_mode[nr] = val;
 	reg = data->read_value(data, data->REG_PWM_MODE[nr]);
 	reg &= ~data->PWM_MODE_MASK[nr];
 	if (!val)
 		reg |= data->PWM_MODE_MASK[nr];
 	data->write_value(data, data->REG_PWM_MODE[nr], reg);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2756,7 +2799,10 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 		return err;
 	val = clamp_val(val, minval[index], maxval[index]);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->pwm[index][nr] = val;
 	data->write_value(data, data->REG_PWM[index][nr], val);
 	if (index == 2)	{ /* floor: disable if val == 0 */
@@ -2766,7 +2812,7 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 			reg |= 0x80;
 		data->write_value(data, data->REG_TEMP_SEL[nr], reg);
 	}
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2866,7 +2912,10 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 	}
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->pwm_enable[nr] = val;
 	if (val == off) {
 		/*
@@ -2880,7 +2929,7 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
 	reg &= 0x0f;
 	reg |= pwm_enable_to_reg(val) << 4;
 	data->write_value(data, data->REG_FAN_MODE[nr], reg);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -2929,14 +2978,17 @@ store_pwm_temp_sel(struct device *dev, struct device_attribute *attr,
 	if (!(data->have_temp & BIT(val - 1)) || !data->temp_src[val - 1])
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	src = data->temp_src[val - 1];
 	data->pwm_temp_sel[nr] = src;
 	reg = data->read_value(data, data->REG_TEMP_SEL[nr]);
 	reg &= 0xe0;
 	reg |= src;
 	data->write_value(data, data->REG_TEMP_SEL[nr], reg);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return count;
 }
@@ -2973,7 +3025,10 @@ store_pwm_weight_temp_sel(struct device *dev, struct device_attribute *attr,
 		    !data->temp_src[val - 1]))
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	if (val) {
 		src = data->temp_src[val - 1];
 		data->pwm_weight_temp_sel[nr] = src;
@@ -2987,7 +3042,7 @@ store_pwm_weight_temp_sel(struct device *dev, struct device_attribute *attr,
 		reg &= 0x7f;
 		data->write_value(data, data->REG_WEIGHT_TEMP_SEL[nr], reg);
 	}
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return count;
 }
@@ -3018,10 +3073,13 @@ store_target_temp(struct device *dev, struct device_attribute *attr,
 	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), 0,
 			data->target_temp_mask);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->target_temp[nr] = val;
 	pwm_update_registers(data, nr);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3055,10 +3113,13 @@ store_target_speed(struct device *dev, struct device_attribute *attr,
 	val = clamp_val(val, 0, 1350000U);
 	speed = fan_to_reg(val, data->fan_div[nr]);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->target_speed[nr] = speed;
 	pwm_update_registers(data, nr);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3092,7 +3153,10 @@ store_temp_tolerance(struct device *dev, struct device_attribute *attr,
 	/* Limit tolerance as needed */
 	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), 0, data->tolerance_mask);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->temp_tolerance[index][nr] = val;
 	if (index)
 		pwm_update_registers(data, nr);
@@ -3100,7 +3164,7 @@ store_temp_tolerance(struct device *dev, struct device_attribute *attr,
 		data->write_value(data,
 				  data->REG_CRITICAL_TEMP_TOLERANCE[nr],
 				  val);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3169,10 +3233,13 @@ store_speed_tolerance(struct device *dev, struct device_attribute *attr,
 	/* Limit tolerance as needed */
 	val = clamp_val(val, 0, data->speed_tolerance_limit);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->target_speed_tolerance[nr] = val;
 	pwm_update_registers(data, nr);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3220,10 +3287,13 @@ store_weight_temp(struct device *dev, struct device_attribute *attr,
 
 	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), 0, 255);
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->weight_temp[index][nr] = val;
 	data->write_value(data, data->REG_WEIGHT_TEMP[index][nr], val);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3269,10 +3339,13 @@ store_fan_time(struct device *dev, struct device_attribute *attr,
 		return err;
 
 	val = step_time_to_reg(val, data->pwm_mode[nr]);
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->fan_time[index][nr] = val;
 	data->write_value(data, data->REG_FAN_TIME[index][nr], val);
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3310,7 +3383,10 @@ store_auto_pwm(struct device *dev, struct device_attribute *attr,
 			val = 0xff;
 	}
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->auto_pwm[nr][point] = val;
 	if (point < data->auto_pwm_num) {
 		data->write_value(data,
@@ -3355,7 +3431,7 @@ store_auto_pwm(struct device *dev, struct device_attribute *attr,
 			break;
 		}
 	}
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3391,7 +3467,10 @@ store_auto_temp(struct device *dev, struct device_attribute *attr,
 	if (val > 255000)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->auto_temp[nr][point] = DIV_ROUND_CLOSEST(val, 1000);
 	if (point < data->auto_pwm_num) {
 		data->write_value(data,
@@ -3401,7 +3480,7 @@ store_auto_temp(struct device *dev, struct device_attribute *attr,
 		data->write_value(data, data->REG_CRITICAL_TEMP[nr],
 				    data->auto_temp[nr][point]);
 	}
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3570,7 +3649,9 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 	if (kstrtoul(buf, 10, &val) || val != 0)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
+	ret = data->lock(data);
+	if (ret)
+		return ret;
 
 	/*
 	 * Use CR registers to clear caseopen status.
@@ -3593,7 +3674,7 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 
 	data->valid = false;	/* Force cache refresh */
 error:
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 	return count;
 }
 
@@ -3981,6 +4062,9 @@ static int nct6775_probe(struct platform_device *pdev)
 	}
 
 	mutex_init(&data->update_lock);
+	data->lock = nct6775_lock;
+	data->unlock = nct6775_unlock;
+
 	data->name = nct6775_device_names[data->kind];
 	data->bank = 0xff;		/* Force initial bank selection */
 	platform_set_drvdata(pdev, data);
@@ -4790,14 +4874,18 @@ static void nct6791_enable_io_mapping(struct nct6775_sio_data *sio_data)
 static int __maybe_unused nct6775_suspend(struct device *dev)
 {
 	struct nct6775_data *data = nct6775_update_device(dev);
+	int err;
+
+	err = data->lock(data);
+	if (err)
+		return err;
 
-	mutex_lock(&data->update_lock);
 	data->vbat = data->read_value(data, data->REG_VBAT);
 	if (data->kind == nct6775) {
 		data->fandiv1 = data->read_value(data, NCT6775_REG_FANDIV1);
 		data->fandiv2 = data->read_value(data, NCT6775_REG_FANDIV2);
 	}
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return 0;
 }
@@ -4806,10 +4894,13 @@ static int __maybe_unused nct6775_resume(struct device *dev)
 {
 	struct nct6775_data *data = dev_get_drvdata(dev);
 	struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
-	int i, j, err = 0;
+	int i, j, err;
 	u8 reg;
 
-	mutex_lock(&data->update_lock);
+	err = data->lock(data);
+	if (err)
+		return err;
+
 	data->bank = 0xff;		/* Force initial bank selection */
 
 	err = sio_data->sio_enter(sio_data);
@@ -4868,7 +4959,7 @@ static int __maybe_unused nct6775_resume(struct device *dev)
 abort:
 	/* Force re-reading all values */
 	data->valid = false;
-	mutex_unlock(&data->update_lock);
+	data->unlock(data, dev);
 
 	return err;
 }
-- 
2.33.0

[PATCH v2 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex

[Denis Pauk]

Use ACPI lock when the board has a separate lock for monitoring IO.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>

---
Changes in v2:
- Fix commit message.
- Use an anonymous union for mutexes.
- Use temporary status varibale in acpi calls.
---
 drivers/hwmon/nct6775.c | 59 +++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 049c42ea66bb..206c20a1ae9b 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -140,6 +140,7 @@ struct nct6775_sio_data {
 	int ld;
 	enum kinds kind;
 	enum sensor_access access;
+	acpi_handle acpi_wmi_mutex;
 
 	/* superio_() callbacks  */
 	void (*sio_outb)(struct nct6775_sio_data *sio_data, int reg, int val);
@@ -155,6 +156,8 @@ struct nct6775_sio_data {
 #define ASUSWMI_METHODID_RHWM		0x5248574D
 #define ASUSWMI_METHODID_WHWM		0x5748574D
 #define ASUSWMI_UNSUPPORTED_METHOD	0xFFFFFFFE
+/* Wait for up to 0.5 s to acquire the lock */
+#define ASUSWMI_LOCK_TIMEOUT_MS		500
 
 static int nct6775_asuswmi_evaluate_method(u32 method_id, u8 bank, u8 reg, u8 val, u32 *retval)
 {
@@ -1243,7 +1246,11 @@ struct nct6775_data {
 	unsigned int (*fan_from_reg)(u16 reg, unsigned int divreg);
 	unsigned int (*fan_from_reg_min)(u16 reg, unsigned int divreg);
 
-	struct mutex update_lock;
+	union {
+		struct mutex update_lock;	/* non ACPI lock */
+		acpi_handle acpi_wmi_mutex;	/* ACPI lock */
+	} mlock;
+
 	bool valid;		/* true if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 
@@ -1563,6 +1570,26 @@ static int nct6775_wmi_write_value(struct nct6775_data *data, u16 reg, u16 value
 	return res;
 }
 
+static int nct6775_wmi_lock(struct nct6775_data *data)
+{
+	acpi_status status;
+
+	status = acpi_acquire_mutex(data->mlock.acpi_wmi_mutex, NULL, ASUSWMI_LOCK_TIMEOUT_MS);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
+}
+
+static void nct6775_wmi_unlock(struct nct6775_data *data, struct device *dev)
+{
+	acpi_status status;
+
+	status = acpi_release_mutex(data->mlock.acpi_wmi_mutex, NULL);
+	if (ACPI_FAILURE(status))
+		dev_err(dev, "Failed to release mutex.");
+}
+
 /*
  * On older chips, only registers 0x50-0x5f are banked.
  * On more recent chips, all registers are banked.
@@ -1922,14 +1949,14 @@ static void nct6775_update_pwm_limits(struct device *dev)
 
 static int nct6775_lock(struct nct6775_data *data)
 {
-	mutex_lock(&data->update_lock);
+	mutex_lock(&data->mlock.update_lock);
 
 	return 0;
 }
 
 static void nct6775_unlock(struct nct6775_data *data, struct device *dev)
 {
-	mutex_unlock(&data->update_lock);
+	mutex_unlock(&data->mlock.update_lock);
 }
 
 static struct nct6775_data *nct6775_update_device(struct device *dev)
@@ -4061,9 +4088,15 @@ static int nct6775_probe(struct platform_device *pdev)
 		data->write_value = nct6775_wmi_write_value;
 	}
 
-	mutex_init(&data->update_lock);
-	data->lock = nct6775_lock;
-	data->unlock = nct6775_unlock;
+	if (sio_data->acpi_wmi_mutex) {
+		data->mlock.acpi_wmi_mutex = sio_data->acpi_wmi_mutex;
+		data->lock = nct6775_wmi_lock;
+		data->unlock = nct6775_wmi_unlock;
+	} else {
+		mutex_init(&data->mlock.update_lock);
+		data->lock = nct6775_lock;
+		data->unlock = nct6775_unlock;
+	}
 
 	data->name = nct6775_device_names[data->kind];
 	data->bank = 0xff;		/* Force initial bank selection */
@@ -5114,6 +5147,7 @@ static int __init sensors_nct6775_init(void)
 	int sioaddr[2] = { 0x2e, 0x4e };
 	enum sensor_access access = access_direct;
 	const char *board_vendor, *board_name;
+	acpi_handle acpi_wmi_mutex = NULL;
 	u8 tmp;
 
 	err = platform_driver_register(&nct6775_driver);
@@ -5159,6 +5193,7 @@ static int __init sensors_nct6775_init(void)
 		found = true;
 
 		sio_data.access = access;
+		sio_data.acpi_wmi_mutex = acpi_wmi_mutex;
 
 		if (access == access_asuswmi) {
 			sio_data.sio_outb = superio_wmi_outb;
@@ -5186,11 +5221,13 @@ static int __init sensors_nct6775_init(void)
 			res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
 			res.flags = IORESOURCE_IO;
 
-			err = acpi_check_resource_conflict(&res);
-			if (err) {
-				platform_device_put(pdev[i]);
-				pdev[i] = NULL;
-				continue;
+			if (!acpi_wmi_mutex) {
+				err = acpi_check_resource_conflict(&res);
+				if (err) {
+					platform_device_put(pdev[i]);
+					pdev[i] = NULL;
+					continue;
+				}
 			}
 
 			err = platform_device_add_resources(pdev[i], &res, 1);
-- 
2.33.0

[PATCH v2 3/3] hwmon: (nct6775) add MAXIMUS VII HERO

[Denis Pauk]

ASUS MAXIMUS VII HERO board has got a nct6775 chip, but by default
there's no use of it because of resource conflict with WMI method.

This commit adds MAXIMUS VII HERO to the list of boards and provides
ACPI mutex name that can be used as shared lock with ASUS WMI.

Logic checks that mutex is available. If mutex is not available
tries to get a chip ID by ACPI WMI interface.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
Tested-by: Olli Asikainen <olli.asikainen@gmail.com>

---
Changes in v2:
- Fix commit message.
- Remove default acpi_board_ANY and use NULL instead.
- Use chip ID everywhere.
---
 drivers/hwmon/nct6775.c | 118 ++++++++++++++++++++++++++--------------
 1 file changed, 76 insertions(+), 42 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 206c20a1ae9b..ea99f68b9146 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -5109,34 +5109,59 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
  */
 static struct platform_device *pdev[2];
 
-static const char * const asus_wmi_boards[] = {
-	"ProArt X570-CREATOR WIFI",
-	"Pro WS X570-ACE",
-	"PRIME B360-PLUS",
-	"PRIME B460-PLUS",
-	"PRIME X570-PRO",
-	"ROG CROSSHAIR VIII DARK HERO",
-	"ROG CROSSHAIR VIII FORMULA",
-	"ROG CROSSHAIR VIII HERO",
-	"ROG CROSSHAIR VIII IMPACT",
-	"ROG STRIX B550-E GAMING",
-	"ROG STRIX B550-F GAMING",
-	"ROG STRIX B550-F GAMING (WI-FI)",
-	"ROG STRIX B550-I GAMING",
-	"ROG STRIX X570-F GAMING",
-	"ROG STRIX Z390-E GAMING",
-	"ROG STRIX Z490-I GAMING",
-	"TUF GAMING B550M-PLUS",
-	"TUF GAMING B550M-PLUS (WI-FI)",
-	"TUF GAMING B550-PLUS",
-	"TUF GAMING B550-PRO",
-	"TUF GAMING X570-PLUS",
-	"TUF GAMING X570-PLUS (WI-FI)",
-	"TUF GAMING X570-PRO (WI-FI)",
-	"TUF GAMING Z490-PLUS",
-	"TUF GAMING Z490-PLUS (WI-FI)",
+struct acpi_board_info {
+	char *acpi_mutex_path;
 };
 
+#define DMI_ASUS_BOARD_INFO(name, mutex_path)			\
+static struct acpi_board_info name = {				\
+	.acpi_mutex_path = mutex_path,				\
+}
+
+DMI_ASUS_BOARD_INFO(acpi_board_MAXIMUS_VII_HERO, "\\_SB_.PCI0.LPCB.SIO1.MUT0");
+DMI_ASUS_BOARD_INFO(acpi_board_ROG_STRIX_B550_E_GAMING, "\\_SB.PCI0.SBRG.SIO1.MUT0");
+
+#define DMI_EXACT_MATCH_ASUS_BOARD_NAME(name, info) {			\
+	.matches = {								\
+		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),	\
+		DMI_EXACT_MATCH(DMI_BOARD_NAME, name),				\
+	},									\
+	.driver_data = info,							\
+}
+
+static const struct dmi_system_id asus_wmi_info_table[] = {
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("MAXIMUS VII HERO", &acpi_board_MAXIMUS_VII_HERO),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B360-PLUS", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B460-PLUS", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B550M-A (WI-FI)", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME X570-PRO", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("Pro WS X570-ACE", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ProArt X570-CREATOR WIFI", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII DARK HERO", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII FORMULA", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII HERO", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII IMPACT", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-E GAMING",
+					&acpi_board_ROG_STRIX_B550_E_GAMING),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING (WI-FI)", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-I GAMING", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX X570-F GAMING", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z390-E GAMING", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z490-I GAMING", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PLUS", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PRO", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS (WI-FI)", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS (WI-FI)", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PRO (WI-FI)", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS", NULL),
+	DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS (WI-FI)", NULL),
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, asus_wmi_info_table);
+
 static int __init sensors_nct6775_init(void)
 {
 	int i, err;
@@ -5146,28 +5171,37 @@ static int __init sensors_nct6775_init(void)
 	struct nct6775_sio_data sio_data;
 	int sioaddr[2] = { 0x2e, 0x4e };
 	enum sensor_access access = access_direct;
-	const char *board_vendor, *board_name;
+	const struct dmi_system_id *dmi_id;
+	struct acpi_board_info *board_info;
 	acpi_handle acpi_wmi_mutex = NULL;
-	u8 tmp;
+	acpi_status status;
+	u8 tmp = 0;
 
 	err = platform_driver_register(&nct6775_driver);
 	if (err)
 		return err;
 
-	board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
-	board_name = dmi_get_system_info(DMI_BOARD_NAME);
-
-	if (board_name && board_vendor &&
-	    !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
-		err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
-				   board_name);
-		if (err >= 0) {
-			/* if reading chip id via WMI succeeds, use WMI */
-			if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
-				pr_info("Using Asus WMI to access %#x chip.\n", tmp);
-				access = access_asuswmi;
-			} else {
-				pr_err("Can't read ChipID by Asus WMI.\n");
+	dmi_id = dmi_first_match(asus_wmi_info_table);
+	if (dmi_id) {
+		/* if reading chip ID via WMI succeeds, use WMI */
+		if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp) &&
+		    tmp) {
+			pr_info("Using Asus WMI to access %#x chip.\n", tmp);
+			access = access_asuswmi;
+		} else {
+			pr_err("Can't read chip ID by Asus WMI.\n");
+		}
+
+		if (dmi_id->driver_data) {
+			board_info = dmi_id->driver_data;
+			if (board_info->acpi_mutex_path) {
+				status = acpi_get_handle(NULL, board_info->acpi_mutex_path,
+							 &acpi_wmi_mutex);
+				if (!ACPI_FAILURE(status)) {
+					pr_info("Using Asus WMI mutex: %s\n",
+						board_info->acpi_mutex_path);
+					access = access_direct;
+				}
 			}
 		}
 	}
-- 
2.33.0

Re: [PATCH v2 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[Denis Pauk]

Hi,

Could you please provide a some feedback about such idea?

I have bigger list of supported boards that requires ACPI mutex lock,
but I prefer to have some feedback before send next version of patch.

I have created separate patch[1] with only boards where WMI methods is
enough. And if work on patch takes some time/additional patch
versions(for sure it will), I prefer to have that patch merged and
rebase current patch over resulted list of boards. 

Thank you.  
1. https://patchwork.kernel.org/project/linux-hwmon/list/?series=594167

On Sun, 28 Nov 2021 20:45:45 +0200
Denis Pauk <pauk.denis@gmail.com> wrote:

> Lock by a ACPI mutex that is required for support ASUS MAXIMUS VII
> HERO motherboard. In other case, all methods are returned zero
> instead of real values. Code uses acpi mutex before any IO operations
> in case when such acpi mutex is known.
> 
> Patch series adds additional check for chip ID, and if method
> returned zero, all calls by acpi_wmi are disabled. 
> 
> @Andy Shevchenko: 
> >> Do you need string_helpers.h after this change?  
> It was not required in the original patch, as it was included as part
> of some other header and I have left includes without changes.
> 
> I have a little bit changed conditionals in "add MAXIMUS VII HERO",
> code can change access variable several times: 
> * By the default, access is set to direct, 
> * after code has checked that wmi methods can return something useful,
> * and as the last step code has checked that mutext is existed and
> set access back to direct.
> 
> But as for me, it should be less confusing.
> 
> What do you think?
> 
> ---
> 
> Changes in v2:
> - Fix commit message.
> - Remove default acpi_board_ANY and use NULL instead.
> - Use chip ID everywhere.
> - Use an anonymous union for mutexes.
> - Use temporary status varibale in acpi calls.
> ---
> 
> Denis Pauk (3):
>   hwmon: (nct6775) Use lock function pointers in nct6775_data
>   hwmon: (nct6775) Implement custom lock by ACPI mutex
>   hwmon: (nct6775) add MAXIMUS VII HERO
> 
>  drivers/hwmon/nct6775.c | 364
> +++++++++++++++++++++++++++++----------- 1 file changed, 263
> insertions(+), 101 deletions(-)
> 
> 
> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf



Best regards,
             Denis.

Re: [PATCH v2 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[Guenter Roeck]

On 12/16/21 2:22 PM, Denis Pauk wrote:
> Hi,
> 
> Could you please provide a some feedback about such idea?
> 
> I have bigger list of supported boards that requires ACPI mutex lock,
> but I prefer to have some feedback before send next version of patch.
> 
> I have created separate patch[1] with only boards where WMI methods is
> enough. And if work on patch takes some time/additional patch
> versions(for sure it will), I prefer to have that patch merged and
> rebase current patch over resulted list of boards.
> 

Looking through the code, I am absolutely not happy with it. It makes
the driver even more unreadable than it already is, and on top of that
makes it vulnerable to problems in the ACPI code. Example: If ACPI fails
to unlock the mutex, the driver will end up being non-functional.

At some point, we have to face it: ASUS doesn't support Linux, and they
make it hard to access chips like this. I think the chip should be
accessed through "official" channels only if provided (ie WMI/ACPI),
or not at all.

Guenter

Re: [PATCH v2 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[Eugene Shalygin]

On Fri, 17 Dec 2021 at 17:23, Guenter Roeck <linux@roeck-us.net> wrote:

> At some point, we have to face it: ASUS doesn't support Linux, and they
> make it hard to access chips like this. I think the chip should be
> accessed through "official" channels only if provided (ie WMI/ACPI),
> or not at all.

My two cents, if you please. Unfortunately, ASUS doesn't support
Windows as well, they only support their own shitty software, and they
change the WMI methods (both names and logic). For example, just
recently they packed a full hardware monitoring solution in X470
boards in WMI, then removed it in X570 and changed hardware access
function names. In order to add support for their next WMI
implementation, one needs to thoroughly read the decompiled DSDT code,
find functions, learn their logic and test. This is hard to do
remotely, without the hardware, obviously. On the other hand it is
much easier to find the required mutex name from the DSDT code and
access the chip normally.

Best regards,
Eugene

Re: [PATCH v2 0/3] hwmon: (nct6775) Support lock by ACPI mutex

[Denis Pauk]

On Fri, 17 Dec 2021 18:14:19 +0100
Eugene Shalygin <eugene.shalygin@gmail.com> wrote:

> On Fri, 17 Dec 2021 at 17:23, Guenter Roeck <linux@roeck-us.net>
> wrote:
> 
> > At some point, we have to face it: ASUS doesn't support Linux, and
> > they make it hard to access chips like this. I think the chip
> > should be accessed through "official" channels only if provided (ie
> > WMI/ACPI), or not at all.  
> 
> My two cents, if you please. Unfortunately, ASUS doesn't support
> Windows as well, they only support their own shitty software, and they
> change the WMI methods (both names and logic). For example, just
> recently they packed a full hardware monitoring solution in X470
> boards in WMI, then removed it in X570 and changed hardware access
> function names. In order to add support for their next WMI
> implementation, one needs to thoroughly read the decompiled DSDT code,
> find functions, learn their logic and test. This is hard to do
> remotely, without the hardware, obviously. On the other hand it is
> much easier to find the required mutex name from the DSDT code and
> access the chip normally.
> 
> Best regards,
> Eugene

I will try to continue to support patch as part of
https://bugzilla.kernel.org/show_bug.cgi?id=204807. 

And If we will have some better solution or ideas, I will send updated
patch. 

Thank you!

Best regards,
             Denis.