lkml.org 
[lkml]   [2018]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mfd: cros_ec: retry commands when EC is known to be busy
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 <briannorris@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

\
 
 \ /
  Last update: 2018-05-23 07:58    [W:0.038 / U:2.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site