Messages in this thread | | | From | Daisuke Nojiri <> | Date | Fri, 18 Mar 2022 11:00:19 -0700 | Subject | Re: [PATCH v2] platform/chrome: Re-introduce cros_ec_cmd_xfer and use it for ioctls |
| |
The comment looks good to me. The rest also looks good to me (based on the FROMLIST patch on Gerrit).
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org> On Fri, Mar 18, 2022 at 10:50 AM Daisuke Nojiri <dnojiri@google.com> wrote: > > The comment looks good to me. The rest also looks good to me (based on the FROMLIST patch on Gerrit). > > On Fri, Mar 18, 2022 at 9:56 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> Commit 413dda8f2c6f ("platform/chrome: cros_ec_chardev: Use >> cros_ec_cmd_xfer_status helper") inadvertendly changed the userspace ABI. >> Previously, cros_ec ioctls would only report errors if the EC communication >> failed, and otherwise return success and the result of the EC >> communication. An EC command execution failure was reported in the EC >> response field. The above mentioned commit changed this behavior, and the >> ioctl itself would fail. This breaks userspace commands trying to analyze >> the EC command execution error since the actual EC command response is no >> longer reported to userspace. >> >> Fix the problem by re-introducing the cros_ec_cmd_xfer() helper, and use it >> to handle ioctl messages. >> >> Fixes: 413dda8f2c6f ("platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper") >> Cc: Daisuke Nojiri <dnojiri@chromium.org> >> Cc: Rob Barnes <robbarnes@google.com> >> Cc: Rajat Jain <rajatja@google.com> >> Cc: Brian Norris <briannorris@chromium.org> >> Cc: Parth Malkan <parthmalkan@google.com> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> v2: Updated comments / return value description. No functional change. >> >> drivers/platform/chrome/cros_ec_chardev.c | 2 +- >> drivers/platform/chrome/cros_ec_proto.c | 50 +++++++++++++++++---- >> include/linux/platform_data/cros_ec_proto.h | 3 ++ >> 3 files changed, 45 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c >> index e0bce869c49a..fd33de546aee 100644 >> --- a/drivers/platform/chrome/cros_ec_chardev.c >> +++ b/drivers/platform/chrome/cros_ec_chardev.c >> @@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg) >> } >> >> s_cmd->command += ec->cmd_offset; >> - ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd); >> + ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd); >> /* Only copy data to userland if data was received. */ >> if (ret < 0) >> goto exit; >> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c >> index c4caf2e2de82..ac1419881ff3 100644 >> --- a/drivers/platform/chrome/cros_ec_proto.c >> +++ b/drivers/platform/chrome/cros_ec_proto.c >> @@ -560,22 +560,28 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev) >> EXPORT_SYMBOL(cros_ec_query_all); >> >> /** >> - * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC. >> + * cros_ec_cmd_xfer() - Send a command to the ChromeOS EC. >> * @ec_dev: EC device. >> * @msg: Message to write. >> * >> - * Call this to send a command to the ChromeOS EC. This should be used instead of calling the EC's >> - * cmd_xfer() callback directly. It returns success status only if both the command was transmitted >> - * successfully and the EC replied with success status. >> + * Call this to send a command to the ChromeOS EC. This should be used instead >> + * of calling the EC's cmd_xfer() callback directly. This function does not >> + * convert EC command execution error codes to Linux error codes. Most >> + * in-kernel users will want to use cros_ec_cmd_xfer_status() instead since >> + * that function implements the conversion. >> * >> * Return: >> - * >=0 - The number of bytes transferred >> - * <0 - Linux error code >> + * >0 - EC command was executed successfully. The return value is the number >> + * of bytes returned by the EC (excluding the header). >> + * =0 - EC communication was successful. EC command execution results are >> + * reported in msg->result. The result will be EC_RES_SUCCESS if the >> + * command was executed successfully or report an EC command execution >> + * error. >> + * <0 - EC communication error. Return value is the Linux error code. >> */ >> -int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev, >> - struct cros_ec_command *msg) >> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg) >> { >> - int ret, mapped; >> + int ret; >> >> mutex_lock(&ec_dev->lock); >> if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) { >> @@ -616,6 +622,32 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev, >> ret = send_command(ec_dev, msg); >> mutex_unlock(&ec_dev->lock); >> >> + return ret; >> +} >> +EXPORT_SYMBOL(cros_ec_cmd_xfer); >> + >> +/** >> + * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC. >> + * @ec_dev: EC device. >> + * @msg: Message to write. >> + * >> + * Call this to send a command to the ChromeOS EC. This should be used instead of calling the EC's >> + * cmd_xfer() callback directly. It returns success status only if both the command was transmitted >> + * successfully and the EC replied with success status. >> + * >> + * Return: >> + * >=0 - The number of bytes transferred. >> + * <0 - Linux error code >> + */ >> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev, >> + struct cros_ec_command *msg) >> +{ >> + int ret, mapped; >> + >> + ret = cros_ec_cmd_xfer(ec_dev, msg); >> + if (ret < 0) >> + return ret; >> + >> mapped = cros_ec_map_error(msg->result); >> if (mapped) { >> dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n", >> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h >> index df3c78c92ca2..16931569adce 100644 >> --- a/include/linux/platform_data/cros_ec_proto.h >> +++ b/include/linux/platform_data/cros_ec_proto.h >> @@ -216,6 +216,9 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev, >> int cros_ec_check_result(struct cros_ec_device *ec_dev, >> struct cros_ec_command *msg); >> >> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, >> + struct cros_ec_command *msg); >> + >> int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev, >> struct cros_ec_command *msg); >> >> -- >> 2.35.1 >>
| |