Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33821 )
Change subject: src: Remove variable length arrays ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/33821/8/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33821/8/src/drivers/spi/spi_flash.c@103 PS8, Line 103: #pragma GCC diagnostic ignored "-Wvla"
I really don't want to ignore this, especially considering the urgent TODO. […]
This driver is used in all stages, so no malloc(). In theory this should be really easy to solve because our SPI API is a pure byte pushing API that controls CS independently, so xfer(cmd <concatenated with> data); should just be equivalent to { xfer(cmd); xfer(data); }. In practice, however, there are a lot of weird Intel and AMD controllers that are not really generic SPI controllers but rather specific SPI NOR flash controllers but were still implemented using the generic controller API. They expect that every xfer() they get sent is a whole SPI command (e.g. they inspect dout[0] and expect that it must be a SPI flash opcode).
I think the only clean solution here would be to make all those drivers that aren't actually SPI controllers stop using the generic SPI API, and instead make them implement custom flash read/write ops by overriding .flash_probe() (like some more recent drivers for special controllers such as Intel fast_spi or the MT8173 flash controller already do). Then we rewrite this cleanly and mandate that from then on forward, drivers shouldn't implement the generic SPI API unless they can actually handle it. But there are a lot of old drivers like this (I think basically everything that implements xfer_vector, which we could remove if we fix them all), so this would be a lot of work (although most of these were copied from each other and are very similar... I think for the most part there's just 6 versions of the Intel controller and 4 versions of the AMD controller).
A hackier but less complicated option might be to add a new SPI_CNTRLR flag for all these controllers and then make only them run the dangerous code, while other ones could run a safer version.
+Aaron, what do you think?
-
Julius Werner (Code Review)