diff options
| author | Wai-Hong Tam <waihong@google.com> | 2021-01-13 19:05:23 -0800 |
|---|---|---|
| committer | Commit Bot <commit-bot@chromium.org> | 2021-01-15 06:15:40 +0000 |
| commit | 4a1bc7646de9d1c1cfc8bcf612ea268135b0ca79 (patch) | |
| tree | 676feb6a5d94b3884082c95169ffa6898aa800a1 /chip/npcx | |
| parent | 6b59de88156fc1ff447b29a80d25feb0441bfc48 (diff) | |
| download | chrome-ec-4a1bc7646de9d1c1cfc8bcf612ea268135b0ca79.tar.gz | |
shi: Fix TX buffer synchronization by leaving a gap on filling status
The original code fills out the entire TX buffer with status byte.
AP keeps pulsing the clock and tries reading status. EC DAM may
read the TX buffer during the write to response AP. A race may
happen.
Should leave a gap, like the gap for the PREAMABLE, when filling
TX buffer. The critical section of filling the TX buffer should be
done within the gap. It guarantees no race as DAM just reads the
old status. It may result one- or two-byte extra status response.
BRANCH=Trogdor
BUG=b:177021164,b:168682309
TEST=Reflashing different firmware images to trigger TCPC update.
No error happens for >20 rounds.
Change-Id: I842d7011f85ef9323fba2af01b9062a5166d38f4
Signed-off-by: Wai-Hong Tam <waihong@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2628456
Reviewed-by: CH Lin <chlin56@nuvoton.com>
Reviewed-by: Eric Yilun Lin <yllin@chromium.org>
Diffstat (limited to 'chip/npcx')
| -rw-r--r-- | chip/npcx/shi.c | 30 |
1 files changed, 18 insertions, 12 deletions
diff --git a/chip/npcx/shi.c b/chip/npcx/shi.c index bb2696acae..0d62f92c03 100644 --- a/chip/npcx/shi.c +++ b/chip/npcx/shi.c @@ -357,25 +357,31 @@ static void shi_parse_header(void) /* This routine fills out all SHI output buffer with status byte */ static void shi_fill_out_status(uint8_t status) { - uint8_t offset; - uint8_t *obuf_ptr; + uint8_t start, end; + uint8_t *fill_ptr; + uint8_t *fill_end; uint8_t *obuf_end; /* Disable interrupts in case the interfere by the other interrupts */ interrupt_disable(); - offset = SHI_OBUF_VALID_OFFSET; + /* + * Fill out output buffer with status byte and leave a gap for PREAMBLE. + * The gap guarantees the synchronization. The critical section should + * be done within this gap. No racing happens. + */ + start = SHI_OBUF_VALID_OFFSET; + end = ((start + SHI_OBUF_FULL_SIZE - SHI_OUT_PREAMBLE_LENGTH) + % SHI_OBUF_FULL_SIZE); - /* Fill out all output buffer with status byte */ - obuf_ptr = (uint8_t *)SHI_OBUF_START_ADDR + offset; + fill_ptr = (uint8_t *)SHI_OBUF_START_ADDR + start; + fill_end = (uint8_t *)SHI_OBUF_START_ADDR + end; obuf_end = (uint8_t *)SHI_OBUF_START_ADDR + SHI_OBUF_FULL_SIZE; - while (obuf_ptr != obuf_end) - *(obuf_ptr++) = status; - - obuf_ptr = (uint8_t *)SHI_OBUF_START_ADDR; - obuf_end = (uint8_t *)SHI_OBUF_START_ADDR + offset; - while (obuf_ptr != obuf_end) - *(obuf_ptr++) = status; + while (fill_ptr != fill_end) { + *(fill_ptr++) = status; + if (fill_ptr == obuf_end) + fill_ptr = (uint8_t *)SHI_OBUF_START_ADDR; + } /* End of critical section */ interrupt_enable(); |