On Tue, 22 May 2018, Brian Norris wrote: > Commit 001dde9400d5 ("mfd: cros ec: spi: Fix "in progress" error > signaling") pointed out some bad code, but its analysis and conclusion > was not 100% correct. > > It *is* correct that we should not propagate result==EC_RES_IN_PROGRESS > for transport errors, because this has a special meaning -- that we > should follow up with EC_CMD_GET_COMMS_STATUS until the EC is no longer > busy. This is definitely the wrong thing for many commands, because > among other problems, EC_CMD_GET_COMMS_STATUS doesn't actually retrieve > any RX data from the EC, so commands that expected some data back will > instead start processing junk. > > For such commands, the right answer is to either propagate the error > (and return that error to the caller) or resend the original command > (*not* EC_CMD_GET_COMMS_STATUS). > > Unfortunately, commit 001dde9400d5 forgets a crucial point: that for > some long-running operations, the EC physically cannot respond to > commands any more. For example, with EC_CMD_FLASH_ERASE, the EC may be > re-flashing its own code regions, so it can't respond to SPI interrupts. > Instead, the EC prepares us ahead of time for being busy for a "long" > time, and fills its hardware buffer with EC_SPI_PAST_END. Thus, we > expect to see several "transport" errors (or, messages filled with > EC_SPI_PAST_END). So we should really translate that to a retryable > error (-EAGAIN) and continue sending EC_CMD_GET_COMMS_STATUS until we > get a ready status. > > IOW, it is actually important to treat some of these "junk" values as > retryable errors. > > Together with commit 001dde9400d5, this resolves bugs like the > following: > > 1. EC_CMD_FLASH_ERASE now works again (with commit 001dde9400d5, we > would abort the first time we saw EC_SPI_PAST_END) > 2. Before commit 001dde9400d5, transport errors (e.g., > EC_SPI_RX_BAD_DATA) seen in other commands (e.g., > EC_CMD_RTC_GET_VALUE) used to yield junk data in the RX buffer; they > will now yield -EAGAIN return values, and tools like 'hwclock' will > simply fail instead of retrieving and re-programming undefined time > values > > Fixes: 001dde9400d5 ("mfd: cros ec: spi: Fix "in progress" error signaling") > Signed-off-by: Brian Norris <briannor...@chromium.org> > --- > drivers/mfd/cros_ec_spi.c | 24 ++++++++++++++++++++---- > drivers/platform/chrome/cros_ec_proto.c | 2 ++ > 2 files changed, 22 insertions(+), 4 deletions(-)
I'm convinced. Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog