linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0
@ 2018-09-27  9:24 Emil Karlson
  2018-09-27  9:30 ` Emil Karlson
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Emil Karlson @ 2018-09-27  9:24 UTC (permalink / raw)
  To: Emil Renner Berthing, Neil Armstrong, Stefan Adolfsson, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0
  2018-09-27  9:24 [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Karlson
@ 2018-09-27  9:30 ` Emil Karlson
  2018-09-28 12:15 ` Neil Armstrong
  2018-10-09 10:13 ` Lee Jones
  2 siblings, 0 replies; 12+ messages in thread
From: Emil Karlson @ 2018-09-27  9:30 UTC (permalink / raw)
  To: Emil Renner Berthing, Neil Armstrong, Stefan Adolfsson, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0
  2018-09-27  9:24 [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Karlson
  2018-09-27  9:30 ` Emil Karlson
@ 2018-09-28 12:15 ` Neil Armstrong
  2018-09-28 17:08   ` [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer Emil Karlson
  2018-09-28 20:27   ` [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Renner Berthing
  2018-10-09 10:13 ` Lee Jones
  2 siblings, 2 replies; 12+ messages in thread
From: Neil Armstrong @ 2018-09-28 12:15 UTC (permalink / raw)
  To: Emil Karlson, Emil Renner Berthing, Stefan Adolfsson,
	linux-kernel, lee.jones, bleung, olof, Enric Balletbo i Serra

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;
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-09-28 12:15 ` Neil Armstrong
@ 2018-09-28 17:08   ` Emil Karlson
  2018-09-28 17:16     ` Emil Karlson
  2018-10-03 11:01     ` Enric Balletbo i Serra
  2018-09-28 20:27   ` [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Renner Berthing
  1 sibling, 2 replies; 12+ messages in thread
From: Emil Karlson @ 2018-09-28 17:08 UTC (permalink / raw)
  To: Emil Renner Berthing, Neil Armstrong, Stefan Adolfsson,
	lee.jones, bleung, olof, Enric Balletbo i Serra, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-09-28 17:08   ` [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer Emil Karlson
@ 2018-09-28 17:16     ` Emil Karlson
  2018-10-03 11:01     ` Enric Balletbo i Serra
  1 sibling, 0 replies; 12+ messages in thread
From: Emil Karlson @ 2018-09-28 17:16 UTC (permalink / raw)
  To: Emil Renner Berthing, Neil Armstrong, Stefan Adolfsson,
	Lee Jones, bleung, olof, Enric Balletbo i Serra, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0
  2018-09-28 12:15 ` Neil Armstrong
  2018-09-28 17:08   ` [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer Emil Karlson
@ 2018-09-28 20:27   ` Emil Renner Berthing
  1 sibling, 0 replies; 12+ messages in thread
From: Emil Renner Berthing @ 2018-09-28 20:27 UTC (permalink / raw)
  To: narmstrong
  Cc: jekarlson, sadolfsson, Linux Kernel Mailing List, lee.jones,
	bleung, olof, Enric Balletbo i Serra

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;
> >
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-09-28 17:08   ` [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer Emil Karlson
  2018-09-28 17:16     ` Emil Karlson
@ 2018-10-03 11:01     ` Enric Balletbo i Serra
  2018-10-03 13:41       ` Neil Armstrong
  2018-10-10  3:44       ` [PATCH] " Benson Leung
  1 sibling, 2 replies; 12+ messages in thread
From: Enric Balletbo i Serra @ 2018-10-03 11:01 UTC (permalink / raw)
  To: Emil Karlson, Emil Renner Berthing, Neil Armstrong,
	Stefan Adolfsson, lee.jones, bleung, olof, linux-kernel

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-10-03 11:01     ` Enric Balletbo i Serra
@ 2018-10-03 13:41       ` Neil Armstrong
  2018-10-03 18:43         ` [PATCH v3] " Emil Karlson
  2018-10-10  3:44       ` [PATCH] " Benson Leung
  1 sibling, 1 reply; 12+ messages in thread
From: Neil Armstrong @ 2018-10-03 13:41 UTC (permalink / raw)
  To: Enric Balletbo i Serra, Emil Karlson, Emil Renner Berthing,
	Stefan Adolfsson, lee.jones, bleung, olof, linux-kernel

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>
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-10-03 13:41       ` Neil Armstrong
@ 2018-10-03 18:43         ` Emil Karlson
  2018-10-10  5:06           ` Benson Leung
  0 siblings, 1 reply; 12+ messages in thread
From: Emil Karlson @ 2018-10-03 18:43 UTC (permalink / raw)
  To: Emil Renner Berthing, Neil Armstrong, Stefan Adolfsson,
	lee.jones, bleung, olof, Enric Balletbo i Serra, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0
  2018-09-27  9:24 [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Karlson
  2018-09-27  9:30 ` Emil Karlson
  2018-09-28 12:15 ` Neil Armstrong
@ 2018-10-09 10:13 ` Lee Jones
  2 siblings, 0 replies; 12+ messages in thread
From: Lee Jones @ 2018-10-09 10:13 UTC (permalink / raw)
  To: Emil Karlson
  Cc: Emil Renner Berthing, Neil Armstrong, Stefan Adolfsson, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-10-03 11:01     ` Enric Balletbo i Serra
  2018-10-03 13:41       ` Neil Armstrong
@ 2018-10-10  3:44       ` Benson Leung
  1 sibling, 0 replies; 12+ messages in thread
From: Benson Leung @ 2018-10-10  3:44 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: jekarlson, kernel, Neil Armstrong, Stefan Adolfsson, Lee Jones,
	Olof Johansson, Linux Kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] mfd: cros-ec: copy the whole event in get_next_event_xfer
  2018-10-03 18:43         ` [PATCH v3] " Emil Karlson
@ 2018-10-10  5:06           ` Benson Leung
  0 siblings, 0 replies; 12+ messages in thread
From: Benson Leung @ 2018-10-10  5:06 UTC (permalink / raw)
  To: Emil Karlson
  Cc: kernel, Neil Armstrong, Stefan Adolfsson, Lee Jones,
	Olof Johansson, Enric Balletbo i Serra, Linux Kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-10-10  5:06 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-27  9:24 [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Karlson
2018-09-27  9:30 ` Emil Karlson
2018-09-28 12:15 ` Neil Armstrong
2018-09-28 17:08   ` [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer Emil Karlson
2018-09-28 17:16     ` Emil Karlson
2018-10-03 11:01     ` Enric Balletbo i Serra
2018-10-03 13:41       ` Neil Armstrong
2018-10-03 18:43         ` [PATCH v3] " Emil Karlson
2018-10-10  5:06           ` Benson Leung
2018-10-10  3:44       ` [PATCH] " Benson Leung
2018-09-28 20:27   ` [PATCH] mfd: cros-ec: copy the whole event when msg->version is 0 Emil Renner Berthing
2018-10-09 10:13 ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).