| 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 |
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 >
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
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 --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; }
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(+)