From 8d30a05ed74730fece3ebf0fd617426c565541ce Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 12 Jul 2022 10:01:23 -0700 Subject: tree: Remove non-standard "%ph" printf format The non-standard "%ph" format is replaced with snprintf_hex_buffer and then using "%s" to print the resulting buffer. Using standard format specifiers makes it easier to switch between the "builtin" EC standard library and the C standard library provided by the toolchain (or Zephyr). BRANCH=none BUG=b:238433667, b:234181908 TEST=Enable CONFIG_CMD_RAND in nocturne_fp/board.h On icetower v0.1 with servo_micro and J-Trace: Before change: > rand rand 8ab8b15090ca5ae83bdad671c906d51a5f2b98a359a4106054ee6b54a4087190 After change: > rand rand 2a8645235a31936a28b8d1b9c4948f46d39662e7fcb10a185ddb14c6a998e2eb Signed-off-by: Tom Hughes Change-Id: I3bff928d32579440d7cdb27a75899e45159accfb Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3759123 Reviewed-by: Denis Brockus --- chip/it83xx/i2c_peripheral.c | 7 ++++++- chip/stm32/trng.c | 6 +++++- common/host_command.c | 34 +++++++++++++++++++++++----------- common/i2c_controller.c | 20 +++++++++++++++----- common/peci.c | 11 +++++++++-- common/printf.c | 21 +-------------------- common/spi_commands.c | 10 ++++++++-- common/usb_host_command.c | 10 ++++++++-- common/vboot_hash.c | 24 +++++++++++++++++++----- driver/nfc/ctn730.c | 19 +++++++++++++++---- driver/touchpad_st.c | 31 ++++++++++++++++++------------- fuzz/host_command_fuzz.c | 8 ++++++-- include/printf.h | 2 -- test/host_command.c | 17 ++++++++++++----- test/printf.c | 9 ++++++--- 15 files changed, 151 insertions(+), 78 deletions(-) diff --git a/chip/it83xx/i2c_peripheral.c b/chip/it83xx/i2c_peripheral.c index c5455327a0..e12173c690 100644 --- a/chip/it83xx/i2c_peripheral.c +++ b/chip/it83xx/i2c_peripheral.c @@ -10,6 +10,7 @@ #include "gpio.h" #include "hooks.h" #include "i2c_peripheral.h" +#include "printf.h" #include "registers.h" #include #include @@ -178,13 +179,17 @@ void i2c_peripheral_read_write_data(int port) /* Peripheral finish */ if (periph_status & IT83XX_I2C_P_CLR) { if (wr_done[idx]) { + char str_buf[hex_str_buf_size( + I2C_MAX_BUFFER_SIZE)]; /* * TODO(b:129360157): Handle controller * write data by "in_data" array. */ - CPRINTS("WData: %ph", + snprintf_hex_buffer( + str_buf, sizeof(str_buf), HEX_BUF(in_data[idx], I2C_MAX_BUFFER_SIZE)); + CPRINTS("WData: %s", str_buf); wr_done[idx] = 0; } } diff --git a/chip/stm32/trng.c b/chip/stm32/trng.c index 6538263add..a975c4c584 100644 --- a/chip/stm32/trng.c +++ b/chip/stm32/trng.c @@ -9,6 +9,7 @@ #include "console.h" #include "host_command.h" #include "panic.h" +#include "printf.h" #include "registers.h" #include "system.h" #include "task.h" @@ -106,12 +107,15 @@ test_mockable void trng_exit(void) static int command_rand(int argc, char **argv) { uint8_t data[32]; + char str_buf[hex_str_buf_size(sizeof(data))]; trng_init(); trng_rand_bytes(data, sizeof(data)); trng_exit(); - ccprintf("rand %ph\n", HEX_BUF(data, sizeof(data))); + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(data, sizeof(data))); + ccprintf("rand %s\n", str_buf); return EC_SUCCESS; } diff --git a/common/host_command.c b/common/host_command.c index 417a2f3465..a2d8defbef 100644 --- a/common/host_command.c +++ b/common/host_command.c @@ -655,10 +655,14 @@ static void host_command_debug_request(struct host_cmd_handler_args *args) hc_prev_cmd = args->command; } - if (hcdebug >= HCDEBUG_PARAMS && args->params_size) - CPRINTS("HC 0x%04x.%d:%ph", args->command, args->version, - HEX_BUF(args->params, args->params_size)); - else + if (hcdebug >= HCDEBUG_PARAMS && args->params_size) { + char str_buf[hex_str_buf_size(args->params_size)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(args->params, args->params_size)); + CPRINTS("HC 0x%04x.%d:%s", args->command, args->version, + str_buf); + } else CPRINTS("HC 0x%04x", args->command); } @@ -714,9 +718,14 @@ uint16_t host_command_process(struct host_cmd_handler_args *args) if (rv != EC_RES_SUCCESS) CPRINTS("HC 0x%04x err %d", args->command, rv); - if (hcdebug >= HCDEBUG_PARAMS && args->response_size) - CPRINTS("HC resp:%ph", - HEX_BUF(args->response, args->response_size)); + if (hcdebug >= HCDEBUG_PARAMS && args->response_size) { + char str_buf[hex_str_buf_size(args->response_size)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(args->response, + args->response_size)); + CPRINTS("HC resp:%s", str_buf); + } return rv; } @@ -878,10 +887,13 @@ static int command_host_command(int argc, char **argv) if (res != EC_RES_SUCCESS) ccprintf("Command returned %d\n", res); - else if (args.response_size) - ccprintf("Response: %ph\n", - HEX_BUF(cmd_params, args.response_size)); - else + else if (args.response_size) { + char str_buf[hex_str_buf_size(args.response_size)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(cmd_params, args.response_size)); + ccprintf("Response: %s\n", str_buf); + } else ccprintf("Command succeeded; no response.\n"); shared_mem_release(cmd_params); diff --git a/common/i2c_controller.c b/common/i2c_controller.c index ea85f80a14..cc7dad9bed 100644 --- a/common/i2c_controller.c +++ b/common/i2c_controller.c @@ -15,6 +15,7 @@ #include "i2c.h" #include "i2c_bitbang.h" #include "i2c_private.h" +#include "printf.h" #include "system.h" #include "task.h" #include "usb_pd.h" @@ -1710,8 +1711,13 @@ static int command_i2cxfer(int argc, char **argv) rv = i2c_xfer(port, addr_flags, (uint8_t *)&offset, 1, data, v); - if (!rv) - ccprintf("Data: %ph\n", HEX_BUF(data, v)); + if (!rv) { + char str_buf[hex_str_buf_size(v)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(data, v)); + ccprintf("Data: %s\n", str_buf); + } } else if (strcasecmp(argv[1], "w") == 0) { /* 8-bit write */ @@ -1780,9 +1786,13 @@ static int command_i2cxfer(int argc, char **argv) read_count, I2C_XFER_START | I2C_XFER_STOP); i2c_lock(port, 0); - if (!rv) - ccprintf("Data: %ph\n", - HEX_BUF(data, read_count)); + if (!rv) { + char str_buf[hex_str_buf_size(read_count)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(data, read_count)); + ccprintf("Data: %s\n", str_buf); + } } #endif /* CONFIG_CMD_I2C_XFER_RAW */ } else { diff --git a/common/peci.c b/common/peci.c index dc17f86a56..5acc49dfd2 100644 --- a/common/peci.c +++ b/common/peci.c @@ -8,6 +8,7 @@ #include "chipset.h" #include "console.h" #include "peci.h" +#include "printf.h" #include "util.h" static int peci_get_cpu_temp(int *cpu_temp) @@ -139,9 +140,15 @@ static int peci_cmd(int argc, char **argv) if (peci_transaction(&peci)) { ccprintf("PECI transaction error\n"); return EC_ERROR_UNKNOWN; + } else { + char str_buf[hex_str_buf_size(peci.r_len)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(r_buf, sizeof(str_buf))); + ccprintf("PECI read data: %s\n", str_buf); + + return EC_SUCCESS; } - ccprintf("PECI read data: %ph\n", HEX_BUF(r_buf, peci.r_len)); - return EC_SUCCESS; } DECLARE_CONSOLE_COMMAND(peci, peci_cmd, "addr wlen rlen cmd timeout(us)", "PECI command"); diff --git a/common/printf.c b/common/printf.c index b3e9d45109..f25a5ac0b2 100644 --- a/common/printf.c +++ b/common/printf.c @@ -393,28 +393,9 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context, ptrspec = *format++; ptrval = va_arg(args, void *); /* - * Avoid null pointer dereference for %ph. * %pP can accept null. */ - if (ptrval == NULL && ptrspec != 'P') - continue; - else if (ptrspec == 'h') { - /* %ph - Print a hex byte buffer. */ - struct hex_buffer_params *hexbuf = - ptrval; - int rc; - - rc = print_hex_buffer(addchar, context, - hexbuf->buffer, - hexbuf->size, 0, - 0); - - if (rc != EC_SUCCESS) - return rc; - - continue; - - } else if (ptrspec == 'P') { + if (ptrspec == 'P') { /* %pP - Print a raw pointer. */ v = (unsigned long)ptrval; base = 16; diff --git a/common/spi_commands.c b/common/spi_commands.c index 45c2f3ce70..b51609d257 100644 --- a/common/spi_commands.c +++ b/common/spi_commands.c @@ -8,6 +8,7 @@ #include "common.h" #include "console.h" +#include "printf.h" #include "spi.h" #include "timer.h" #include "util.h" @@ -45,8 +46,13 @@ static int command_spixfer(int argc, char **argv) rv = spi_transaction(&spi_devices[dev_id], &cmd, 1, data, v); - if (!rv) - ccprintf("Data: %ph\n", HEX_BUF(data, v)); + if (!rv) { + char str_buf[hex_str_buf_size(v)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(data, v)); + ccprintf("Data: %s\n", str_buf); + } } else if (strcasecmp(argv[1], "w") == 0) { /* 8-bit write */ diff --git a/common/usb_host_command.c b/common/usb_host_command.c index e4872750dd..697885a1f7 100644 --- a/common/usb_host_command.c +++ b/common/usb_host_command.c @@ -9,6 +9,7 @@ #include "ec_commands.h" #include "queue_policies.h" #include "host_command.h" +#include "printf.h" #include "system.h" #include "usb_api.h" #include "usb_hw.h" @@ -161,8 +162,13 @@ static void usbhc_written(struct consumer const *consumer, size_t count) block_index = 0; /* Only version 3 is supported. Using in_msg as a courtesy. */ QUEUE_REMOVE_UNITS(consumer->queue, in_msg, count); - if (IS_ENABLED(DEBUG)) - CPRINTS("%ph", HEX_BUF(in_msg, count)); + if (IS_ENABLED(DEBUG)) { + char str_buf[hex_str_buf_size(count)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(in_msg, count)); + CPRINTS("%s", str_buf); + } if (in_msg[0] != EC_HOST_REQUEST_VERSION) { CPRINTS("Unsupported version: %u", in_msg[0]); return; diff --git a/common/vboot_hash.c b/common/vboot_hash.c index fe32f774eb..bb51858374 100644 --- a/common/vboot_hash.c +++ b/common/vboot_hash.c @@ -11,6 +11,7 @@ #include "flash.h" #include "hooks.h" #include "host_command.h" +#include "printf.h" #include "sha256.h" #include "shared_mem.h" #include "stdbool.h" @@ -130,6 +131,8 @@ static void hash_next_chunk(size_t size) static void vboot_hash_all_chunks(void) { + char str_buf[hex_str_buf_size(SHA256_PRINT_SIZE)]; + do { size_t size = MIN(CHUNK_SIZE, data_size - curr_pos); hash_next_chunk(size); @@ -137,7 +140,9 @@ static void vboot_hash_all_chunks(void) } while (curr_pos < data_size); hash = SHA256_final(&ctx); - CPRINTS("hash done %ph", HEX_BUF(hash, SHA256_PRINT_SIZE)); + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(hash, SHA256_PRINT_SIZE)); + CPRINTS("hash done %s", str_buf); in_progress = 0; clock_enable_module(MODULE_FAST_CPU, 0); @@ -165,9 +170,14 @@ static void vboot_hash_next_chunk(void) curr_pos += size; if (curr_pos >= data_size) { + char str_buf[hex_str_buf_size(SHA256_PRINT_SIZE)]; + /* Store the final hash */ hash = SHA256_final(&ctx); - CPRINTS("hash done %ph", HEX_BUF(hash, SHA256_PRINT_SIZE)); + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(hash, SHA256_PRINT_SIZE)); + CPRINTS("hash done %s", str_buf); in_progress = 0; @@ -343,9 +353,13 @@ static int command_hash(int argc, char **argv) ccprintf("(aborting)\n"); else if (in_progress) ccprintf("(in progress)\n"); - else if (hash) - ccprintf("%ph\n", HEX_BUF(hash, SHA256_DIGEST_SIZE)); - else + else if (hash) { + char str_buf[hex_str_buf_size(SHA256_DIGEST_SIZE)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(hash, SHA256_DIGEST_SIZE)); + ccprintf("%s\n", str_buf); + } else ccprintf("(invalid)\n"); return EC_SUCCESS; diff --git a/driver/nfc/ctn730.c b/driver/nfc/ctn730.c index 16ab40d025..ea7eeb29df 100644 --- a/driver/nfc/ctn730.c +++ b/driver/nfc/ctn730.c @@ -10,6 +10,7 @@ #include "gpio.h" #include "i2c.h" #include "peripheral_charger.h" +#include "printf.h" #include "timer.h" #include "util.h" #include "watchdog.h" @@ -256,8 +257,13 @@ static int _process_payload_response(struct pchg *ctx, struct ctn730_msg *res) int rv = _i2c_read(ctx->cfg->i2c_port, buf, len); if (rv) return rv; - if (IS_ENABLED(CTN730_DEBUG)) - CPRINTS("Payload: %ph", HEX_BUF(buf, len)); + if (IS_ENABLED(CTN730_DEBUG)) { + char str_buf[hex_str_buf_size(len)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(buf, len)); + CPRINTS("Payload: %s", str_buf); + } } ctx->event = PCHG_EVENT_NONE; @@ -357,8 +363,13 @@ static int _process_payload_event(struct pchg *ctx, struct ctn730_msg *res) int rv = _i2c_read(ctx->cfg->i2c_port, buf, len); if (rv) return rv; - if (IS_ENABLED(CTN730_DEBUG)) - CPRINTS("Payload: %ph", HEX_BUF(buf, len)); + if (IS_ENABLED(CTN730_DEBUG)) { + char str_buf[hex_str_buf_size(len)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(buf, len)); + CPRINTS("Payload: %s", str_buf); + } } ctx->event = PCHG_EVENT_NONE; diff --git a/driver/touchpad_st.c b/driver/touchpad_st.c index db45b951c6..e5fab5ef7d 100644 --- a/driver/touchpad_st.c +++ b/driver/touchpad_st.c @@ -11,6 +11,7 @@ #include "hwtimer.h" #include "hooks.h" #include "i2c.h" +#include "printf.h" #include "registers.h" #include "spi.h" #include "task.h" @@ -578,7 +579,7 @@ static void dump_error(void) static void dump_memory(void) { uint32_t size = 0x10000, rx_len = 512 + ST_TP_EXTRA_BYTE; - uint32_t offset, i; + uint32_t offset, i, j; uint8_t cmd[] = { 0xFB, 0x00, 0x10, 0x00, 0x00 }; if (!dump_memory_on_error) @@ -591,16 +592,16 @@ static void dump_memory(void) rx_len); for (i = 0; i < rx_len - ST_TP_EXTRA_BYTE; i += 32) { - CPRINTF("%ph %ph %ph %ph " - "%ph %ph %ph %ph\n", - HEX_BUF(rx_buf.bytes + i + 4 * 0, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 1, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 2, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 3, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 4, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 5, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 6, 4), - HEX_BUF(rx_buf.bytes + i + 4 * 7, 4)); + for (j = 0; j < 8; j++) { + char str_buf[hex_str_buf_size(4)]; + + snprintf_hex_buffer( + str_buf, sizeof(str_buf), + HEX_BUF(rx_buf.bytes + i + 4 * j, 4)); + + CPRINTF("%s ", str_buf); + } + CPRINTF("\n"); msleep(8); } } @@ -1235,13 +1236,17 @@ int touchpad_debug(const uint8_t *param, unsigned int param_size, *data_size = 0; st_tp_stop_scan(); return EC_SUCCESS; - case ST_TP_DEBUG_CMD_READ_BUF_HEADER: + case ST_TP_DEBUG_CMD_READ_BUF_HEADER: { + char str_buf[hex_str_buf_size(8)]; *data = buf; *data_size = 8; st_tp_read_host_buffer_header(); memcpy(buf, rx_buf.bytes, *data_size); - CPRINTS("header: %ph", HEX_BUF(buf, *data_size)); + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(buf, *data_size)); + CPRINTS("header: %s", str_buf); return EC_SUCCESS; + } case ST_TP_DEBUG_CMD_READ_EVENTS: num_events = st_tp_read_all_events(0); if (num_events) { diff --git a/fuzz/host_command_fuzz.c b/fuzz/host_command_fuzz.c index 856310db2b..b6c4c4685f 100644 --- a/fuzz/host_command_fuzz.c +++ b/fuzz/host_command_fuzz.c @@ -12,6 +12,7 @@ #include "console.h" #include "host_command.h" #include "host_test.h" +#include "printf.h" #include "task.h" #include "test_util.h" #include "timer.h" @@ -112,8 +113,11 @@ static int hostcmd_fill(const uint8_t *data, size_t size) * issues. */ if (first) { - ccprintf("Request: cmd=%04x data=%ph\n", req->command, - HEX_BUF(req_buf, req_size)); + char str_buf[hex_str_buf_size(req_size)]; + + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(req_buf, req_size)); + ccprintf("Request: cmd=%04x data=%s\n", req->command, str_buf); first = 0; } diff --git a/include/printf.h b/include/printf.h index e5afb7f2d1..48117042f9 100644 --- a/include/printf.h +++ b/include/printf.h @@ -65,8 +65,6 @@ * - 'b' - unsigned integer, print as binary * * Special format codes: - * - '%ph' - binary data, print as hex; Use HEX_BUF(buffer, size) to encode - * parameters. * - '%pP' - raw pointer. */ diff --git a/test/host_command.c b/test/host_command.c index 5c13e2b4cf..17637b52de 100644 --- a/test/host_command.c +++ b/test/host_command.c @@ -8,6 +8,7 @@ #include "common.h" #include "console.h" #include "host_command.h" +#include "printf.h" #include "task.h" #include "test_util.h" #include "timer.h" @@ -177,6 +178,7 @@ static int test_hostcmd_reuse_response_buffer(void) struct ec_host_request *h = (struct ec_host_request *)resp_buf; struct ec_params_hello *d = (struct ec_params_hello *)(resp_buf + sizeof(*h)); + char str_buf[hex_str_buf_size(BUFFER_SIZE)]; h->struct_version = 3; h->checksum = 0; @@ -201,13 +203,15 @@ static int test_hostcmd_reuse_response_buffer(void) h->checksum = calculate_checksum(resp_buf, pkt.request_size); - ccprintf("\nBuffer contents before process 0x%ph\n", - HEX_BUF(resp_buf, BUFFER_SIZE)); + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(resp_buf, BUFFER_SIZE)); + ccprintf("\nBuffer contents before process 0x%s\n", str_buf); host_packet_receive(&pkt); task_wait_event(-1); - ccprintf("\nBuffer contents after process 0x%ph\n", - HEX_BUF(resp_buf, BUFFER_SIZE)); + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(resp_buf, BUFFER_SIZE)); + ccprintf("\nBuffer contents after process 0x%s\n", str_buf); TEST_EQ(calculate_checksum(resp_buf, sizeof(*resp) + resp->data_len), 0, "%d"); @@ -239,6 +243,7 @@ static void hostcmd_fill_chip_info(void) static int test_hostcmd_clears_unused_data(void) { int i, found_null; + char str_buf[hex_str_buf_size(BUFFER_SIZE)]; /* Set the buffer to junk and ensure that is gets cleared */ memset(resp_buf, 0xAA, BUFFER_SIZE); @@ -246,7 +251,9 @@ static int test_hostcmd_clears_unused_data(void) hostcmd_send(); - ccprintf("\nBuffer contents 0x%ph\n", HEX_BUF(resp_buf, BUFFER_SIZE)); + snprintf_hex_buffer(str_buf, sizeof(str_buf), + HEX_BUF(resp_buf, BUFFER_SIZE)); + ccprintf("\nBuffer contents 0x%s\n", str_buf); TEST_EQ(calculate_checksum(resp_buf, sizeof(*resp) + resp->data_len), 0, "%d"); diff --git a/test/printf.c b/test/printf.c index 0ef24d1acd..5b7f3de99e 100644 --- a/test/printf.c +++ b/test/printf.c @@ -416,9 +416,12 @@ test_static int test_vsnprintf_hexdump(void) { const char bytes[] = { 0x00, 0x5E }; - T(expect_success("005e", "%ph", HEX_BUF(bytes, 2))); - T(expect_success("", "%ph", HEX_BUF(bytes, 0))); - T(expect_success("00", "%ph", HEX_BUF(bytes, 1))); + /* + * Test %ph, which used to print buffers as hex, but is non-standard + * and no longer supported. + */ + T(expect(EC_ERROR_INVAL, "", false, sizeof(output), "%ph", + HEX_BUF(bytes, 2))); return EC_SUCCESS; } -- cgit v1.2.1