[PATCH] mfd: cros-ec: copy the whole event when msg->version is 0

[PATCH] mfd: cros-ec: copy the whole event when msg->version is 0

[Emil Karlson]

Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
be truncated on many chromebooks so that Left and Right keys on Column 12 were
always 0. This commit fixes the issue by restoring the old semantics when the
protocol version is 0.
---
 drivers/platform/chrome/cros_ec_proto.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 398393ab5df8..457e4940dba4 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -519,8 +519,14 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
 
 	ret = cros_ec_cmd_xfer(ec_dev, msg);
 	if (ret > 0) {
+		unsigned int copy_size;
+
 		ec_dev->event_size = ret - 1;
-		memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
+		if (!version)
+			copy_size = sizeof(struct ec_response_get_next_event);
+		else
+			copy_size = ec_dev->event_size;
+		memcpy(&ec_dev->event_data, msg->data, copy_size);
 	}
 
 	return ret;
-- 
2.19.0

Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0

[Emil Karlson]

To note I have almost no idea what I am doing and ended up wondering,
whether the messages are always truncated and it only shows on version
0, because the messages are not padded to longer lenght. Alternatively
the ret could possibly be used as copy length without the -1, right
now I am unable to test with version 1, possibly it requires update of
cros-ec firmware.

Best regards
-Emil
On Thu, Sep 27, 2018 at 12:24 PM Emil Karlson <jekarlson@gmail.com> wrote:
>
> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> be truncated on many chromebooks so that Left and Right keys on Column 12 were
> always 0. This commit fixes the issue by restoring the old semantics when the
> protocol version is 0.
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 398393ab5df8..457e4940dba4 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -519,8 +519,14 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>
>         ret = cros_ec_cmd_xfer(ec_dev, msg);
>         if (ret > 0) {
> +               unsigned int copy_size;
> +
>                 ec_dev->event_size = ret - 1;
> -               memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
> +               if (!version)
> +                       copy_size = sizeof(struct ec_response_get_next_event);
> +               else
> +                       copy_size = ec_dev->event_size;
> +               memcpy(&ec_dev->event_data, msg->data, copy_size);
>         }
>
>         return ret;
> --
> 2.19.0
>

Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0

[Neil Armstrong]

Hi All,

On 27/09/2018 11:24, Emil Karlson wrote:
> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> be truncated on many chromebooks so that Left and Right keys on Column 12 were
> always 0. This commit fixes the issue by restoring the old semantics when the
> protocol version is 0.

OK for me, can someone confirms it fixes the issue ??

Thanks,
Neil

> ---
>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 398393ab5df8..457e4940dba4 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -519,8 +519,14 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>  
>  	ret = cros_ec_cmd_xfer(ec_dev, msg);
>  	if (ret > 0) {
> +		unsigned int copy_size;
> +
>  		ec_dev->event_size = ret - 1;
> -		memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
> +		if (!version)
> +			copy_size = sizeof(struct ec_response_get_next_event);
> +		else
> +			copy_size = ec_dev->event_size;
> +		memcpy(&ec_dev->event_data, msg->data, copy_size);
>  	}
>  
>  	return ret;
> 

[PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Emil Karlson]

Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
be truncated on many chromebooks so that Left and Right keys on Column 12 were
always 0. Use ret as memcpy len to fix this.

drivers/platform/chrome/cros_ec_proto.c:509
get_next_event_xfer uses ret from cros_ec_cmd_xfer for memcpy for msg->data len
drivers/platform/chrome/cros_ec_proto.c:445
cros_ec_cmd_xfer gets ret from send_command
drivers/platform/chrome/cros_ec_proto.c:93
send_command gets ret from bus specific xfer_fn
drivers/mfd/cros_ec_spi.c:598
cros_ec_cmd_xfer_spi copies len amount to ec_msg->data and returns len as ret
drivers/mfd/cros_ec_i2c.c:267
cros_ec_cmd_xfer_i2c copies len amount to ec_msg->data and returns len as ret

so msg->data length is always the same as ret.

Fixes: 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")
Signed-off-by: Emil Karlson <jekarlson@gmail.com>
---
 drivers/platform/chrome/cros_ec_proto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 398393ab5df8..b6fd4838f60f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -520,7 +520,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
 	ret = cros_ec_cmd_xfer(ec_dev, msg);
 	if (ret > 0) {
 		ec_dev->event_size = ret - 1;
-		memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
+		memcpy(&ec_dev->event_data, msg->data, ret);
 	}
 
 	return ret;
-- 
2.19.0

Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Emil Karlson]

I failed in In-Reply-To:
<22cc40a7-015b-6038-2093-8cd1ff0c807e@baylibre.com> somehow.

I was urged to go for correct rather than conservative fix on IRC. I
have tested this on my message
version 0 Kevin and it fixes the original problem for me, but someone
should perhaps test this on
newer system that has message version 1. Background work documented in
commit message.

Best regards
-Emil
On Fri, Sep 28, 2018 at 8:08 PM Emil Karlson <jekarlson@gmail.com> wrote:
>
> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> be truncated on many chromebooks so that Left and Right keys on Column 12 were
> always 0. Use ret as memcpy len to fix this.
>
> drivers/platform/chrome/cros_ec_proto.c:509
> get_next_event_xfer uses ret from cros_ec_cmd_xfer for memcpy for msg->data len
> drivers/platform/chrome/cros_ec_proto.c:445
> cros_ec_cmd_xfer gets ret from send_command
> drivers/platform/chrome/cros_ec_proto.c:93
> send_command gets ret from bus specific xfer_fn
> drivers/mfd/cros_ec_spi.c:598
> cros_ec_cmd_xfer_spi copies len amount to ec_msg->data and returns len as ret
> drivers/mfd/cros_ec_i2c.c:267
> cros_ec_cmd_xfer_i2c copies len amount to ec_msg->data and returns len as ret
>
> so msg->data length is always the same as ret.
>
> Fixes: 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")
> Signed-off-by: Emil Karlson <jekarlson@gmail.com>
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 398393ab5df8..b6fd4838f60f 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -520,7 +520,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>         ret = cros_ec_cmd_xfer(ec_dev, msg);
>         if (ret > 0) {
>                 ec_dev->event_size = ret - 1;
> -               memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
> +               memcpy(&ec_dev->event_data, msg->data, ret);
>         }
>
>         return ret;
> --
> 2.19.0
>

Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Enric Balletbo i Serra]

Hi Emil,

Many thanks to catch this and fix. Some comments below.

You missed to add the v2, please send the next patch with v3 prefix.

On 28/9/18 19:08, Emil Karlson wrote:
> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> be truncated on many chromebooks so that Left and Right keys on Column 12 were
> always 0. Use ret as memcpy len to fix this.
>

That's fine

> drivers/platform/chrome/cros_ec_proto.c:509
> get_next_event_xfer uses ret from cros_ec_cmd_xfer for memcpy for msg->data len
> drivers/platform/chrome/cros_ec_proto.c:445
> cros_ec_cmd_xfer gets ret from send_command
> drivers/platform/chrome/cros_ec_proto.c:93
> send_command gets ret from bus specific xfer_fn
> drivers/mfd/cros_ec_spi.c:598
> cros_ec_cmd_xfer_spi copies len amount to ec_msg->data and returns len as ret
> drivers/mfd/cros_ec_i2c.c:267
> cros_ec_cmd_xfer_i2c copies len amount to ec_msg->data and returns len as ret
> 
> so msg->data length is always the same as ret.
> 

Instead of describe the different calls involved and the returns. I'd explain
why using ret fixes the issue.

> Fixes: 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")
> Signed-off-by: Emil Karlson <jekarlson@gmail.com>
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 398393ab5df8..b6fd4838f60f 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -520,7 +520,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>  	ret = cros_ec_cmd_xfer(ec_dev, msg);
>  	if (ret > 0) {
>  		ec_dev->event_size = ret - 1;
> -		memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
> +		memcpy(&ec_dev->event_data, msg->data, ret);
>  	}
>  
>  	return ret;
> 

After thinking a bit more on this I think that how you fixed this is really
clear. I was wondering if the downstream solution would be better but as is
really late and will be good have this as urgent fix for the coming release I am
happy with it. We can always send follow up patches to sync with the downstream
version if is preferred.

Neil, can you give us your Tested-by to have the make sure this doesn't break
with protocol v1, I don't have such hardware.

Benson, will be really good have this merged in this rc. Are you fine with this
solution?

BTW, you can add my in next version.

Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Neil Armstrong]

Hi Enric,

On 03/10/2018 13:01, Enric Balletbo i Serra wrote:
> Hi Emil,
> 
> Many thanks to catch this and fix. Some comments below.
> 
> You missed to add the v2, please send the next patch with v3 prefix.
> 
> On 28/9/18 19:08, Emil Karlson wrote:
>> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
>> be truncated on many chromebooks so that Left and Right keys on Column 12 were
>> always 0. Use ret as memcpy len to fix this.
>>

[...]

> 
> Neil, can you give us your Tested-by to have the make sure this doesn't break
> with protocol v1, I don't have such hardware.

I'd like to, but I'm unable to get developer mode and legacy boot back on my TEEMO device...

Neil

> 
> Benson, will be really good have this merged in this rc. Are you fine with this
> solution?
> 
> BTW, you can add my in next version.
> 
> Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 

[PATCH v3] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Emil Karlson]

Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
be truncated on many chromebooks so that Left and Right keys on Column 12 were
always 0. Use ret as memcpy len to fix this.

The old code was using ec_dev->event_size, which is the event payload/data size
excluding event_type header, for the length of the memcpy operation. Use ret
as memcpy length to avoid the off by one and copy the whole msg->data.

Fixes: 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")

Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Tested-by: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Emil Karlson <jekarlson@gmail.com>
---

Notes:
    Changes in v3
    - Improve commit message.
    Changes in v2
    - Fix the off by one in memcpy parameter rather than restoring the old behaviour
      for v0 messages.

 drivers/platform/chrome/cros_ec_proto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 398393ab5df8..b6fd4838f60f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -520,7 +520,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
 	ret = cros_ec_cmd_xfer(ec_dev, msg);
 	if (ret > 0) {
 		ec_dev->event_size = ret - 1;
-		memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
+		memcpy(&ec_dev->event_data, msg->data, ret);
 	}
 
 	return ret;
-- 
2.19.0

Re: [PATCH v3] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Benson Leung]

Hi Emil,
On Wed, Oct 3, 2018 at 11:43 AM Emil Karlson <jekarlson@gmail.com> wrote:
>
> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> be truncated on many chromebooks so that Left and Right keys on Column 12 were
> always 0. Use ret as memcpy len to fix this.
>
> The old code was using ec_dev->event_size, which is the event payload/data size
> excluding event_type header, for the length of the memcpy operation. Use ret
> as memcpy length to avoid the off by one and copy the whole msg->data.
>
> Fixes: 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")
>
> Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Tested-by: Emil Renner Berthing <kernel@esmil.dk>
> Signed-off-by: Emil Karlson <jekarlson@gmail.com>

Applied. Sent to Greg for rc8, hopefully.



-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer

[Benson Leung]

Hi Enric,
On Wed, Oct 3, 2018 at 4:01 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Emil,
>
> Many thanks to catch this and fix. Some comments below.
>
> You missed to add the v2, please send the next patch with v3 prefix.
>
> On 28/9/18 19:08, Emil Karlson wrote:
> > Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> > be truncated on many chromebooks so that Left and Right keys on Column 12 were
> > always 0. Use ret as memcpy len to fix this.
> >
>
> That's fine
>
> > drivers/platform/chrome/cros_ec_proto.c:509
> > get_next_event_xfer uses ret from cros_ec_cmd_xfer for memcpy for msg->data len
> > drivers/platform/chrome/cros_ec_proto.c:445
> > cros_ec_cmd_xfer gets ret from send_command
> > drivers/platform/chrome/cros_ec_proto.c:93
> > send_command gets ret from bus specific xfer_fn
> > drivers/mfd/cros_ec_spi.c:598
> > cros_ec_cmd_xfer_spi copies len amount to ec_msg->data and returns len as ret
> > drivers/mfd/cros_ec_i2c.c:267
> > cros_ec_cmd_xfer_i2c copies len amount to ec_msg->data and returns len as ret
> >
> > so msg->data length is always the same as ret.
> >
>
> Instead of describe the different calls involved and the returns. I'd explain
> why using ret fixes the issue.
>
> > Fixes: 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")
> > Signed-off-by: Emil Karlson <jekarlson@gmail.com>
> > ---
> >  drivers/platform/chrome/cros_ec_proto.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 398393ab5df8..b6fd4838f60f 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -520,7 +520,7 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> >       ret = cros_ec_cmd_xfer(ec_dev, msg);
> >       if (ret > 0) {
> >               ec_dev->event_size = ret - 1;
> > -             memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
> > +             memcpy(&ec_dev->event_data, msg->data, ret);
> >       }
> >
> >       return ret;
> >
>
> After thinking a bit more on this I think that how you fixed this is really
> clear. I was wondering if the downstream solution would be better but as is
> really late and will be good have this as urgent fix for the coming release I am
> happy with it. We can always send follow up patches to sync with the downstream
> version if is preferred.
>
> Neil, can you give us your Tested-by to have the make sure this doesn't break
> with protocol v1, I don't have such hardware.
>
> Benson, will be really good have this merged in this rc. Are you fine with this
> solution?

Yes, I'm fine with it. Let me get this ready and try to get it before
we run out of rcs of 4.19.

>
> BTW, you can add my in next version.
>
> Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>



-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0

[Emil Renner Berthing]

On Fri, 28 Sep 2018 at 14:15, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 27/09/2018 11:24, Emil Karlson wrote:
> > Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> > be truncated on many chromebooks so that Left and Right keys on Column 12 were
> > always 0. This commit fixes the issue by restoring the old semantics when the
> > protocol version is 0.
>
> OK for me, can someone confirms it fixes the issue ??

Both this patch and the updated version fixes the same issue for me on
rk3399-gru-kevin.

/Emil

> Thanks,
> Neil
>
> > ---
> >  drivers/platform/chrome/cros_ec_proto.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 398393ab5df8..457e4940dba4 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -519,8 +519,14 @@ static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> >
> >       ret = cros_ec_cmd_xfer(ec_dev, msg);
> >       if (ret > 0) {
> > +             unsigned int copy_size;
> > +
> >               ec_dev->event_size = ret - 1;
> > -             memcpy(&ec_dev->event_data, msg->data, ec_dev->event_size);
> > +             if (!version)
> > +                     copy_size = sizeof(struct ec_response_get_next_event);
> > +             else
> > +                     copy_size = ec_dev->event_size;
> > +             memcpy(&ec_dev->event_data, msg->data, copy_size);
> >       }
> >
> >       return ret;
> >
>

Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0

[Lee Jones]

On Thu, 27 Sep 2018, Emil Karlson wrote:

> Commit 57e94c8b974db2d83c60e1139c89a70806abbea0 caused cros-ec keyboard events
> be truncated on many chromebooks so that Left and Right keys on Column 12 were
> always 0. This commit fixes the issue by restoring the old semantics when the
> protocol version is 0.
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Once you guys have all reviewed, agreed and tested the patches, can
you please send a final version with all of the Acks pre-collected
as a separate (not threaded/attached to this thread) submission
please?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog