[PATCH v2 0/4] hwmon (it87): Correct handling for configuration mode

[PATCH v2 0/4] hwmon (it87): Correct handling for configuration mode

[Frank Crawford]

On various Gigabyte boards, incorrectly entering configuration mode
causes the second Super-IO chip to generate LPC bus access errors.
This was preivously fixed by ensuring that the second chip receives the
code to enter configuration mode before the first chip.

On discussion with people who have access to the specification documents
it was noted that this is wrong, and you should not enter or leave
configuration mode for the second chip, as it is enable during
initialisation and should not be changed.

In particular, this was found to be the case on the Gigabyte X670E Aorus
Master board, where it was reporting a totally wrong chip ID (0x8883)
using the previous method.  This was corrected by not entering
configuration mode, and this has been found to still work with older boards.

---
Frank Crawford (4):
  Rename FEAT_CONF_NOEXIT to FEAT_NOCONF as more descriptive of requirement
  Do not enter configuration mode for some chiptypes
  Test for chipset before entering configuration mode
  Remove tests nolonger required

 drivers/hwmon/it87.c | 127 +++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 66 deletions(-)

-- 
2.44.0

[PATCH v2 1/4] hwmon (it87): Rename FEAT_CONF_NOEXIT to FEAT_NOCONF as more descriptive of requirement

[Frank Crawford]

Rename previous definitions to match the new information that they are
preinitialised as enabled and should not receive codes to enter or exit
configuration mode.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index fbe86cec6055..6eeba3a01e3c 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -320,7 +320,7 @@ struct it87_devices {
  * second SIO address. Never exit configuration mode on these
  * chips to avoid the problem.
  */
-#define FEAT_CONF_NOEXIT	BIT(19)	/* Chip should not exit conf mode */
+#define FEAT_NOCONF		BIT(19)	/* Chip conf mode enabled on startup */
 #define FEAT_FOUR_FANS		BIT(20)	/* Supports four fans */
 #define FEAT_FOUR_PWM		BIT(21)	/* Supports four fan controls */
 #define FEAT_FOUR_TEMP		BIT(22)
@@ -452,7 +452,7 @@ static const struct it87_devices it87_devices[] = {
 		.model = "IT8790E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
-		  | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_NOEXIT,
+		  | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_NOCONF,
 		.peci_mask = 0x07,
 	},
 	[it8792] = {
@@ -461,7 +461,7 @@ static const struct it87_devices it87_devices[] = {
 		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF
-		  | FEAT_CONF_NOEXIT,
+		  | FEAT_NOCONF,
 		.peci_mask = 0x07,
 		.old_peci_mask = 0x02,	/* Actually reports PCH */
 	},
@@ -507,7 +507,7 @@ static const struct it87_devices it87_devices[] = {
 		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF
-		  | FEAT_CONF_NOEXIT,
+		  | FEAT_NOCONF,
 		.peci_mask = 0x07,
 		.old_peci_mask = 0x02,	/* Actually reports PCH */
 	},
@@ -544,7 +544,7 @@ static const struct it87_devices it87_devices[] = {
 #define has_four_temp(data)	((data)->features & FEAT_FOUR_TEMP)
 #define has_six_temp(data)	((data)->features & FEAT_SIX_TEMP)
 #define has_vin3_5v(data)	((data)->features & FEAT_VIN3_5V)
-#define has_conf_noexit(data)	((data)->features & FEAT_CONF_NOEXIT)
+#define has_noconf(data)	((data)->features & FEAT_NOCONF)
 #define has_scaling(data)	((data)->features & (FEAT_12MV_ADC | \
 						     FEAT_10_9MV_ADC))
 #define has_fanctl_onoff(data)	((data)->features & FEAT_FANCTL_ONOFF)
@@ -748,7 +748,7 @@ static int smbus_disable(struct it87_data *data)
 		superio_select(data->sioaddr, PME);
 		superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG,
 			     data->ec_special_config & ~data->smbus_bitmap);
-		superio_exit(data->sioaddr, has_conf_noexit(data));
+		superio_exit(data->sioaddr, has_noconf(data));
 	}
 	return 0;
 }
@@ -765,7 +765,7 @@ static int smbus_enable(struct it87_data *data)
 		superio_select(data->sioaddr, PME);
 		superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG,
 			     data->ec_special_config);
-		superio_exit(data->sioaddr, has_conf_noexit(data));
+		superio_exit(data->sioaddr, has_noconf(data));
 	}
 	return 0;
 }
@@ -3143,7 +3143,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	}
 
 exit:
-	superio_exit(sioaddr, config ? has_conf_noexit(config) : false);
+	superio_exit(sioaddr, config ? has_noconf(config) : false);
 	return err;
 }
 
@@ -3540,7 +3540,7 @@ static void it87_resume_sio(struct platform_device *pdev)
 			     reg2c);
 	}
 
-	superio_exit(data->sioaddr, has_conf_noexit(data));
+	superio_exit(data->sioaddr, has_noconf(data));
 }
 
 static int it87_resume(struct device *dev)
-- 
2.44.0

[PATCH v2 2/4] hwmon (it87): Do not enter configuration mode for some chiptypes

[Frank Crawford]

Update the configuration mode entry code to allow conditional entry, and
apply to all calls.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 6eeba3a01e3c..396c2d3afbf7 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -117,7 +117,7 @@ static inline void superio_select(int ioreg, int ldn)
 	outb(ldn, ioreg + 1);
 }
 
-static inline int superio_enter(int ioreg)
+static inline int superio_enter(int ioreg, bool noentry)
 {
 	/*
 	 * Try to reserve ioreg and ioreg + 1 for exclusive access.
@@ -125,7 +125,8 @@ static inline int superio_enter(int ioreg)
 	if (!request_muxed_region(ioreg, 2, DRVNAME))
 		return -EBUSY;
 
-	__superio_enter(ioreg);
+	if (!noentry)
+		__superio_enter(ioreg);
 	return 0;
 }
 
@@ -742,7 +743,7 @@ static int smbus_disable(struct it87_data *data)
 	int err;
 
 	if (data->smbus_bitmap) {
-		err = superio_enter(data->sioaddr);
+		err = superio_enter(data->sioaddr, has_noconf(data));
 		if (err)
 			return err;
 		superio_select(data->sioaddr, PME);
@@ -758,7 +759,7 @@ static int smbus_enable(struct it87_data *data)
 	int err;
 
 	if (data->smbus_bitmap) {
-		err = superio_enter(data->sioaddr);
+		err = superio_enter(data->sioaddr, has_noconf(data));
 		if (err)
 			return err;
 
@@ -2674,7 +2675,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	u16 chip_type;
 	const struct it87_devices *config = NULL;
 
-	err = superio_enter(sioaddr);
+	err = superio_enter(sioaddr, false);
 	if (err)
 		return err;
 
@@ -3520,7 +3521,7 @@ static void it87_resume_sio(struct platform_device *pdev)
 	if (!data->need_in7_reroute)
 		return;
 
-	err = superio_enter(data->sioaddr);
+	err = superio_enter(data->sioaddr, has_noconf(data));
 	if (err) {
 		dev_warn(&pdev->dev,
 			 "Unable to enter Super I/O to reroute in7 (%d)",
-- 
2.44.0

[PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Frank Crawford]

Major part of the change for the new method to avoid chipset issues.

The actual update does the following:

1) Lock the memory, but does not perform a SIO entry (previously it
would have performed an SIO entry).

2) Attempt to read the chipID.  This should be safe no matter which
chip we have.

3) If step (2) fails, then perform SIO entry and retry chipID read.  For
older chips and on failure it acts similarly to prior to this patch.

4) Set the sio_data->type, similar to previously.

5) If we have not performed an SIO entry, and this is not a chip type
with the NOCONF feature, then it will perform an SIO entry at this
point.

6) Proceed with setup as prior to this patch.

7) Any following access to the SIO registers will invoke the SIO entry
and SIO exit steps unless it is a chip with the NOCONF feature set.
This was set up in the previous patches in this patchset.

8) There is also some minor update to the failure exit based on if it
had performed a SIO entry or not, in addition to the previous tests.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 52 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 396c2d3afbf7..6a77f2f6e1e1 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -2667,6 +2667,27 @@ static const struct attribute_group it87_group_auto_pwm = {
 	.is_visible = it87_auto_pwm_is_visible,
 };
 
+/*
+ * Original explanation:
+ * On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
+ * (IT8792E) needs to be in configuration mode before accessing the first
+ * due to a bug in IT8792E which otherwise results in LPC bus access errors.
+ * This needs to be done before accessing the first Super-IO chip since
+ * the second chip may have been accessed prior to loading this driver.
+ *
+ * The problem is also reported to affect IT8795E, which is used on X299 boards
+ * and has the same chip ID as IT8792E (0x8733). It also appears to affect
+ * systems with IT8790E, which is used on some Z97X-Gaming boards as well as
+ * Z87X-OC.
+ *
+ * From other information supplied:
+ * ChipIDs 0x8733, 0x8695 (early ID for IT87952E) and 0x8790 are intialised
+ * and left in configuration mode, and entering and/or exiting configuration
+ * mode is what causes the crash.
+ *
+ * The recommendation is to look up the chipID before doing any mode swap
+ * and then act accordingly.
+ */
 /* SuperIO detection - will change isa_address if a chip is found */
 static int __init it87_find(int sioaddr, unsigned short *address,
 			    struct it87_sio_data *sio_data, int chip_cnt)
@@ -2674,16 +2695,25 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	int err;
 	u16 chip_type;
 	const struct it87_devices *config = NULL;
+	bool opened = false;
 
-	err = superio_enter(sioaddr, false);
+	/* First step, lock memory but don't enter configuration mode */
+	err = superio_enter(sioaddr, true);
 	if (err)
 		return err;
 
 	err = -ENODEV;
 	chip_type = superio_inw(sioaddr, DEVID);
-	/* check first for a valid chip before forcing chip id */
-	if (chip_type == 0xffff)
-		goto exit;
+	/* Check for a valid chip before forcing chip id */
+	if (chip_type == 0xffff) {
+		/* Enter configuration mode */
+		__superio_enter(sioaddr);
+		opened = true;
+		/* and then try again */
+		chip_type = superio_inw(sioaddr, DEVID);
+		if (chip_type == 0xffff)
+			goto exit;
+	}
 
 	if (force_id_cnt == 1) {
 		/* If only one value given use for all chips */
@@ -2767,6 +2797,18 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 
 	config = &it87_devices[sio_data->type];
 
+	/*
+	 * If previously we didn't enter configuration mode and it isn't a
+	 * chip we know is initialised in configuration mode, then enter
+	 * configuration mode.
+	 *
+	 * I don't know if any such chips can exist but be defensive.
+	 */
+	if (!opened && !has_noconf(config)) {
+		__superio_enter(sioaddr);
+		opened = true;
+	}
+
 	superio_select(sioaddr, PME);
 	if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) {
 		pr_info("Device (chip %s ioreg 0x%x) not activated, skipping\n",
@@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	}
 
 exit:
-	superio_exit(sioaddr, config ? has_noconf(config) : false);
+	superio_exit(sioaddr, opened && config && has_noconf(config));
 	return err;
 }
 
-- 
2.44.0

[PATCH v2 4/4] hwmon (it87): Remove tests nolonger required

[Frank Crawford]

Remove DMI tests for boards that are known to have issues with entering
configuration mode, as now we are testing the chips directly and no
longer need to rely on matching the board.

Leave the DMI table in place, for the nVIDIA board, and any future
expansions.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 48 --------------------------------------------
 1 file changed, 48 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 6a77f2f6e1e1..b850eb3e5907 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -3683,27 +3683,6 @@ static int it87_dmi_cb(const struct dmi_system_id *dmi_entry)
 	return 1;
 }
 
-/*
- * On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
- * (IT8792E) needs to be in configuration mode before accessing the first
- * due to a bug in IT8792E which otherwise results in LPC bus access errors.
- * This needs to be done before accessing the first Super-IO chip since
- * the second chip may have been accessed prior to loading this driver.
- *
- * The problem is also reported to affect IT8795E, which is used on X299 boards
- * and has the same chip ID as IT8792E (0x8733). It also appears to affect
- * systems with IT8790E, which is used on some Z97X-Gaming boards as well as
- * Z87X-OC.
- * DMI entries for those systems will be added as they become available and
- * as the problem is confirmed to affect those boards.
- */
-static int it87_sio_force(const struct dmi_system_id *dmi_entry)
-{
-	__superio_enter(REG_4E);
-
-	return it87_dmi_cb(dmi_entry);
-};
-
 /*
  * On the Shuttle SN68PT, FAN_CTL2 is apparently not
  * connected to a fan, but to something else. One user
@@ -3726,34 +3705,7 @@ static struct it87_dmi_data nvidia_fn68pt = {
 		.driver_data = data, \
 	}
 
-#define IT87_DMI_MATCH_GBT(name, cb, data) \
-	IT87_DMI_MATCH_VND("Gigabyte Technology Co., Ltd.", name, cb, data)
-
 static const struct dmi_system_id it87_dmi_table[] __initconst = {
-	IT87_DMI_MATCH_GBT("AB350", it87_sio_force, NULL),
-		/* ? + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("AX370", it87_sio_force, NULL),
-		/* ? + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("Z97X-Gaming G1", it87_sio_force, NULL),
-		/* ? + IT8790E */
-	IT87_DMI_MATCH_GBT("TRX40 AORUS XTREME", it87_sio_force, NULL),
-		/* IT8688E + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("Z390 AORUS ULTRA-CF", it87_sio_force, NULL),
-		/* IT8688E + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("B550 AORUS PRO AC", it87_sio_force, NULL),
-		/* IT8688E + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("X570 AORUS MASTER", it87_sio_force, NULL),
-		/* IT8688E + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("X570 AORUS PRO", it87_sio_force, NULL),
-		/* IT8688E + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("X570 AORUS PRO WIFI", it87_sio_force, NULL),
-		/* IT8688E + IT8792E/IT8795E */
-	IT87_DMI_MATCH_GBT("X570S AERO G", it87_sio_force, NULL),
-		/* IT8689E + IT87952E */
-	IT87_DMI_MATCH_GBT("Z690 AORUS PRO DDR4", it87_sio_force, NULL),
-		/* IT8689E + IT87952E */
-	IT87_DMI_MATCH_GBT("Z690 AORUS PRO", it87_sio_force, NULL),
-		/* IT8689E + IT87952E */
 	IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", it87_dmi_cb, &nvidia_fn68pt),
 	{ }
 
-- 
2.44.0

Re: [PATCH v2 0/4] hwmon (it87): Correct handling for configuration mode

[Frank Crawford]

On Sat, 2024-04-27 at 18:33 +1000, Frank Crawford wrote:
> On various Gigabyte boards, incorrectly entering configuration mode
> causes the second Super-IO chip to generate LPC bus access errors.
> This was preivously fixed by ensuring that the second chip receives
> the
> code to enter configuration mode before the first chip.
> 
> On discussion with people who have access to the specification
> documents
> it was noted that this is wrong, and you should not enter or leave
> configuration mode for the second chip, as it is enable during
> initialisation and should not be changed.
> 
> In particular, this was found to be the case on the Gigabyte X670E
> Aorus
> Master board, where it was reporting a totally wrong chip ID (0x8883)
> using the previous method.  This was corrected by not entering
> configuration mode, and this has been found to still work with older
> boards.
> 
> ---

Sorry, I forgot to add what the changes are since v1:

- renamed the feature to FEAT_NOCONF and macro to has_noconf.
- reworked patch descriptions to be more explicit about the actions and
remove any reference to the BIOS.

There is no code changes of any significance.

> Frank Crawford (4):
>   Rename FEAT_CONF_NOEXIT to FEAT_NOCONF as more descriptive of
> requirement
>   Do not enter configuration mode for some chiptypes
>   Test for chipset before entering configuration mode
>   Remove tests nolonger required
> 
>  drivers/hwmon/it87.c | 127 +++++++++++++++++++++--------------------
> --
>  1 file changed, 61 insertions(+), 66 deletions(-)
> 

Re: [PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Guenter Roeck]

On 4/27/24 01:33, Frank Crawford wrote:
> Major part of the change for the new method to avoid chipset issues.
> 
> The actual update does the following:
> 
> 1) Lock the memory, but does not perform a SIO entry (previously it
> would have performed an SIO entry).
> 
> 2) Attempt to read the chipID.  This should be safe no matter which
> chip we have.
> 
> 3) If step (2) fails, then perform SIO entry and retry chipID read.  For
> older chips and on failure it acts similarly to prior to this patch.
> 
> 4) Set the sio_data->type, similar to previously.
> 
> 5) If we have not performed an SIO entry, and this is not a chip type
> with the NOCONF feature, then it will perform an SIO entry at this
> point.
> 
> 6) Proceed with setup as prior to this patch.
> 
> 7) Any following access to the SIO registers will invoke the SIO entry
> and SIO exit steps unless it is a chip with the NOCONF feature set.
> This was set up in the previous patches in this patchset.
> 
> 8) There is also some minor update to the failure exit based on if it
> had performed a SIO entry or not, in addition to the previous tests.
> 
> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> ---
>   drivers/hwmon/it87.c | 52 +++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 396c2d3afbf7..6a77f2f6e1e1 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -2667,6 +2667,27 @@ static const struct attribute_group it87_group_auto_pwm = {
>   	.is_visible = it87_auto_pwm_is_visible,
>   };
>   
> +/*
> + * Original explanation:
> + * On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
> + * (IT8792E) needs to be in configuration mode before accessing the first
> + * due to a bug in IT8792E which otherwise results in LPC bus access errors.
> + * This needs to be done before accessing the first Super-IO chip since
> + * the second chip may have been accessed prior to loading this driver.
> + *
> + * The problem is also reported to affect IT8795E, which is used on X299 boards
> + * and has the same chip ID as IT8792E (0x8733). It also appears to affect
> + * systems with IT8790E, which is used on some Z97X-Gaming boards as well as
> + * Z87X-OC.
> + *
> + * From other information supplied:
> + * ChipIDs 0x8733, 0x8695 (early ID for IT87952E) and 0x8790 are intialised
> + * and left in configuration mode, and entering and/or exiting configuration
> + * mode is what causes the crash.
> + *
> + * The recommendation is to look up the chipID before doing any mode swap
> + * and then act accordingly.
> + */
>   /* SuperIO detection - will change isa_address if a chip is found */
>   static int __init it87_find(int sioaddr, unsigned short *address,
>   			    struct it87_sio_data *sio_data, int chip_cnt)
> @@ -2674,16 +2695,25 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>   	int err;
>   	u16 chip_type;
>   	const struct it87_devices *config = NULL;
> +	bool opened = false;
>   
> -	err = superio_enter(sioaddr, false);
> +	/* First step, lock memory but don't enter configuration mode */
> +	err = superio_enter(sioaddr, true);
>   	if (err)
>   		return err;
>   
>   	err = -ENODEV;
>   	chip_type = superio_inw(sioaddr, DEVID);
> -	/* check first for a valid chip before forcing chip id */
> -	if (chip_type == 0xffff)
> -		goto exit;
> +	/* Check for a valid chip before forcing chip id */
> +	if (chip_type == 0xffff) {
> +		/* Enter configuration mode */
> +		__superio_enter(sioaddr);
> +		opened = true;
> +		/* and then try again */
> +		chip_type = superio_inw(sioaddr, DEVID);
> +		if (chip_type == 0xffff)
> +			goto exit;
> +	}
>   
>   	if (force_id_cnt == 1) {
>   		/* If only one value given use for all chips */
> @@ -2767,6 +2797,18 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>   
>   	config = &it87_devices[sio_data->type];
>   
> +	/*
> +	 * If previously we didn't enter configuration mode and it isn't a
> +	 * chip we know is initialised in configuration mode, then enter
> +	 * configuration mode.
> +	 *
> +	 * I don't know if any such chips can exist but be defensive.
> +	 */
> +	if (!opened && !has_noconf(config)) {
> +		__superio_enter(sioaddr);
> +		opened = true;
> +	}
> +
>   	superio_select(sioaddr, PME);
>   	if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) {
>   		pr_info("Device (chip %s ioreg 0x%x) not activated, skipping\n",
> @@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>   	}
>   
>   exit:
> -	superio_exit(sioaddr, config ? has_noconf(config) : false);
> +	superio_exit(sioaddr, opened && config && has_noconf(config));

If 'opened' is false, this could be an affected chip. Are you sure
that it makes sense to pass 'false' as parameter here in that case ?
Doesn't that mean that the chip might be one of the affected chips,
but the superio exit sequence would be executed anyway ?
Am I missing something ?

Guenter

Re: [PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Frank Crawford]

On Sat, 2024-04-27 at 04:11 -0700, Guenter Roeck wrote:
> On 4/27/24 01:33, Frank Crawford wrote:
...
> > @@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr,
> > unsigned short *address,
> >    }
> >   
> >   exit:
> > - superio_exit(sioaddr, config ? has_noconf(config) : false);
> > + superio_exit(sioaddr, opened && config && has_noconf(config));
> 
> If 'opened' is false, this could be an affected chip. Are you sure
> that it makes sense to pass 'false' as parameter here in that case ?
> Doesn't that mean that the chip might be one of the affected chips,
> but the superio exit sequence would be executed anyway ?
> Am I missing something ?

Ohh, you may be right, I think I have got myself confused here with
opened and how it is used in superio_exit.

I think it should be !opened, but I will just check I still haven't
messed it up.

> 
> Guenter

Thanks
Frank

Re: [PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Guenter Roeck]

On 4/27/24 04:53, Frank Crawford wrote:
> On Sat, 2024-04-27 at 04:11 -0700, Guenter Roeck wrote:
>> On 4/27/24 01:33, Frank Crawford wrote:
> ...
>>> @@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr,
>>> unsigned short *address,
>>>     }
>>>    
>>>    exit:
>>> - superio_exit(sioaddr, config ? has_noconf(config) : false);
>>> + superio_exit(sioaddr, opened && config && has_noconf(config));
>>
>> If 'opened' is false, this could be an affected chip. Are you sure
>> that it makes sense to pass 'false' as parameter here in that case ?
>> Doesn't that mean that the chip might be one of the affected chips,
>> but the superio exit sequence would be executed anyway ?
>> Am I missing something ?
> 
> Ohh, you may be right, I think I have got myself confused here with
> opened and how it is used in superio_exit.
> 

It took me a while to understand as well. The double negation of the
'noentry' parameter makes it difficult to understand.

> I think it should be !opened, but I will just check I still haven't
> messed it up.
> 

Maybe it should be something like
	!config && !opened ||		/* no or unknown chip, not enabled */
	config && has_noconf(config)	/* chip known to be affected */

which would translate "don't disable configuration mode for affected chips
and for unknown chips if configuration mode was not enabled".

Btw, I think "enabled" might be a better variable name.

Thanks,
Guenter

Re: [PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Frank Crawford]

On Sat, 2024-04-27 at 10:00 -0700, Guenter Roeck wrote:
> On 4/27/24 04:53, Frank Crawford wrote:
> > On Sat, 2024-04-27 at 04:11 -0700, Guenter Roeck wrote:
> > > On 4/27/24 01:33, Frank Crawford wrote:
> > ...
> > > > @@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr,
> > > > unsigned short *address,
> > > >     }
> > > >    
> > > >    exit:
> > > > - superio_exit(sioaddr, config ? has_noconf(config) : false);
> > > > + superio_exit(sioaddr, opened && config &&
> > > > has_noconf(config));
> > > 
> > > If 'opened' is false, this could be an affected chip. Are you
> > > sure
> > > that it makes sense to pass 'false' as parameter here in that
> > > case ?
> > > Doesn't that mean that the chip might be one of the affected
> > > chips,
> > > but the superio exit sequence would be executed anyway ?
> > > Am I missing something ?
> > 
> > Ohh, you may be right, I think I have got myself confused here with
> > opened and how it is used in superio_exit.
> > 
> 
> It took me a while to understand as well. The double negation of the
> 'noentry' parameter makes it difficult to understand.
> 
> > I think it should be !opened, but I will just check I still haven't
> > messed it up.
> > 
> 
> Maybe it should be something like
>  !config && !opened || /* no or unknown chip, not enabled */
>  config && has_noconf(config) /* chip known to be affected */
> 
> which would translate "don't disable configuration mode for affected
> chips
> and for unknown chips if configuration mode was not enabled".

In fact, I think in this case that entire expression can be simplified
to just "!opened", as we only want to perform the exit code when the
entry code was given, and it is only set to true in those cases.

The only points where there is some "concern" is where it is an unknown
chipID, in which case we should not send the exit unless we previously
needed it to get a chipID (so probably a valid chip we don't know
about), or in the case of an error, which almost certainly means that
sending the code will have no effect, and is actually the same as
previously anyway.

> 
> Btw, I think "enabled" might be a better variable name.

Yes, it probably is.  I'll rename the variable.

I'll send a new version of this patch with those updates.
> 
> Thanks,
> Guenter

Regards
Frank

Re: [PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Guenter Roeck]

On 4/27/24 21:31, Frank Crawford wrote:
> 
> On Sat, 2024-04-27 at 10:00 -0700, Guenter Roeck wrote:
>> On 4/27/24 04:53, Frank Crawford wrote:
>>> On Sat, 2024-04-27 at 04:11 -0700, Guenter Roeck wrote:
>>>> On 4/27/24 01:33, Frank Crawford wrote:
>>> ...
>>>>> @@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr,
>>>>> unsigned short *address,
>>>>>      }
>>>>>     
>>>>>     exit:
>>>>> - superio_exit(sioaddr, config ? has_noconf(config) : false);
>>>>> + superio_exit(sioaddr, opened && config &&
>>>>> has_noconf(config));
>>>>
>>>> If 'opened' is false, this could be an affected chip. Are you
>>>> sure
>>>> that it makes sense to pass 'false' as parameter here in that
>>>> case ?
>>>> Doesn't that mean that the chip might be one of the affected
>>>> chips,
>>>> but the superio exit sequence would be executed anyway ?
>>>> Am I missing something ?
>>>
>>> Ohh, you may be right, I think I have got myself confused here with
>>> opened and how it is used in superio_exit.
>>>
>>
>> It took me a while to understand as well. The double negation of the
>> 'noentry' parameter makes it difficult to understand.
>>
>>> I think it should be !opened, but I will just check I still haven't
>>> messed it up.
>>>
>>
>> Maybe it should be something like
>>   !config && !opened || /* no or unknown chip, not enabled */
>>   config && has_noconf(config) /* chip known to be affected */
>>
>> which would translate "don't disable configuration mode for affected
>> chips
>> and for unknown chips if configuration mode was not enabled".
> 
> In fact, I think in this case that entire expression can be simplified
> to just "!opened", as we only want to perform the exit code when the
> entry code was given, and it is only set to true in those cases.
> 

Not really, because it is at least theoretically possible that it was
necessary to enter configuration mode to determine that the chip
is one of the broken ones. I'll leave that up to you to make the call,
though.

Guenter

Re: [PATCH v2 3/4] hwmon (it87): Test for chipset before entering configuration mode

[Frank Crawford]

On Sun, 2024-04-28 at 00:03 -0700, Guenter Roeck wrote:
> On 4/27/24 21:31, Frank Crawford wrote:
...
> > In fact, I think in this case that entire expression can be
> > simplified
> > to just "!opened", as we only want to perform the exit code when
> > the
> > entry code was given, and it is only set to true in those cases.
> > 
> 
> Not really, because it is at least theoretically possible that it was
> necessary to enter configuration mode to determine that the chip
> is one of the broken ones. I'll leave that up to you to make the
> call,
> though.

I guess it is theoretically possible, if some other driver has done
something, but as well I guess this patch is different in that it
doesn't do the second chip first, and so is still different.

However, in the only chipset we have been able to reliably lock up
lately we haven't been able to read the chipID even after trying to
enter configuration mode, when we have locked up the chip, so I don't
think this issue will actually happen in practice.

I will still try to get hold of the configuration documentation, to see
any additional notes from the vendor.  I don't expect that soon though.
> 
> Guenter

Regards
Frank

[PATCH v2 1/4] hwmon (it87): Rename FEAT_CONF_NOEXIT to FEAT_NOCONF as more descriptive of requirement

[Frank Crawford]

Rename previous definitions to match the new information that they are
preinitialised as enabled and should not receive codes to enter or exit
configuration mode.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---

v2:
 * renamed the feature to FEAT_NOCONF and macro to has_noconf following
   review.

v3:
 * No change.

---
 drivers/hwmon/it87.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index fbe86cec6055..6eeba3a01e3c 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -320,7 +320,7 @@ struct it87_devices {
  * second SIO address. Never exit configuration mode on these
  * chips to avoid the problem.
  */
-#define FEAT_CONF_NOEXIT	BIT(19)	/* Chip should not exit conf mode */
+#define FEAT_NOCONF		BIT(19)	/* Chip conf mode enabled on startup */
 #define FEAT_FOUR_FANS		BIT(20)	/* Supports four fans */
 #define FEAT_FOUR_PWM		BIT(21)	/* Supports four fan controls */
 #define FEAT_FOUR_TEMP		BIT(22)
@@ -452,7 +452,7 @@ static const struct it87_devices it87_devices[] = {
 		.model = "IT8790E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
-		  | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_CONF_NOEXIT,
+		  | FEAT_PWM_FREQ2 | FEAT_FANCTL_ONOFF | FEAT_NOCONF,
 		.peci_mask = 0x07,
 	},
 	[it8792] = {
@@ -461,7 +461,7 @@ static const struct it87_devices it87_devices[] = {
 		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF
-		  | FEAT_CONF_NOEXIT,
+		  | FEAT_NOCONF,
 		.peci_mask = 0x07,
 		.old_peci_mask = 0x02,	/* Actually reports PCH */
 	},
@@ -507,7 +507,7 @@ static const struct it87_devices it87_devices[] = {
 		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL | FEAT_FANCTL_ONOFF
-		  | FEAT_CONF_NOEXIT,
+		  | FEAT_NOCONF,
 		.peci_mask = 0x07,
 		.old_peci_mask = 0x02,	/* Actually reports PCH */
 	},
@@ -544,7 +544,7 @@ static const struct it87_devices it87_devices[] = {
 #define has_four_temp(data)	((data)->features & FEAT_FOUR_TEMP)
 #define has_six_temp(data)	((data)->features & FEAT_SIX_TEMP)
 #define has_vin3_5v(data)	((data)->features & FEAT_VIN3_5V)
-#define has_conf_noexit(data)	((data)->features & FEAT_CONF_NOEXIT)
+#define has_noconf(data)	((data)->features & FEAT_NOCONF)
 #define has_scaling(data)	((data)->features & (FEAT_12MV_ADC | \
 						     FEAT_10_9MV_ADC))
 #define has_fanctl_onoff(data)	((data)->features & FEAT_FANCTL_ONOFF)
@@ -748,7 +748,7 @@ static int smbus_disable(struct it87_data *data)
 		superio_select(data->sioaddr, PME);
 		superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG,
 			     data->ec_special_config & ~data->smbus_bitmap);
-		superio_exit(data->sioaddr, has_conf_noexit(data));
+		superio_exit(data->sioaddr, has_noconf(data));
 	}
 	return 0;
 }
@@ -765,7 +765,7 @@ static int smbus_enable(struct it87_data *data)
 		superio_select(data->sioaddr, PME);
 		superio_outb(data->sioaddr, IT87_SPECIAL_CFG_REG,
 			     data->ec_special_config);
-		superio_exit(data->sioaddr, has_conf_noexit(data));
+		superio_exit(data->sioaddr, has_noconf(data));
 	}
 	return 0;
 }
@@ -3143,7 +3143,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	}
 
 exit:
-	superio_exit(sioaddr, config ? has_conf_noexit(config) : false);
+	superio_exit(sioaddr, config ? has_noconf(config) : false);
 	return err;
 }
 
@@ -3540,7 +3540,7 @@ static void it87_resume_sio(struct platform_device *pdev)
 			     reg2c);
 	}
 
-	superio_exit(data->sioaddr, has_conf_noexit(data));
+	superio_exit(data->sioaddr, has_noconf(data));
 }
 
 static int it87_resume(struct device *dev)
-- 
2.44.0