Re: [PATCH v2] platform/chrome: Re-introduce cros_ec_cmd_xfer and use it for ioctls
From: Daisuke Nojiri
Date: Fri Mar 18 2022 - 13:59:40 EST
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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Cc: Rob Barnes <robbarnes@xxxxxxxxxx>
> Cc: Rajat Jain <rajatja@xxxxxxxxxx>
> Cc: Brian Norris <briannorris@xxxxxxxxxxxx>
> Cc: Parth Malkan <parthmalkan@xxxxxxxxxx>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> 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
>