* [PATCH 1/8] platform/chrome: cros_ec_proto: Report command not supported
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper Enric Balletbo i Serra
` (7 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
In practice most drivers that use the EC protocol what really care is if
the result was successful or not, hence, we introduced a
cros_ec_cmd_xfer_status() function that converts EC errors to standard
Linux error codes. On some few cases, though, we are interested on know
if the command is supported or not, and in such cases, just ignore the
error. To achieve this, return a -ENOTSUPP error when the command is not
supported.
This will allow us to finish the conversion of all users to use the
cros_ec_cmd_xfer_status() function instead of cros_ec_cmd_xfer() and
make the latest private to the protocol driver, so users of the protocol
are not confused in which function they should use.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_proto.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3cfa643f1d07..3e745e0fe092 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -553,7 +553,10 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
* replied with success status. It's not necessary to check msg->result when
* using this function.
*
- * Return: The number of bytes transferred on success or negative error code.
+ * Return:
+ * >=0 - The number of bytes transferred
+ * -ENOTSUPP - Operation not supported
+ * -EPROTO - Protocol error
*/
int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg)
@@ -563,6 +566,10 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
ret = cros_ec_cmd_xfer(ec_dev, msg);
if (ret < 0) {
dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+ } else if (msg->result == EC_RES_INVALID_VERSION) {
+ dev_dbg(ec_dev->dev, "Command invalid version (err:%d)\n",
+ msg->result);
+ return -ENOTSUPP;
} else if (msg->result != EC_RES_SUCCESS) {
dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
return -EPROTO;
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 1/8] platform/chrome: cros_ec_proto: Report command not supported Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-03-02 10:08 ` Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 3/8] platform/chrome: cros_ec_vbc: " Enric Balletbo i Serra
` (6 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel
Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani,
Jonathan Cameron, Ting Shen, Neil Armstrong, Dmitry Torokhov,
Fei Shao, linux-input
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case there is no advantage of doing this
apart from that we want to make cros_ec_cmd_xfer() a private function
for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/input/keyboard/cros_ec_keyb.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 2b71c5a51f90..fc1793ca2f17 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -347,18 +347,14 @@ static int cros_ec_keyb_info(struct cros_ec_device *ec_dev,
params->info_type = info_type;
params->event_type = event_type;
- ret = cros_ec_cmd_xfer(ec_dev, msg);
- if (ret < 0) {
- dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
- (int)info_type, (int)event_type, ret);
- } else if (msg->result == EC_RES_INVALID_VERSION) {
+ ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+ if (ret == -ENOTSUPP) {
/* With older ECs we just return 0 for everything */
memset(result, 0, result_size);
ret = 0;
- } else if (msg->result != EC_RES_SUCCESS) {
- dev_warn(ec_dev->dev, "Error getting info %d/%d: %d\n",
- (int)info_type, (int)event_type, msg->result);
- ret = -EPROTO;
+ } else if (ret < 0) {
+ dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
+ (int)info_type, (int)event_type, ret);
} else if (ret != result_size) {
dev_warn(ec_dev->dev, "Wrong size %d/%d: %d != %zu\n",
(int)info_type, (int)event_type,
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 ` [PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper Enric Balletbo i Serra
@ 2020-03-02 10:08 ` Enric Balletbo i Serra
0 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-03-02 10:08 UTC (permalink / raw)
To: linux-kernel
Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani,
Jonathan Cameron, Ting Shen, Neil Armstrong, Dmitry Torokhov,
Fei Shao, linux-input
Hi Dmitry,
Gentle ping, I'd like feedback from you on this series, and are you fine with
this change?
Thanks,
Enric
On 20/2/20 16:58, Enric Balletbo i Serra wrote:
> This patch makes use of cros_ec_cmd_xfer_status() instead of
> cros_ec_cmd_xfer(). In this case there is no advantage of doing this
> apart from that we want to make cros_ec_cmd_xfer() a private function
> for the EC protocol and let people only use the
> cros_ec_cmd_xfer_status() to return Linux standard error codes.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> drivers/input/keyboard/cros_ec_keyb.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 2b71c5a51f90..fc1793ca2f17 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -347,18 +347,14 @@ static int cros_ec_keyb_info(struct cros_ec_device *ec_dev,
> params->info_type = info_type;
> params->event_type = event_type;
>
> - ret = cros_ec_cmd_xfer(ec_dev, msg);
> - if (ret < 0) {
> - dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
> - (int)info_type, (int)event_type, ret);
> - } else if (msg->result == EC_RES_INVALID_VERSION) {
> + ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> + if (ret == -ENOTSUPP) {
> /* With older ECs we just return 0 for everything */
> memset(result, 0, result_size);
> ret = 0;
> - } else if (msg->result != EC_RES_SUCCESS) {
> - dev_warn(ec_dev->dev, "Error getting info %d/%d: %d\n",
> - (int)info_type, (int)event_type, msg->result);
> - ret = -EPROTO;
> + } else if (ret < 0) {
> + dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
> + (int)info_type, (int)event_type, ret);
> } else if (ret != result_size) {
> dev_warn(ec_dev->dev, "Wrong size %d/%d: %d != %zu\n",
> (int)info_type, (int)event_type,
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/8] platform/chrome: cros_ec_vbc: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 1/8] platform/chrome: cros_ec_proto: Report command not supported Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 4/8] platform/chrome: cros_ec_chardev: " Enric Balletbo i Serra
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_vbc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
index 8edae465105c..46482d12cffe 100644
--- a/drivers/platform/chrome/cros_ec_vbc.c
+++ b/drivers/platform/chrome/cros_ec_vbc.c
@@ -40,7 +40,7 @@ static ssize_t vboot_context_read(struct file *filp, struct kobject *kobj,
msg->outsize = para_sz;
msg->insize = resp_sz;
- err = cros_ec_cmd_xfer(ecdev, msg);
+ err = cros_ec_cmd_xfer_status(ecdev, msg);
if (err < 0) {
dev_err(dev, "Error sending read request: %d\n", err);
kfree(msg);
@@ -83,7 +83,7 @@ static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
msg->outsize = para_sz;
msg->insize = 0;
- err = cros_ec_cmd_xfer(ecdev, msg);
+ err = cros_ec_cmd_xfer_status(ecdev, msg);
if (err < 0) {
dev_err(dev, "Error sending write request: %d\n", err);
kfree(msg);
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
` (2 preceding siblings ...)
2020-02-20 15:58 ` [PATCH 3/8] platform/chrome: cros_ec_vbc: " Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-25 19:55 ` Prashant Malani
2020-02-20 15:58 ` [PATCH 5/8] platform/chrome: cros_ec_sysfs: " Enric Balletbo i Serra
` (4 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_chardev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c65e70bc168d..b51ab24055f3 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(ec->ec_dev, s_cmd);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
/* Only copy data to userland if data was received. */
if (ret < 0)
goto exit;
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 ` [PATCH 4/8] platform/chrome: cros_ec_chardev: " Enric Balletbo i Serra
@ 2020-02-25 19:55 ` Prashant Malani
2020-02-26 14:59 ` Enric Balletbo i Serra
0 siblings, 1 reply; 14+ messages in thread
From: Prashant Malani @ 2020-02-25 19:55 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Collabora Kernel ML, groeck, bleung, dtor, gwendal
Hi Enric,
On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
> This patch makes use of cros_ec_cmd_xfer_status() instead of
> cros_ec_cmd_xfer(). In this case the change is trivial and the only
> reason to do it is because we want to make cros_ec_cmd_xfer() a private
> function for the EC protocol and let people only use the
> cros_ec_cmd_xfer_status() to return Linux standard error codes.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> drivers/platform/chrome/cros_ec_chardev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
> index c65e70bc168d..b51ab24055f3 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(ec->ec_dev, s_cmd);
> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
One issue I see here is that if we were to later convert
cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
s_cmd->result value, since I was hoping to avoid returning msg->result
via a pointer parameter. I don't know if userspace actually uses that, but I
am assuming it does.
So, should cros_ec_cmd() keep the *result pointer like so ?:
int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);
This way, we can manually re-populate s_cmd->result with |*result|.
Or, should we drop msg->result while returning s_cmd to userspace? I am
guessing the answer is no, but thought I'd check with you first.
Best regards,
> /* Only copy data to userland if data was received. */
> if (ret < 0)
> goto exit;
> --
> 2.25.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
2020-02-25 19:55 ` Prashant Malani
@ 2020-02-26 14:59 ` Enric Balletbo i Serra
2020-02-26 17:32 ` Prashant Malani
0 siblings, 1 reply; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-26 14:59 UTC (permalink / raw)
To: Prashant Malani
Cc: linux-kernel, Collabora Kernel ML, groeck, bleung, dtor, gwendal
Hi Prashant,
On 25/2/20 20:55, Prashant Malani wrote:
> Hi Enric,
>
> On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
>> This patch makes use of cros_ec_cmd_xfer_status() instead of
>> cros_ec_cmd_xfer(). In this case the change is trivial and the only
>> reason to do it is because we want to make cros_ec_cmd_xfer() a private
>> function for the EC protocol and let people only use the
>> cros_ec_cmd_xfer_status() to return Linux standard error codes.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> drivers/platform/chrome/cros_ec_chardev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
>> index c65e70bc168d..b51ab24055f3 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(ec->ec_dev, s_cmd);
>> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
>
> One issue I see here is that if we were to later convert
> cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
> s_cmd->result value, since I was hoping to avoid returning msg->result
> via a pointer parameter. I don't know if userspace actually uses that, but I
> am assuming it does.
>
I'd like to avoid returning msg->result via a pointer parameter too. I didn't
analyze all the cases but I suspect that in most cases it is not really needed,
so let's start by converting to the cros_ec_cmd the cases that are clear and
let's go one by one for the ones that are not clear.
IMO cros_ec_cmd should return the same as cros_ec_cmd_xfer_status. So you should
use cros_ec_cmd_xfer_status inside cros_ec_cmd.
Regards,
Enric
> So, should cros_ec_cmd() keep the *result pointer like so ?:
>
> int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
> void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);
>
> This way, we can manually re-populate s_cmd->result with |*result|.
>
> Or, should we drop msg->result while returning s_cmd to userspace? I am
> guessing the answer is no, but thought I'd check with you first.
>
> Best regards,
>
>
>> /* Only copy data to userland if data was received. */
>> if (ret < 0)
>> goto exit;
>> --
>> 2.25.0
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
2020-02-26 14:59 ` Enric Balletbo i Serra
@ 2020-02-26 17:32 ` Prashant Malani
0 siblings, 0 replies; 14+ messages in thread
From: Prashant Malani @ 2020-02-26 17:32 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Linux Kernel Mailing List, Collabora Kernel ML, Guenter Roeck,
Benson Leung, Dmitry Torokhov, Gwendal Grignou
Hi Enric,
On Wed, Feb 26, 2020 at 7:00 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 25/2/20 20:55, Prashant Malani wrote:
> > Hi Enric,
> >
> > On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
> >> This patch makes use of cros_ec_cmd_xfer_status() instead of
> >> cros_ec_cmd_xfer(). In this case the change is trivial and the only
> >> reason to do it is because we want to make cros_ec_cmd_xfer() a private
> >> function for the EC protocol and let people only use the
> >> cros_ec_cmd_xfer_status() to return Linux standard error codes.
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >> drivers/platform/chrome/cros_ec_chardev.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
> >> index c65e70bc168d..b51ab24055f3 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(ec->ec_dev, s_cmd);
> >> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
> >
> > One issue I see here is that if we were to later convert
> > cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
> > s_cmd->result value, since I was hoping to avoid returning msg->result
> > via a pointer parameter. I don't know if userspace actually uses that, but I
> > am assuming it does.
> >
>
> I'd like to avoid returning msg->result via a pointer parameter too. I didn't
> analyze all the cases but I suspect that in most cases it is not really needed,
> so let's start by converting to the cros_ec_cmd the cases that are clear and
> let's go one by one for the ones that are not clear.
>
I think the major one I was concerned about was in user-space:
https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/util/comm-dev.c#149
The above seems to return the result (after applying an offset).
FWIU the attempt would be to not change the user-space API, so would
we need to change this user-space too?
Best regards,
> IMO cros_ec_cmd should return the same as cros_ec_cmd_xfer_status. So you should
> use cros_ec_cmd_xfer_status inside cros_ec_cmd.
>
> Regards,
> Enric
>
> > So, should cros_ec_cmd() keep the *result pointer like so ?:
> >
> > int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
> > void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);
> >
> > This way, we can manually re-populate s_cmd->result with |*result|.
> >
> > Or, should we drop msg->result while returning s_cmd to userspace? I am
> > guessing the answer is no, but thought I'd check with you first.
> >
> > Best regards,
> >
> >
> >> /* Only copy data to userland if data was received. */
> >> if (ret < 0)
> >> goto exit;
> >> --
> >> 2.25.0
> >>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/8] platform/chrome: cros_ec_sysfs: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
` (3 preceding siblings ...)
2020-02-20 15:58 ` [PATCH 4/8] platform/chrome: cros_ec_chardev: " Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 6/8] platform/chrome: cros_ec_lightbar: " Enric Balletbo i Serra
` (3 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Looking at the code I am even unsure that makes sense differentiate
these two errors but let's not change the behaviour for now.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_sysfs.c | 36 ++++++++++++-------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 07dac97ad57c..d45ea5d5bfa4 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -149,14 +149,14 @@ static ssize_t version_show(struct device *dev,
/* Get build info. */
msg->command = EC_CMD_GET_BUILD_INFO + ec->cmd_offset;
msg->insize = EC_HOST_PARAM_SIZE;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- count += scnprintf(buf + count, PAGE_SIZE - count,
- "Build info: XFER ERROR %d\n", ret);
- else if (msg->result != EC_RES_SUCCESS)
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
count += scnprintf(buf + count, PAGE_SIZE - count,
"Build info: EC error %d\n", msg->result);
- else {
+ } else if (ret < 0) {
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Build info: XFER ERROR %d\n", ret);
+ } else {
msg->data[EC_HOST_PARAM_SIZE - 1] = '\0';
count += scnprintf(buf + count, PAGE_SIZE - count,
"Build info: %s\n", msg->data);
@@ -165,14 +165,14 @@ static ssize_t version_show(struct device *dev,
/* Get chip info. */
msg->command = EC_CMD_GET_CHIP_INFO + ec->cmd_offset;
msg->insize = sizeof(*r_chip);
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- count += scnprintf(buf + count, PAGE_SIZE - count,
- "Chip info: XFER ERROR %d\n", ret);
- else if (msg->result != EC_RES_SUCCESS)
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
count += scnprintf(buf + count, PAGE_SIZE - count,
"Chip info: EC error %d\n", msg->result);
- else {
+ } else if (ret < 0) {
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Chip info: XFER ERROR %d\n", ret);
+ } else {
r_chip = (struct ec_response_get_chip_info *)msg->data;
r_chip->vendor[sizeof(r_chip->vendor) - 1] = '\0';
@@ -189,14 +189,14 @@ static ssize_t version_show(struct device *dev,
/* Get board version */
msg->command = EC_CMD_GET_BOARD_VERSION + ec->cmd_offset;
msg->insize = sizeof(*r_board);
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- count += scnprintf(buf + count, PAGE_SIZE - count,
- "Board version: XFER ERROR %d\n", ret);
- else if (msg->result != EC_RES_SUCCESS)
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
count += scnprintf(buf + count, PAGE_SIZE - count,
"Board version: EC error %d\n", msg->result);
- else {
+ } else if (ret < 0) {
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Board version: XFER ERROR %d\n", ret);
+ } else {
r_board = (struct ec_response_board_version *)msg->data;
count += scnprintf(buf + count, PAGE_SIZE - count,
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 6/8] platform/chrome: cros_ec_lightbar: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
` (4 preceding siblings ...)
2020-02-20 15:58 ` [PATCH 5/8] platform/chrome: cros_ec_sysfs: " Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 7/8] platform/chrome: cros_ec: " Enric Balletbo i Serra
` (2 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). It allows us to remove some redundand code. In this
case, though, we are changing a bit the behaviour because of returning
-EINVAL on protocol error we propagate the error return for
cros_ec_cmd_xfer_status() function, but I think it will be fine, even
more clear as we don't mask the Linux error code.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_lightbar.c | 50 ++++++----------------
1 file changed, 13 insertions(+), 37 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index b4c110c5fee0..b59180bff5a3 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -116,7 +116,7 @@ static int get_lightbar_version(struct cros_ec_dev *ec,
param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_VERSION;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0) {
ret = 0;
goto exit;
@@ -193,15 +193,10 @@ static ssize_t brightness_store(struct device *dev,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto exit;
- }
-
ret = count;
exit:
kfree(msg);
@@ -258,13 +253,10 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
goto exit;
}
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS)
- goto exit;
-
i = 0;
ok = 1;
}
@@ -305,14 +297,13 @@ static ssize_t sequence_show(struct device *dev,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- goto exit;
-
- if (msg->result != EC_RES_SUCCESS) {
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
ret = scnprintf(buf, PAGE_SIZE,
"ERROR: EC returned %d\n", msg->result);
goto exit;
+ } else if (ret < 0) {
+ goto exit;
}
resp = (struct ec_response_lightbar *)msg->data;
@@ -344,13 +335,10 @@ static int lb_send_empty_cmd(struct cros_ec_dev *ec, uint8_t cmd)
if (ret)
goto error;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto error;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto error;
- }
+
ret = 0;
error:
kfree(msg);
@@ -377,13 +365,10 @@ static int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
if (ret)
goto error;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto error;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto error;
- }
+
ret = 0;
error:
kfree(msg);
@@ -425,15 +410,10 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto exit;
- }
-
ret = count;
exit:
kfree(msg);
@@ -487,13 +467,9 @@ static ssize_t program_store(struct device *dev, struct device_attribute *attr,
*/
msg->outsize = count + extra_bytes;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto exit;
- }
ret = count;
exit:
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 7/8] platform/chrome: cros_ec: Use cros_ec_cmd_xfer_status helper
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
` (5 preceding siblings ...)
2020-02-20 15:58 ` [PATCH 6/8] platform/chrome: cros_ec_lightbar: " Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-20 15:58 ` [PATCH 8/8] platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer() Enric Balletbo i Serra
2020-02-26 1:12 ` [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Prashant Malani
8 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 6fc8f2c3ac51..e179411bdfbe 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -120,7 +120,7 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
- ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+ ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
/* For now, report failure to transition to S0ix with a warning. */
if (ret >= 0 && ec_dev->host_sleep_v1 &&
--
2.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 8/8] platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer()
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
` (6 preceding siblings ...)
2020-02-20 15:58 ` [PATCH 7/8] platform/chrome: cros_ec: " Enric Balletbo i Serra
@ 2020-02-20 15:58 ` Enric Balletbo i Serra
2020-02-26 1:12 ` [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Prashant Malani
8 siblings, 0 replies; 14+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel
Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani,
Jonathan Cameron, Enrico Granata, Andy Shevchenko, Lee Jones,
Pi-Hsun Shih, Evan Green
Now that all the remaining users of cros_ec_cmd_xfer() has been removed,
make this function private to the cros_ec_proto module.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_proto.c | 5 ++---
include/linux/platform_data/cros_ec_proto.h | 3 ---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3e745e0fe092..11a2db7cd0f7 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -496,8 +496,8 @@ EXPORT_SYMBOL(cros_ec_query_all);
*
* Return: 0 on success or negative error code.
*/
-int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
- struct cros_ec_command *msg)
+static int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+ struct cros_ec_command *msg)
{
int ret;
@@ -541,7 +541,6 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
return ret;
}
-EXPORT_SYMBOL(cros_ec_cmd_xfer);
/**
* cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index ba5914770191..1334fedc07cb 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -212,9 +212,6 @@ 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.25.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status()
2020-02-20 15:58 [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status() Enric Balletbo i Serra
` (7 preceding siblings ...)
2020-02-20 15:58 ` [PATCH 8/8] platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer() Enric Balletbo i Serra
@ 2020-02-26 1:12 ` Prashant Malani
8 siblings, 0 replies; 14+ messages in thread
From: Prashant Malani @ 2020-02-26 1:12 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Collabora Kernel ML, groeck, bleung, dtor, gwendal,
Enrico Granata, Andy Shevchenko, Jonathan Cameron, Ting Shen,
Lee Jones, Neil Armstrong, Dmitry Torokhov, Fei Shao,
Pi-Hsun Shih, Evan Green, linux-input
On Thu, Feb 20, 2020 at 04:58:51PM +0100, Enric Balletbo i Serra wrote:
> Dear all,
>
> The purpose of this series is get rid of the remaining places where the
> cros_ec_cmd_xfer() function is used in favour of the
> cros_ec_cmd_xfer_status() helper. This allows us to make the
> cros_ec_cmd_xfer() function private and only expose to the users a
> single way to send commands to the Embedded Controller.
>
> With these changes we also want to help future improvements in the
> interface, like the Prashant's series (i.e [1]) to introduce a
> cros_ec_cmd() that will allow us to remove more duplicated code in
> different places.
>
> Best regards,
> Enric
>
> Note: Prashant, looks like you should fix your sendmail as the patches
> are not threaded.
>
> [1] https://lkml.org/lkml/2020/2/5/614
>
> Enric Balletbo i Serra (8):
> platform/chrome: cros_ec_proto: Report command not supported
> Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper
> platform/chrome: cros_ec_vbc: Use cros_ec_cmd_xfer_status helper
> platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
> platform/chrome: cros_ec_sysfs: Use cros_ec_cmd_xfer_status helper
> platform/chrome: cros_ec_lightbar: Use cros_ec_cmd_xfer_status helper
> platform/chrome: cros_ec: Use cros_ec_cmd_xfer_status helper
> platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer()
I picked this series on a device running 4.19 and didn't see any
unusual behaviour or dmesg logs, so for the entire series:
Tested-by: Prashant Malani <pmalani@chromium.org>
>
> drivers/input/keyboard/cros_ec_keyb.c | 14 +++---
> drivers/platform/chrome/cros_ec.c | 2 +-
> drivers/platform/chrome/cros_ec_chardev.c | 2 +-
> drivers/platform/chrome/cros_ec_lightbar.c | 50 ++++++---------------
> drivers/platform/chrome/cros_ec_proto.c | 14 ++++--
> drivers/platform/chrome/cros_ec_sysfs.c | 36 +++++++--------
> drivers/platform/chrome/cros_ec_vbc.c | 4 +-
> include/linux/platform_data/cros_ec_proto.h | 3 --
> 8 files changed, 50 insertions(+), 75 deletions(-)
>
> --
> 2.25.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread