Hello Tim Wawrzynczak,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37930
to review the following change.
Change subject: ec/google: Fix wedging AP on early ec sw sync ......................................................................
ec/google: Fix wedging AP on early ec sw sync
If the EC doesn't support the EARLY_EC_SYNC we don't properly set power limits to reasonable defaults and can wedge the AP by browning out at the end of vboot.
Change-Id: I4e683e5a1c5b453b3742a12a519cad9069e8b7f7 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/ec.c 1 file changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/37930/1
diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 5dff162..de43eb5 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -757,12 +757,17 @@ .cmd_data_out = &resp, .cmd_dev_index = 0, }; + int rv;
- if (google_chromeec_command(&cmd)) - return -1; + rv = google_chromeec_command(&cmd); + if (rv == -EC_RES_INVALID_PARAM || rv == -EC_RES_INVALID_COMMAND) { + printk(BIOS_INFO, "PARAM_LIMIT_POWER not supported by EC.\n"); + *limit_power = 0; + return 0; + }
*limit_power = resp.get_param.value; - return 0; + return rv; }
int google_chromeec_get_protocol_info(
Hello Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37930
to look at the new patch set (#2).
Change subject: ec/google: Fix wedging AP on early ec sw sync ......................................................................
ec/google: Fix wedging AP on early ec sw sync
If the EC doesn't support the EARLY_EC_SYNC we don't properly set power limits to reasonable defaults and can wedge the AP by browning out at the end of vboot.
BRANCH=none BUG=b:146165519 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I4e683e5a1c5b453b3742a12a519cad9069e8b7f7 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/ec.c 1 file changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/37930/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37930 )
Change subject: ec/google: Fix wedging AP on early ec sw sync ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37930 )
Change subject: ec/google: Fix wedging AP on early ec sw sync ......................................................................
ec/google: Fix wedging AP on early ec sw sync
If the EC doesn't support the EARLY_EC_SYNC we don't properly set power limits to reasonable defaults and can wedge the AP by browning out at the end of vboot.
BRANCH=none BUG=b:146165519 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I4e683e5a1c5b453b3742a12a519cad9069e8b7f7 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37930 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/ec/google/chromeec/ec.c 1 file changed, 8 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 5dff162..de43eb5 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -757,12 +757,17 @@ .cmd_data_out = &resp, .cmd_dev_index = 0, }; + int rv;
- if (google_chromeec_command(&cmd)) - return -1; + rv = google_chromeec_command(&cmd); + if (rv == -EC_RES_INVALID_PARAM || rv == -EC_RES_INVALID_COMMAND) { + printk(BIOS_INFO, "PARAM_LIMIT_POWER not supported by EC.\n"); + *limit_power = 0; + return 0; + }
*limit_power = resp.get_param.value; - return 0; + return rv; }
int google_chromeec_get_protocol_info(
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37930 )
Change subject: ec/google: Fix wedging AP on early ec sw sync ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37930/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/37930/3/src/ec/google/chromeec/ec.c... PS3, Line 768: With this change, if rv is non-zero but other than -EC_RES_INVALID_PARAM and -EC_RES_INVALID_COMMAND, then limit_power would be set to resp.get_param.value which might not really be valid. Probably, it would get ignored by the caller, but shouldn't there be an early return here?
if (rv == -EC_RES_INVALID_PARAM || rv == -EC_RES_INVALID_COMMAND) { ... } else if (rv != 0) { return -1; }
*limit_power = resp.get_param.value; return 0;
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37930 )
Change subject: ec/google: Fix wedging AP on early ec sw sync ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
seems to be addressed in a follow up patch by Intel https://review.coreboot.org/c/coreboot/+/37947
-
Edward O'Callaghan (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)