mfd: cros-ec: copy the whole event when msg->version is 0
Subsystems (2)
PATCH 1/1 mfd: cros-ec: copy the whole event when msg->version is 0
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(-)
drivers/platform/chrome/cros_ec_proto.c +7 -1
@@ -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;
Signature · 1 line
2.19.0
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
Quoted 32 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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(-)drivers/platform/chrome/cros_ec_proto.c +7
-2@@ -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
Hi All,
Quoted 4 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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
Quoted 25 lines
--- drivers/platform/chrome/cros_ec_proto.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/platform/chrome/cros_ec_proto.c +7
-1@@ -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 1/1 mfd: cros-ec: copy the whole event in get_next_event_xfer
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:509get_next_event_xfer uses ret from cros_ec_cmd_xfer for memcpy for msg->data len drivers/platform/chrome/cros_ec_proto.c:445cros_ec_cmd_xfer gets ret from send_command drivers/platform/chrome/cros_ec_proto.c:93send_command gets ret from bus specific xfer_fn drivers/mfd/cros_ec_spi.c:598cros_ec_cmd_xfer_spi copies len amount to ec_msg->data and returns len as ret drivers/mfd/cros_ec_i2c.c:267cros_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.
--- drivers/platform/chrome/cros_ec_proto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/chrome/cros_ec_proto.c +1 -1
@@ -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;
Signature · 1 line
2.19.0
Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
I failed in In-Reply-To:
<[email protected]> 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
Quoted 40 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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:509get_next_event_xferuses ret fromcros_ec_cmd_xferfor memcpy formsg->datalendrivers/platform/chrome/cros_ec_proto.c:445cros_ec_cmd_xfergets ret fromsend_commanddrivers/platform/chrome/cros_ec_proto.c:93send_commandgets ret from bus specificxfer_fndrivers/mfd/cros_ec_spi.c:598cros_ec_cmd_xfer_spicopies len amount toec_msg->dataand returns len as retdrivers/mfd/cros_ec_i2c.c:267cros_ec_cmd_xfer_i2ccopies len amount toec_msg->dataand returns len as retso
msg->datalength is always the same as ret.Fixes 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")Signed-off-by Emil Karlson <[email protected]>--- drivers/platform/chrome/cros_ec_proto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/platform/chrome/cros_ec_proto.c +1
-2@@ -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
In reply to Emil KarlsonHi 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.
Quoted 4 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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
Quoted 11 lines
drivers/platform/chrome/cros_ec_proto.c:509get_next_event_xferuses ret fromcros_ec_cmd_xferfor memcpy formsg->datalendrivers/platform/chrome/cros_ec_proto.c:445cros_ec_cmd_xfergets ret fromsend_commanddrivers/platform/chrome/cros_ec_proto.c:93send_commandgets ret from bus specificxfer_fndrivers/mfd/cros_ec_spi.c:598cros_ec_cmd_xfer_spicopies len amount toec_msg->dataand returns len as retdrivers/mfd/cros_ec_i2c.c:267cros_ec_cmd_xfer_i2ccopies len amount toec_msg->dataand returns len as ret
Quoted 2 lines
so
msg->datalength 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.
Quoted 12 lines
Fixes 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")Signed-off-by Emil Karlson <[email protected]>--- drivers/platform/chrome/cros_ec_proto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/platform/chrome/cros_ec_proto.c +0
-0
drivers/platform/chrome/cros_ec_proto.c static int get_next_event_xfer(struct cros_ec_device *ec_dev, · line 526
Quoted 8 lines
drivers/platform/chrome/cros_ec_proto.c +1
-1@@ -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.
Re: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
Hi Enric,
Quoted 11 lines · 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.
Quoted 4 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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.
[...]
Quoted 3 lines
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
Quoted 8 lines
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.
PATCH v3 1/1 mfd: cros-ec: copy the whole event in get_next_event_xfer
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.
---
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(-)
drivers/platform/chrome/cros_ec_proto.c +1 -1
@@ -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;
Signature · 1 line
2.19.0
Re: [PATCH v3] mfd: cros-ec: copy the whole event in get_next_event_xfer
Hi Emil,
Quoted 9 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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 excludingevent_typeheader, for the length of the memcpy operation. Use ret as memcpy length to avoid the off by one and copy the wholemsg->data.
Quoted 5 lines
Fixes 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")Tested-by Emil Renner Berthing <[email protected]>Signed-off-by Emil Karlson <[email protected]>
Applied. Sent to Greg for rc8, hopefully.
Signature · 7 lines
Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. [email protected] Chromium OS Project [email protected]
Benson Leung
maintainerRe: [PATCH] mfd: cros-ec: copy the whole event in get_next_event_xfer
In reply to Enric Balletbo i SerraHi Enric,
Quoted 59 lines · 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.
Quoted 4 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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
Quoted 11 lines
drivers/platform/chrome/cros_ec_proto.c:509get_next_event_xferuses ret fromcros_ec_cmd_xferfor memcpy formsg->datalendrivers/platform/chrome/cros_ec_proto.c:445cros_ec_cmd_xfergets ret fromsend_commanddrivers/platform/chrome/cros_ec_proto.c:93send_commandgets ret from bus specificxfer_fndrivers/mfd/cros_ec_spi.c:598cros_ec_cmd_xfer_spicopies len amount toec_msg->dataand returns len as retdrivers/mfd/cros_ec_i2c.c:267cros_ec_cmd_xfer_i2ccopies len amount toec_msg->dataand returns len as retQuoted 2 lines
so
msg->datalength 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.
Quoted 12 lines
Fixes 57e94c8b974d ("mfd: cros-ec: Increase maximum mkbp event size")Signed-off-by Emil Karlson <[email protected]>--- drivers/platform/chrome/cros_ec_proto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/platform/chrome/cros_ec_proto.c +0
-0drivers/platform/chrome/cros_ec_proto.c static int get_next_event_xfer(struct cros_ec_device *ec_dev, · line 526
Quoted 8 lines
drivers/platform/chrome/cros_ec_proto.c +1
-1@@ -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.
Quoted 5 lines
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.
Quoted 4 lines
BTW, you can add my in next version.
Signature · 7 lines
Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. [email protected] Chromium OS Project [email protected]
Quoted 7 lines · Neil Armstrong
Quoted 4 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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
Quoted 29 lines
Thanks,
NeilQuoted 25 lines
--- drivers/platform/chrome/cros_ec_proto.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/platform/chrome/cros_ec_proto.c +7
-1@@ -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;
Quoted 7 lines · Emil Karlson
Commit
57e94c8b974db2d83c60e1139c89a70806abbea0caused 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?
Signature · 4 lines
Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog