[PATCH 0/2] hw/misc/tmp105: Correct temperature limit check logic

[PATCH 0/2] hw/misc/tmp105: Correct temperature limit check logic

[Peter Maydell]

This patchseries fixes bug https://bugs.launchpad.net/qemu/+bug/1734474
which is a regression between QEMU 1.3.0 and 1.4.0 of ability to boot a
guest image on the n800 machine.  The regression was introduced by
commit cb5ef3fa1871522a08, which fixed a logic error in the tmp105
device's handling of i2c writes.  That commit is correct, but it exposed
an underlying bug in the tmp105 implementation.  Previously, we
accidentallywrote 0 to the config register when this guest tried to
write 0x36, which meant (among other things) that we left the device
in "comparator mode" rather than putting it into "interrupt mode" as
the guest wanted, and it turns out that our interrupt-mode logic was
buggy, so we would signal an over-temperature interrupt immediately
and continuously, and the guest would hang.

Patch 1 fixes a silly omission where we weren't setting the
reset values for the T_high and T_low limit registers.
Patch 2 fixes the interrupt mode limit checks.

With these two the n9800 image linked to in the bug report can
boot properly again.

thanks
-- PMM

Peter Maydell (2):
  hw/misc/tmp105: reset the T_low and T_High registers
  tmp105: Correct handling of temperature limit checks

 hw/misc/tmp105.c | 73 ++++++++++++++++++++++++++++++++++++++++++------
 hw/misc/tmp105.h |  7 +++++
 2 files changed, 71 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/2] hw/misc/tmp105: reset the T_low and T_High registers

[Peter Maydell]

The TMP105 datasheet (https://www.ti.com/lit/gpn/tmp105) says that the
power-up reset values for the T_low and T_high registers are 80 degrees C
and 75 degrees C, which are 0x500 and 0x4B0 hex according to table 5.  These
values are then shifted right by four bits to give the register reset
values, since both registers store the 12 bits of temperature data in bits
[15..4] of a 16 bit register.

We were resetting these registers to zero, which is problematic for Linux
guests which enable the alert interrupt and then immediately take an
unexpected overtemperature alert because the current temperature is above
freezing...

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/tmp105.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index b47120492a..0a4aad4854 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -225,6 +225,9 @@ static void tmp105_reset(I2CSlave *i2c)
     s->faults = tmp105_faultq[(s->config >> 3) & 3];
     s->alarm = 0;
 
+    s->limit[0] = 0x4b00; /* T_LOW, 75 degrees C */
+    s->limit[1] = 0x5000; /* T_HIGH, 80 degrees C */
+
     tmp105_interrupt_update(s);
 }
 
-- 
2.20.1

Re: [PATCH 1/2] hw/misc/tmp105: reset the T_low and T_High registers

[Cédric Le Goater]

On 11/10/20 4:00 PM, Peter Maydell wrote:
> The TMP105 datasheet (https://www.ti.com/lit/gpn/tmp105) says that the
> power-up reset values for the T_low and T_high registers are 80 degrees C
> and 75 degrees C, which are 0x500 and 0x4B0 hex according to table 5.  These
> values are then shifted right by four bits to give the register reset
> values, since both registers store the 12 bits of temperature data in bits
> [15..4] of a 16 bit register.
> 
> We were resetting these registers to zero, which is problematic for Linux
> guests which enable the alert interrupt and then immediately take an
> unexpected overtemperature alert because the current temperature is above
> freezing...
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>



> ---
>  hw/misc/tmp105.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
> index b47120492a..0a4aad4854 100644
> --- a/hw/misc/tmp105.c
> +++ b/hw/misc/tmp105.c
> @@ -225,6 +225,9 @@ static void tmp105_reset(I2CSlave *i2c)
>      s->faults = tmp105_faultq[(s->config >> 3) & 3];
>      s->alarm = 0;
> 
> +    s->limit[0] = 0x4b00; /* T_LOW, 75 degrees C */
> +    s->limit[1] = 0x5000; /* T_HIGH, 80 degrees C */
> +
>      tmp105_interrupt_update(s);
>  }
> 

[PATCH 2/2] tmp105: Correct handling of temperature limit checks

[Peter Maydell]

The TMP105 datasheet says that in Interrupt Mode (when TM==1) the device
signals an alert when the temperature equals or exceeds the T_high value and
then remains high until a device register is read or the device responds to
the SMBUS Alert Response address, or the device is put into Shutdown Mode.
Thereafter the Alert pin will only be re-signalled when temperature falls
below T_low; alert can then be cleared in the same set of ways, and the
device returns to its initial "alert when temperature goes above T_high"
mode. (If this textual description is confusing, see figure 3 in the
TI datasheet at https://www.ti.com/lit/gpn/tmp105 .)

We were misimplementing this as a simple "always alert if temperature is
above T_high or below T_low" condition, which gives a spurious alert on
startup if using the "T_high = 80 degrees C, T_low = 75 degrees C" reset
limit values.

Implement the correct (hysteresis) behaviour by tracking whether we
are currently looking for the temperature to rise over T_high or
for it to fall below T_low. Our implementation of the comparator
mode (TM==0) wasn't wrong, but rephrase it to match the way that
interrupt mode is now handled for clarity.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/tmp105.c | 70 +++++++++++++++++++++++++++++++++++++++++-------
 hw/misc/tmp105.h |  7 +++++
 2 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index 0a4aad4854..d299d9b21b 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -41,16 +41,40 @@ static void tmp105_alarm_update(TMP105State *s)
             return;
     }
 
-    if ((s->config >> 1) & 1) {					/* TM */
-        if (s->temperature >= s->limit[1])
-            s->alarm = 1;
-        else if (s->temperature < s->limit[0])
-            s->alarm = 1;
+    if (s->config >> 1 & 1) {
+        /*
+         * TM == 1 : Interrupt mode. We signal Alert when the
+         * temperature rises above T_high, and expect the guest to clear
+         * it (eg by reading a device register).
+         */
+        if (s->detect_falling) {
+            if (s->temperature < s->limit[0]) {
+                s->alarm = 1;
+                s->detect_falling = false;
+            }
+        } else {
+            if (s->temperature >= s->limit[1]) {
+                s->alarm = 1;
+                s->detect_falling = true;
+            }
+        }
     } else {
-        if (s->temperature >= s->limit[1])
-            s->alarm = 1;
-        else if (s->temperature < s->limit[0])
-            s->alarm = 0;
+        /*
+         * TM == 0 : Comparator mode. We signal Alert when the temperature
+         * rises above T_high, and stop signalling it when the temperature
+         * falls below T_low.
+         */
+        if (s->detect_falling) {
+            if (s->temperature < s->limit[0]) {
+                s->alarm = 0;
+                s->detect_falling = false;
+            }
+        } else {
+            if (s->temperature >= s->limit[1]) {
+                s->alarm = 1;
+                s->detect_falling = true;
+            }
+        }
     }
 
     tmp105_interrupt_update(s);
@@ -197,6 +221,29 @@ static int tmp105_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool detect_falling_needed(void *opaque)
+{
+    TMP105State *s = opaque;
+
+    /*
+     * We only need to migrate the detect_falling bool if it's set;
+     * for migration from older machines we assume that it is false
+     * (ie temperature is not out of range).
+     */
+    return s->detect_falling;
+}
+
+static const VMStateDescription vmstate_tmp105_detect_falling = {
+    .name = "TMP105/detect-falling",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = detect_falling_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(detect_falling, TMP105State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_tmp105 = {
     .name = "TMP105",
     .version_id = 0,
@@ -212,6 +259,10 @@ static const VMStateDescription vmstate_tmp105 = {
         VMSTATE_UINT8(alarm, TMP105State),
         VMSTATE_I2C_SLAVE(i2c, TMP105State),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_tmp105_detect_falling,
+        NULL
     }
 };
 
@@ -224,6 +275,7 @@ static void tmp105_reset(I2CSlave *i2c)
     s->config = 0;
     s->faults = tmp105_faultq[(s->config >> 3) & 3];
     s->alarm = 0;
+    s->detect_falling = false;
 
     s->limit[0] = 0x4b00; /* T_LOW, 75 degrees C */
     s->limit[1] = 0x5000; /* T_HIGH, 80 degrees C */
diff --git a/hw/misc/tmp105.h b/hw/misc/tmp105.h
index e5198fce80..7c97071ad7 100644
--- a/hw/misc/tmp105.h
+++ b/hw/misc/tmp105.h
@@ -43,6 +43,13 @@ struct TMP105State {
     int16_t limit[2];
     int faults;
     uint8_t alarm;
+    /*
+     * The TMP105 initially looks for a temperature rising above T_high;
+     * once this is detected, the condition it looks for next is the
+     * temperature falling below T_low. This flag is false when initially
+     * looking for T_high, true when looking for T_low.
+     */
+    bool detect_falling;
 };
 
 #endif
-- 
2.20.1

Re: [PATCH 2/2] tmp105: Correct handling of temperature limit checks

[Cédric Le Goater]

On 11/10/20 4:00 PM, Peter Maydell wrote:
> The TMP105 datasheet says that in Interrupt Mode (when TM==1) the device
> signals an alert when the temperature equals or exceeds the T_high value and
> then remains high until a device register is read or the device responds to
> the SMBUS Alert Response address, or the device is put into Shutdown Mode.
> Thereafter the Alert pin will only be re-signalled when temperature falls
> below T_low; alert can then be cleared in the same set of ways, and the
> device returns to its initial "alert when temperature goes above T_high"
> mode. (If this textual description is confusing, see figure 3 in the
> TI datasheet at https://www.ti.com/lit/gpn/tmp105 .)
> 
> We were misimplementing this as a simple "always alert if temperature is
> above T_high or below T_low" condition, which gives a spurious alert on
> startup if using the "T_high = 80 degrees C, T_low = 75 degrees C" reset
> limit values.
> 
> Implement the correct (hysteresis) behaviour by tracking whether we
> are currently looking for the temperature to rise over T_high or
> for it to fall below T_low. Our implementation of the comparator
> mode (TM==0) wasn't wrong, but rephrase it to match the way that
> interrupt mode is now handled for clarity.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

LGTM,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>  hw/misc/tmp105.c | 70 +++++++++++++++++++++++++++++++++++++++++-------
>  hw/misc/tmp105.h |  7 +++++
>  2 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
> index 0a4aad4854..d299d9b21b 100644
> --- a/hw/misc/tmp105.c
> +++ b/hw/misc/tmp105.c
> @@ -41,16 +41,40 @@ static void tmp105_alarm_update(TMP105State *s)
>              return;
>      }
>  
> -    if ((s->config >> 1) & 1) {					/* TM */
> -        if (s->temperature >= s->limit[1])
> -            s->alarm = 1;
> -        else if (s->temperature < s->limit[0])
> -            s->alarm = 1;
> +    if (s->config >> 1 & 1) {
> +        /*
> +         * TM == 1 : Interrupt mode. We signal Alert when the
> +         * temperature rises above T_high, and expect the guest to clear
> +         * it (eg by reading a device register).
> +         */
> +        if (s->detect_falling) {
> +            if (s->temperature < s->limit[0]) {
> +                s->alarm = 1;
> +                s->detect_falling = false;
> +            }
> +        } else {
> +            if (s->temperature >= s->limit[1]) {
> +                s->alarm = 1;
> +                s->detect_falling = true;
> +            }
> +        }
>      } else {
> -        if (s->temperature >= s->limit[1])
> -            s->alarm = 1;
> -        else if (s->temperature < s->limit[0])
> -            s->alarm = 0;
> +        /*
> +         * TM == 0 : Comparator mode. We signal Alert when the temperature
> +         * rises above T_high, and stop signalling it when the temperature
> +         * falls below T_low.
> +         */
> +        if (s->detect_falling) {
> +            if (s->temperature < s->limit[0]) {
> +                s->alarm = 0;
> +                s->detect_falling = false;
> +            }
> +        } else {
> +            if (s->temperature >= s->limit[1]) {
> +                s->alarm = 1;
> +                s->detect_falling = true;
> +            }
> +        }
>      }
>  
>      tmp105_interrupt_update(s);
> @@ -197,6 +221,29 @@ static int tmp105_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool detect_falling_needed(void *opaque)
> +{
> +    TMP105State *s = opaque;
> +
> +    /*
> +     * We only need to migrate the detect_falling bool if it's set;
> +     * for migration from older machines we assume that it is false
> +     * (ie temperature is not out of range).
> +     */
> +    return s->detect_falling;
> +}
> +
> +static const VMStateDescription vmstate_tmp105_detect_falling = {
> +    .name = "TMP105/detect-falling",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = detect_falling_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(detect_falling, TMP105State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_tmp105 = {
>      .name = "TMP105",
>      .version_id = 0,
> @@ -212,6 +259,10 @@ static const VMStateDescription vmstate_tmp105 = {
>          VMSTATE_UINT8(alarm, TMP105State),
>          VMSTATE_I2C_SLAVE(i2c, TMP105State),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_tmp105_detect_falling,
> +        NULL
>      }
>  };
>  
> @@ -224,6 +275,7 @@ static void tmp105_reset(I2CSlave *i2c)
>      s->config = 0;
>      s->faults = tmp105_faultq[(s->config >> 3) & 3];
>      s->alarm = 0;
> +    s->detect_falling = false;
>  
>      s->limit[0] = 0x4b00; /* T_LOW, 75 degrees C */
>      s->limit[1] = 0x5000; /* T_HIGH, 80 degrees C */
> diff --git a/hw/misc/tmp105.h b/hw/misc/tmp105.h
> index e5198fce80..7c97071ad7 100644
> --- a/hw/misc/tmp105.h
> +++ b/hw/misc/tmp105.h
> @@ -43,6 +43,13 @@ struct TMP105State {
>      int16_t limit[2];
>      int faults;
>      uint8_t alarm;
> +    /*
> +     * The TMP105 initially looks for a temperature rising above T_high;
> +     * once this is detected, the condition it looks for next is the
> +     * temperature falling below T_low. This flag is false when initially
> +     * looking for T_high, true when looking for T_low.
> +     */
> +    bool detect_falling;
>  };
>  
>  #endif
> 

Re: [PATCH 0/2] hw/misc/tmp105: Correct temperature limit check logic

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201110150023.25533-1-peter.maydell@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20201110150023.25533-1-peter.maydell@linaro.org
Subject: [PATCH 0/2]  hw/misc/tmp105: Correct temperature limit check logic
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1604643893-8223-1-git-send-email-zhengchuan@huawei.com -> patchew/1604643893-8223-1-git-send-email-zhengchuan@huawei.com
 - [tag update]      patchew/20201104172512.2381656-1-ehabkost@redhat.com -> patchew/20201104172512.2381656-1-ehabkost@redhat.com
 - [tag update]      patchew/20201109091719.2449141-1-f4bug@amsat.org -> patchew/20201109091719.2449141-1-f4bug@amsat.org
 * [new tag]         patchew/20201110150023.25533-1-peter.maydell@linaro.org -> patchew/20201110150023.25533-1-peter.maydell@linaro.org
Switched to a new branch 'test'
aeeb041 tmp105: Correct handling of temperature limit checks
d5c6bca hw/misc/tmp105: reset the T_low and T_High registers

=== OUTPUT BEGIN ===
1/2 Checking commit d5c6bca1904c (hw/misc/tmp105: reset the T_low and T_High registers)
2/2 Checking commit aeeb0419f673 (tmp105: Correct handling of temperature limit checks)
ERROR: spaces required around that '*' (ctx:VxV)
#120: FILE: hw/misc/tmp105.c:263:
+    .subsections = (const VMStateDescription*[]) {
                                             ^

total: 1 errors, 0 warnings, 108 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201110150023.25533-1-peter.maydell@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] hw/misc/tmp105: Correct temperature limit check logic

[Peter Maydell]

Ping for code review? Might be nice to have in 5.2, though given
how old the bug is it's scarcely critical...

Ccing the ASPEED maintainers since that's the only other
machine that uses the tmp105.

thanks
-- PMM

On Tue, 10 Nov 2020 at 15:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchseries fixes bug https://bugs.launchpad.net/qemu/+bug/1734474
> which is a regression between QEMU 1.3.0 and 1.4.0 of ability to boot a
> guest image on the n800 machine.  The regression was introduced by
> commit cb5ef3fa1871522a08, which fixed a logic error in the tmp105
> device's handling of i2c writes.  That commit is correct, but it exposed
> an underlying bug in the tmp105 implementation.  Previously, we
> accidentallywrote 0 to the config register when this guest tried to
> write 0x36, which meant (among other things) that we left the device
> in "comparator mode" rather than putting it into "interrupt mode" as
> the guest wanted, and it turns out that our interrupt-mode logic was
> buggy, so we would signal an over-temperature interrupt immediately
> and continuously, and the guest would hang.
>
> Patch 1 fixes a silly omission where we weren't setting the
> reset values for the T_high and T_low limit registers.
> Patch 2 fixes the interrupt mode limit checks.
>
> With these two the n9800 image linked to in the bug report can
> boot properly again.
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   hw/misc/tmp105: reset the T_low and T_High registers
>   tmp105: Correct handling of temperature limit checks
>
>  hw/misc/tmp105.c | 73 ++++++++++++++++++++++++++++++++++++++++++------
>  hw/misc/tmp105.h |  7 +++++
>  2 files changed, 71 insertions(+), 9 deletions(-)