diff mbox series

[RESEND,07/11] platform/chrome: cros_ec_proto: return -EAGAIN when retries timed out

Message ID 20220628024913.1755292-8-tzungbi@kernel.org (mailing list archive)
State Superseded
Headers show
Series platform/chrome: Kunit tests and refactor for cros_ec_cmd_xfer() | expand

Commit Message

Tzung-Bi Shih June 28, 2022, 2:49 a.m. UTC
While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
cros_ec_wait_until_complete() doesn't return an error code.

Return -EAGAIN in the case instead.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/platform/chrome/cros_ec_proto.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Guenter Roeck July 13, 2022, 6:18 p.m. UTC | #1
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> cros_ec_wait_until_complete() doesn't return an error code.
>
> Return -EAGAIN in the case instead.

Does this make sense, or should it be -ETIMEDOUT ? What does the EC do
if it is still busy (stuck ?) with executing a command and it gets
another one ?

>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 466ecb063bd6..49772a4c5353 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -167,6 +167,9 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
>                         break;
>         }
>
> +       if (i >= EC_COMMAND_RETRIES)
> +               ret = -EAGAIN;
> +
>         kfree(msg);
>         return ret;
>  }
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Tzung-Bi Shih July 14, 2022, 3:41 a.m. UTC | #2
On Wed, Jul 13, 2022 at 11:18:32AM -0700, Guenter Roeck wrote:
> On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> > EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> > cros_ec_wait_until_complete() doesn't return an error code.
> >
> > Return -EAGAIN in the case instead.
> 
> Does this make sense, or should it be -ETIMEDOUT ? What does the EC do
> if it is still busy (stuck ?) with executing a command and it gets
> another one ?

AFAIK, most existing ECs use single task for host command[1][2].  As a
result, EC won't reply if it was busying on executing a host command.
Not sure if it would change after leveraging Zephyr (if enabling multi-core
support).

EC_CMD_GET_COMMS_STATUS is the only exception.  EC executes the command in
interrupt context[3].  That's why AP can use EC_CMD_GET_COMMS_STATUS to query
the status while EC was busying on another host command.

I have no strong preference for the return code but tried to align to another
timeout case (when cros_ec_xfer_command() returned -EAGAIN for
EC_COMMAND_RETRIES times).  Do we want to separate the cases: one for -EAGAIN
and another one for -ETIMEDOUT?

[1]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#428
[2]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/board/volteer/ec.tasklist#20
[3]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#176
Guenter Roeck July 14, 2022, 2:15 p.m. UTC | #3
On Wed, Jul 13, 2022 at 8:41 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Wed, Jul 13, 2022 at 11:18:32AM -0700, Guenter Roeck wrote:
> > On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > >
> > > While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> > > EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> > > cros_ec_wait_until_complete() doesn't return an error code.
> > >
> > > Return -EAGAIN in the case instead.
> >
> > Does this make sense, or should it be -ETIMEDOUT ? What does the EC do
> > if it is still busy (stuck ?) with executing a command and it gets
> > another one ?
>
> AFAIK, most existing ECs use single task for host command[1][2].  As a
> result, EC won't reply if it was busying on executing a host command.
> Not sure if it would change after leveraging Zephyr (if enabling multi-core
> support).
>
> EC_CMD_GET_COMMS_STATUS is the only exception.  EC executes the command in
> interrupt context[3].  That's why AP can use EC_CMD_GET_COMMS_STATUS to query
> the status while EC was busying on another host command.
>
> I have no strong preference for the return code but tried to align to another
> timeout case (when cros_ec_xfer_command() returned -EAGAIN for
> EC_COMMAND_RETRIES times).  Do we want to separate the cases: one for -EAGAIN
> and another one for -ETIMEDOUT?

If -EAGAIN is used elsewhere, let's stick with it.

Thanks,
Guenter

>
> [1]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#428
> [2]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/board/volteer/ec.tasklist#20
> [3]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#176
diff mbox series

Patch

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 466ecb063bd6..49772a4c5353 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -167,6 +167,9 @@  static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
 			break;
 	}
 
+	if (i >= EC_COMMAND_RETRIES)
+		ret = -EAGAIN;
+
 	kfree(msg);
 	return ret;
 }