summaryrefslogtreecommitdiff
path: root/chip
diff options
context:
space:
mode:
authorDustin L. Howett <dustin@howett.net>2022-12-18 10:13:00 -0600
committerChromeos LUCI <chromeos-scoped@luci-project-accounts.iam.gserviceaccount.com>2023-01-23 17:29:35 +0000
commit9ae74605306a5d5cb8e6f209fe62e09208e4577f (patch)
tree37e2d27267d729ae744c830af42ea7f14b8dae17 /chip
parent51afaca321fc4df3277568fb241e0b804bb96a60 (diff)
downloadchrome-ec-9ae74605306a5d5cb8e6f209fe62e09208e4577f.tar.gz
mchp: Remove undefined behavior in espi msvw handlers
The code in espi_msvw[12]_interrupt relies on undefined behavior today. __builtin_ctz is specified as returning values in the range [0, 31], but we are checking for 32. This behavior may be unexpected compared to the CTZ/CLZ instruction on ARM, which use the value 32 to indicate that there are no ones in the provided input. GCC 11+ optimizes the two loops below into infinite loops, as it can see that the condition will never be met. After this change, the disassembly of espi_mswv1_interrupt can be confirmed to contain an exit behind a branch. ... // r4 is loaded with girq24_result and has bits successively cleared 1a: b90c cbnz r4, 20 <espi_mswv1_interrupt+0x20> 1c: e8bd 81f0 ldmia.w sp!, {r4, r5, r6, r7, r8, pc} 20: fa94 f5a4 rbit r5, r4 ... BUG=None BRANCH=main TEST=Examined the disassembly for espi_msvw[12]_interrupt; see above Change-Id: I68a5c753233a17b6b0fb61a31f1eeccf78c00aba Signed-off-by: Dustin L. Howett <dustin@howett.net> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4114450 Reviewed-by: Jack Rosenthal <jrosenth@chromium.org> Reviewed-by: Alexandru Stan <amstan@chromium.org>
Diffstat (limited to 'chip')
-rw-r--r--chip/mchp/espi.c10
1 files changed, 4 insertions, 6 deletions
diff --git a/chip/mchp/espi.c b/chip/mchp/espi.c
index dce81448d7..552739abc7 100644
--- a/chip/mchp/espi.c
+++ b/chip/mchp/espi.c
@@ -1059,14 +1059,13 @@ static void espi_mswv1_interrupt(void)
girq24_result = MCHP_INT_RESULT(24);
MCHP_INT_SOURCE(24) = girq24_result;
- bpos = __builtin_ctz(girq24_result); /* rbit, clz sequence */
- while (bpos != 32) {
+ while (girq24_result) {
+ bpos = __builtin_ctz(girq24_result); /* rbit, clz sequence */
d = *(uint8_t *)(MCHP_ESPI_MSVW_BASE + 8 + (12 * (bpos >> 2)) +
(bpos & 0x03)) &
0x01;
(girq24_vw_handlers[bpos])(d, bpos);
girq24_result &= ~(1ul << bpos);
- bpos = __builtin_ctz(girq24_result);
}
}
DECLARE_IRQ(MCHP_IRQ_GIRQ24, espi_mswv1_interrupt, 2);
@@ -1080,14 +1079,13 @@ static void espi_msvw2_interrupt(void)
girq25_result = MCHP_INT_RESULT(25);
MCHP_INT_SOURCE(25) = girq25_result;
- bpos = __builtin_ctz(girq25_result); /* rbit, clz sequence */
- while (bpos != 32) {
+ while (girq25_result) {
+ bpos = __builtin_ctz(girq25_result); /* rbit, clz sequence */
d = *(uint8_t *)(MCHP_ESPI_MSVW_BASE + (12 * 7) + 8 +
(12 * (bpos >> 2)) + (bpos & 0x03)) &
0x01;
(girq25_vw_handlers[bpos])(d, bpos);
girq25_result &= ~(1ul << bpos);
- bpos = __builtin_ctz(girq25_result);
}
}
DECLARE_IRQ(MCHP_IRQ_GIRQ25, espi_msvw2_interrupt, 2);