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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
>
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 <[email protected]>
>> ---
>>
>> 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
>>
Hi Enric,
On Wed, Feb 26, 2020 at 7:00 AM Enric Balletbo i Serra
<[email protected]> 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 <[email protected]>
> >> ---
> >>
> >> 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
> >>