Change in ...coreboot[master]: google_chromeec_command: makes the error status consistent
YH Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31626
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
google_chromeec_command: makes the error status consistent
Currently various google_chromeec_command() implementation the error can be returned differently (LPC: -1 for v3, 1 for v1, SPI: -1/1, I2C: 1). This patch attempts to make it consistent across all implementations by using only negative number as error.
All direct/indirect callers of google_chromeec_command() have also been modified accordingly as well.
BRANCH=None BUG=chromium:935038
Signed-off-by: YH Lin yueherngl@google.com Change-Id: I39f12502bc6982670dd200dcef0724e7eed2ed29 --- M src/ec/google/chromeec/crosec_proto.c M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_i2c.c M src/ec/google/chromeec/ec_lpc.c M src/ec/google/chromeec/smihandler.c M src/ec/google/chromeec/vboot_storage.c M src/ec/google/chromeec/vstore.c M src/mainboard/google/gru/mainboard.c M src/security/vboot/vboot_logic.c 10 files changed, 35 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/31626/1
diff --git a/src/ec/google/chromeec/crosec_proto.c b/src/ec/google/chromeec/crosec_proto.c index 2043347..603f1df 100644 --- a/src/ec/google/chromeec/crosec_proto.c +++ b/src/ec/google/chromeec/crosec_proto.c @@ -246,7 +246,7 @@ int rv = send_command_proto3(cec_command, crosec_io, context); if (rv < 0) { cec_command->cmd_code = rv; - return 1; + return -1; } return 0; } diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 09357b0..897dceb 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -363,7 +363,7 @@ cmd.cmd_size_out = sizeof(rsp); cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd) == 0) + if (google_chromeec_command(&cmd) >= 0) return rsp.event_mask; return 0; } @@ -404,7 +404,7 @@ cmd.cmd_size_out = sizeof(rsp); cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd) == 0) + if (google_chromeec_command(&cmd) >= 0) return rsp.event_mask; return 0; } @@ -507,7 +507,7 @@ cmd.cmd_size_out = sizeof(r); cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd) != 0) + if (google_chromeec_command(&cmd) < 0) return -1;
if (feature >= 8 * sizeof(r.flags)) @@ -531,7 +531,7 @@ cmd.cmd_size_out = 0; cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd) != 0) + if (google_chromeec_command(&cmd) < 0) return -1;
return 0; @@ -550,7 +550,7 @@ cmd.cmd_size_out = sizeof(r); cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd) != 0) + if (google_chromeec_command(&cmd) < 0) return -1;
return rtc_to_tm(r.time, time); @@ -658,7 +658,7 @@ cmd.cmd_data_out = &board_v; cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd)) + if (google_chromeec_command(&cmd) < 0) return -1;
*version = board_v.board_version; @@ -677,7 +677,7 @@ cmd.cmd_data_out = &sku_v; cmd.cmd_dev_index = 0;
- if (google_chromeec_command(&cmd) != 0) + if (google_chromeec_command(&cmd) < 0) return 0;
return sku_v.sku_id; @@ -708,7 +708,7 @@ if (!is_read) memcpy(&cmd_vbnvcontext.block, data, EC_VBNV_BLOCK_SIZE);
- if (google_chromeec_command(&cec_cmd)) { + if (google_chromeec_command(&cec_cmd) < 0) { printk(BIOS_ERR, "ERROR: failed to %s vbnv_ec context: %d\n", is_read ? "read" : "write", (int)cec_cmd.cmd_code); mdelay(10); /* just in case */ @@ -792,7 +792,7 @@ cmd.cmd_size_out = sizeof(*r) + read_len; cmd.cmd_dev_index = 0; rv = google_chromeec_command(&cmd); - if (rv != 0) + if (rv < 0) return rv;
/* Parse response */ @@ -875,7 +875,7 @@ }; struct usb_chg_measures m; int rv = google_chromeec_command(&cmd); - if (rv != 0) + if (rv < 0) return rv; /* values are given in milliAmps and milliVolts */ *type = rsp.type; diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index 7a38336..51124f6 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -108,7 +108,7 @@ * * @param type charger type * @param max_watts charger max wattage - * @return non-zero for error, otherwise 0. + * @return 0 on success or negative integer for errors. */ int google_chromeec_get_usb_pd_power_info(enum usb_chg_type *type, u32 *max_watts); @@ -118,7 +118,7 @@ * * @param current_lim Max current in mA * @param voltage_lim Max voltage in mV - * @return non-zero for error, otherwise 0. + * @return 0 on success or negative integer for errors. */ int google_chromeec_override_dedicated_charger_limit(u16 current_lim, u16 voltage_lim); @@ -150,6 +150,8 @@ * The lower level transport works on the buffers handed out to the * upper level. Therefore, only the size of the request and response * are required. + * + * @return 0 on success or negative integer for errors. */ typedef int (*crosec_io_t)(size_t req_size, size_t resp_size, void *context); int crosec_command_proto(struct chromeec_command *cec_command, @@ -159,7 +161,7 @@ * Send a command to a CrOS EC * * @param cec_command: CrOS EC command to send - * @return 0 for success. Non-zero for error. + * @return 0 on success or negative integer for errors. */ int google_chromeec_command(struct chromeec_command *cec_command);
diff --git a/src/ec/google/chromeec/ec_i2c.c b/src/ec/google/chromeec/ec_i2c.c index 22b491a..7f2ead5 100644 --- a/src/ec/google/chromeec/ec_i2c.c +++ b/src/ec/google/chromeec/ec_i2c.c @@ -205,7 +205,7 @@ __func__, cec_command->cmd_size_in, cec_command->cmd_size_out); cec_command->cmd_code = EC_RES_INVALID_PARAM; - return 1; + return -1; }
/* Construct command. */ @@ -221,13 +221,13 @@ printk(BIOS_ERR, "%s: Cannot complete write to i2c-%d:%#x\n", __func__, bus, chip); cec_command->cmd_code = EC_RES_ERROR; - return 1; + return -1; } if (i2c_read_raw(bus, chip, (uint8_t *)&resp, size_i2c_resp) != 0) { printk(BIOS_ERR, "%s: Cannot complete read from i2c-%d:%#x\n", __func__, bus, chip); cec_command->cmd_code = EC_RES_ERROR; - return 1; + return -1; }
/* Verify and return response */ @@ -235,17 +235,17 @@ if (resp.response != EC_RES_SUCCESS) { printk(BIOS_DEBUG, "%s: Received bad result code %d\n", __func__, (int)resp.response); - return 1; + return -1; } if (resp.length > cec_command->cmd_size_out) { printk(BIOS_ERR, "%s: Received len %#02x too large\n", __func__, (int)resp.length); cec_command->cmd_code = EC_RES_INVALID_RESPONSE; - return 1; + return -1; } if (!ec_verify_checksum(&resp)) { cec_command->cmd_code = EC_RES_INVALID_CHECKSUM; - return 1; + return -1; } cec_command->cmd_size_out = resp.length; memcpy(cec_command->cmd_data_out, resp.data, resp.length); diff --git a/src/ec/google/chromeec/ec_lpc.c b/src/ec/google/chromeec/ec_lpc.c index 61005d3..097b1b4 100644 --- a/src/ec/google/chromeec/ec_lpc.c +++ b/src/ec/google/chromeec/ec_lpc.c @@ -330,13 +330,13 @@ if (google_chromeec_wait_ready(EC_LPC_ADDR_HOST_CMD)) { printk(BIOS_ERR, "Timeout waiting for EC process command %d!\n", cec_command->cmd_code); - return 1; + return -1; }
/* Check result */ cec_command->cmd_code = read_byte(EC_LPC_ADDR_HOST_DATA); if (cec_command->cmd_code) - return 1; + return -1;
/* Read back args */ read_bytes(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8*)&args, NULL); @@ -348,12 +348,12 @@ */ if (!(args.flags & EC_HOST_ARGS_FLAG_TO_HOST)) { printk(BIOS_ERR, "EC protocol mismatch\n"); - return 1; + return -1; }
if (args.data_size > cec_command->cmd_size_out) { printk(BIOS_ERR, "EC returned too much data\n"); - return 1; + return -1; } cec_command->cmd_size_out = args.data_size;
@@ -369,7 +369,7 @@ /* Verify checksum */ if (args.checksum != csum) { printk(BIOS_ERR, "EC response has invalid checksum\n"); - return 1; + return -1; }
return 0; diff --git a/src/ec/google/chromeec/smihandler.c b/src/ec/google/chromeec/smihandler.c index cab7192..3f703f9 100644 --- a/src/ec/google/chromeec/smihandler.c +++ b/src/ec/google/chromeec/smihandler.c @@ -54,7 +54,7 @@ ;
printk(BIOS_DEBUG,"Clearing pending EC events. Error code 1 is expected.\n"); - while (google_chromeec_get_mkbp_event(&mkbp_event) == 0) + while (google_chromeec_get_mkbp_event(&mkbp_event) >= 0) ; }
diff --git a/src/ec/google/chromeec/vboot_storage.c b/src/ec/google/chromeec/vboot_storage.c index f47c2f1..08af2f6 100644 --- a/src/ec/google/chromeec/vboot_storage.c +++ b/src/ec/google/chromeec/vboot_storage.c @@ -30,7 +30,7 @@ /* Ensure the digests being saved match the EC's slot size. */ assert(digest_size == EC_VSTORE_SLOT_SIZE);
- if (google_chromeec_vstore_write(slot, digest, digest_size)) + if (google_chromeec_vstore_write(slot, digest, digest_size) < 0) return -1;
/* Assert the slot is locked on successful write. */ diff --git a/src/ec/google/chromeec/vstore.c b/src/ec/google/chromeec/vstore.c index 28c2603..39e59f8 100644 --- a/src/ec/google/chromeec/vstore.c +++ b/src/ec/google/chromeec/vstore.c @@ -42,7 +42,7 @@ .cmd_data_out = &info, };
- if (google_chromeec_command(&cmd) != 0) + if (google_chromeec_command(&cmd) < 0) return 0;
if (locked) diff --git a/src/mainboard/google/gru/mainboard.c b/src/mainboard/google/gru/mainboard.c index 41307c7..245a70e 100644 --- a/src/mainboard/google/gru/mainboard.c +++ b/src/mainboard/google/gru/mainboard.c @@ -257,12 +257,14 @@
static void usb_power_cycle(int port) { - if (google_chromeec_set_usb_pd_role(port, USB_PD_CTRL_ROLE_FORCE_SINK)) + if (google_chromeec_set_usb_pd_role(port, USB_PD_CTRL_ROLE_FORCE_SINK) + < 0) printk(BIOS_ERR, "ERROR: Cannot force USB%d PD sink\n", port);
mdelay(10); /* Make sure USB stick is fully depowered. */
- if (google_chromeec_set_usb_pd_role(port, USB_PD_CTRL_ROLE_TOGGLE_ON)) + if (google_chromeec_set_usb_pd_role(port, USB_PD_CTRL_ROLE_TOGGLE_ON) + < 0) printk(BIOS_ERR, "ERROR: Cannot restore USB%d PD mode\n", port); }
diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 00bbae6..c379463 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -141,7 +141,7 @@
printk(BIOS_DEBUG, "Platform is resuming.\n");
- if (vboot_retrieve_hash(saved_hash, saved_hash_sz)) { + if (vboot_retrieve_hash(saved_hash, saved_hash_sz) < 0) { printk(BIOS_ERR, "Couldn't retrieve saved hash.\n"); return -1; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31626 )
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
Patch Set 1:
I'm okay with changing all the return codes to -1, but I'm not so sure about changing all the checks to only check for <0. As long as we don't have any valid responses >0, why not keep the old checks just to be safe? It makes for shorter code, too.
(Note that there is plenty of other precedent for using 1 instead of -1 as error returns in coreboot, e.g. plenty of platform_i2c_transfer() implementations. I'm worried that one of those might still have a chance to slip through into the EC code in some odd edge case we're overlooking.)
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31626 )
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h@164 PS1, Line 164: @return 0 on success or negative integer for errors. Currently, != 0 works and it's consistent. We can make this change when we actually do need to use positive return values.
YH Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31626 )
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
Patch Set 1:
(1 comment)
Thanks for the comments. Keeping the check as they are while returning "-1" seems to be acceptable by all it seems?
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h@164 PS1, Line 164: @return 0 on success or negative integer for errors.
Currently, != 0 works and it's consistent. […]
Yes, the caller can definitely use "!= 0" which still conforms to this comment. But the comment itself needs to be accurate or else it would be misleading?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31626 )
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h@164 PS1, Line 164: @return 0 on success or negative integer for errors.
Yes, the caller can definitely use "!= 0" which still conforms to this comment. […]
Yes, updating the comment seems fine.
Hello Aaron Durbin, Daisuke Nojiri, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31626
to look at the new patch set (#2).
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
google_chromeec_command: makes the error status consistent
Currently within various google_chromeec_command() implementation the error can be returned differently (LPC: -1 for v3, 1 for v1, SPI: -1/1, I2C: 1). This patch attempts to make it consistent across all implementations by using only negative number as error.
All direct/indirect callers of google_chromeec_command() have already checked for non-zero value so no modification is needed.
BRANCH=None BUG=chromium:935038
Signed-off-by: YH Lin yueherngl@google.com Change-Id: I39f12502bc6982670dd200dcef0724e7eed2ed29 --- M src/ec/google/chromeec/crosec_proto.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_i2c.c M src/ec/google/chromeec/ec_lpc.c 4 files changed, 17 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/31626/2
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31626 )
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/#/c/31626/1/src/ec/google/chromeec/ec.h@164 PS1, Line 164: @return 0 on success or negative integer for errors.
Yes, updating the comment seems fine.
Sorry it was ambiguous. I was referring to this entire patch. Do we have a plan to use positive integers for something else?
As long as we know we won't break any board, it's ok to do this change now (sparing positive integers for future use) but currently the risk isn't worth.
YH Lin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31626 )
Change subject: google_chromeec_command: makes the error status consistent ......................................................................
Abandoned
Not needed anymore.
-
Daisuke Nojiri (Code Review) -
Julius Werner (Code Review)
-
YH Lin (Code Review)