Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/50519 )
Change subject: [RFC] drivers/spi: Stop using a variable-length array (alt) ......................................................................
[RFC] drivers/spi: Stop using a variable-length array (alt)
Change-Id: Ic432a75fce24b94fb4f3b74fa52ce925e12219a4 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/winbond.c 4 files changed, 37 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/50519/1
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index b674749..bd70b93 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -88,21 +88,10 @@ return ret; }
-/* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */ -#pragma GCC diagnostic push -#if defined(__GNUC__) && !defined(__clang__) -#pragma GCC diagnostic ignored "-Wstack-usage=" -#endif -#pragma GCC diagnostic ignored "-Wvla" -int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *cmd, - size_t cmd_len, const void *data, size_t data_len) +int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *buff, + size_t cmd_len, size_t data_len) { - int ret; - u8 buff[cmd_len + data_len]; - memcpy(buff, cmd, cmd_len); - memcpy(buff + cmd_len, data, data_len); - - ret = do_spi_flash_cmd(spi, buff, cmd_len + data_len, NULL, 0); + int ret = do_spi_flash_cmd(spi, buff, cmd_len + data_len, NULL, 0); if (ret) { printk(BIOS_WARNING, "SF: Failed to send write command (%zu bytes): %d\n", data_len, ret); @@ -110,7 +99,6 @@
return ret; } -#pragma GCC diagnostic pop
/* Perform the read operation honoring spi controller fifo size, reissuing * the read command until the full request completed. */ @@ -221,7 +209,7 @@ if (ret) goto out;
- ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), NULL, 0); + ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), 0); if (ret) goto out;
@@ -242,6 +230,8 @@ return spi_flash_cmd(&flash->spi, flash->status_cmd, reg, sizeof(*reg)); }
+#define MAX_FLASH_CMD_DATA_SIZE 256 + int spi_flash_cmd_write_page_program(const struct spi_flash *flash, u32 offset, size_t len, const void *buf) { @@ -250,20 +240,22 @@ size_t chunk_len; size_t actual; int ret = 0; - u8 cmd[4]; + u8 cmd_data[4 + MAX_FLASH_CMD_DATA_SIZE];
page_size = flash->page_size; - cmd[0] = flash->pp_cmd; + cmd_data[0] = flash->pp_cmd;
for (actual = 0; actual < len; actual += chunk_len) { byte_addr = offset % page_size; chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); + chunk_len = spi_crop_chunk(&flash->spi, 4, chunk_len); + chunk_len = MIN(chunk_len, MAX_FLASH_CMD_DATA_SIZE);
- spi_flash_addr(offset, cmd); + spi_flash_addr(offset, cmd_data); if (CONFIG(DEBUG_SPI_FLASH)) { printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n", - buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], + buf + actual, + cmd_data[0], cmd_data[1], cmd_data[2], cmd_data[3], chunk_len); }
@@ -273,8 +265,8 @@ goto out; }
- ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); + memcpy(&cmd_data[4], buf + actual, chunk_len); + ret = spi_flash_cmd_write(&flash->spi, cmd_data, 4, chunk_len); if (ret < 0) { printk(BIOS_WARNING, "SF: Page Program failed\n"); goto out; diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index b4d39b3..fdf8d77 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -31,8 +31,8 @@ * Send a multi-byte command to the device followed by (optional) * data. Used for programming the flash array, etc. */ -int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *cmd, - size_t cmd_len, const void *data, size_t data_len); +int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *buff, + size_t cmd_len, size_t data_len);
/* Send a command to the device and wait for some bit to clear itself. */ int spi_flash_cmd_poll_bit(const struct spi_flash *flash, unsigned long timeout, diff --git a/src/drivers/spi/sst.c b/src/drivers/spi/sst.c index 887380f..a514bfa 100644 --- a/src/drivers/spi/sst.c +++ b/src/drivers/spi/sst.c @@ -115,26 +115,27 @@ }
static int -sst_byte_write(const struct spi_flash *flash, u32 offset, const void *buf) +sst_byte_write(const struct spi_flash *flash, u32 offset, const u8 *buf) { int ret; - u8 cmd[4] = { + u8 cmd_data[4 + 1] = { CMD_SST_BP, offset >> 16, offset >> 8, offset, + *buf, };
#if CONFIG(DEBUG_SPI_FLASH) printk(BIOS_SPEW, "BP[%02x]: %p => cmd = { 0x%02x 0x%06x }\n", - spi_w8r8(&flash->spi, CMD_SST_RDSR), buf, cmd[0], offset); + spi_w8r8(&flash->spi, CMD_SST_RDSR), buf, cmd_data[0], offset); #endif
ret = sst_enable_writing(flash); if (ret) return ret;
- ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), buf, 1); + ret = spi_flash_cmd_write(&flash->spi, cmd_data, 4, 1); if (ret) return ret;
@@ -146,7 +147,7 @@ { size_t actual, cmd_len; int ret = 0; - u8 cmd[4]; + u8 cmd_data[4 + 2];
/* If the data is not word aligned, write out leading single byte */ actual = offset % 2; @@ -162,20 +163,21 @@ goto done;
cmd_len = 4; - cmd[0] = CMD_SST_AAI_WP; - cmd[1] = offset >> 16; - cmd[2] = offset >> 8; - cmd[3] = offset; + cmd_data[0] = CMD_SST_AAI_WP; + cmd_data[1] = offset >> 16; + cmd_data[2] = offset >> 8; + cmd_data[3] = offset;
for (; actual < len - 1; actual += 2) { #if CONFIG(DEBUG_SPI_FLASH) printk(BIOS_SPEW, "WP[%02x]: %p => cmd = { 0x%02x 0x%06x }\n", - spi_w8r8(&flash->spi, CMD_SST_RDSR), buf + actual, cmd[0], + spi_w8r8(&flash->spi, CMD_SST_RDSR), buf + actual, cmd_data[0], offset); #endif
- ret = spi_flash_cmd_write(&flash->spi, cmd, cmd_len, - buf + actual, 2); + cmd_data[4] = *(u8 *)(buf + actual + 0); + cmd_data[5] = *(u8 *)(buf + actual + 1); + ret = spi_flash_cmd_write(&flash->spi, cmd_data, cmd_len, 2); if (ret) { printk(BIOS_WARNING, "SF: SST word program failed\n"); break; @@ -209,15 +211,16 @@ static int sst_unlock(const struct spi_flash *flash) { int ret; - u8 cmd, status; + u8 cmd_data[1 + 1] = { + CMD_SST_WRSR, + 0, + };
ret = sst_enable_writing_status(flash); if (ret) return ret;
- cmd = CMD_SST_WRSR; - status = 0; - ret = spi_flash_cmd_write(&flash->spi, &cmd, 1, &status, 1); + ret = spi_flash_cmd_write(&flash->spi, cmd_data, 1, 1); if (ret) printk(BIOS_WARNING, "SF: Unable to set status byte\n");
diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index e4151de..d4b79bc 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -355,8 +355,7 @@ cmdbuf.cmd = CMD_W25_WRSR;
/* Legacy method of writing status register 1 & 2 */ - ret = spi_flash_cmd_write(&flash->spi, (u8 *)&cmdbuf, sizeof(cmdbuf), - NULL, 0); + ret = spi_flash_cmd_write(&flash->spi, (u8 *)&cmdbuf, sizeof(cmdbuf), 0); if (ret) return ret;