Attention is currently required from: Angel Pons, Jacob Garber. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50480 )
Change subject: drivers/spi: Stop using a variable-length array ......................................................................
Patch Set 1:
(2 comments)
File src/drivers/spi/adesto.c:
https://review.coreboot.org/c/coreboot/+/50480/comment/40e4a87e_d13cf89c PS1, Line 94: DEFAULT_PAGE_SIZE_SHIFT Hiding this behind a macro here doesn't feel right to me, even if it's currently the same everywhere. The goal of this API is that it shouldn't need to be the same and there should be nothing special about the 256 byte size (and I don't think there needs to be, see below).
File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/c/coreboot/+/50480/comment/306b1c1f_7b10ceb4 PS1, Line 95: u8 buff[4 + (1 << DEFAULT_PAGE_SIZE_SHIFT)]; Rather than explicitly tying this to any page size, why not just define a constant like MAX_FLASH_CMD_DATA_SIZE (which can be 256 if that makes most sense for us), and then every caller has to make sure it doesn't exceed that number. I think the only relevant case is spi_flash_cmd_write_page_program(), so there you could just add another `chunk_len = MIN(chunk_len, MAX_FLASH_CMD_DATA_SIZE)` to put an upper bound on the amount of data we can write in one command.
-
Julius Werner (Code Review)