Aaron Durbin 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"
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?
I think the answer is the 2nd paragraph. For the fixed function spi flash controllers make sure they are implemented according to their semantics. i.e. struct spi_flash->ops should be filled in with special ops corresponding to the controllers' implementations. e.g. src/soc/intel/common/block/fast_spi/fast_spi_flash.c
I think the list of things to cutover are any users of spi_flash_vector_helper(). The xfer_vector as you noted is a good proxy as well. The cn81xx spi driver is the odd one out:
$ git grep -l xfer_vector src/drivers/spi/spi-generic.c src/drivers/spi/spi_flash.c src/include/spi-generic.h src/soc/amd/stoneyridge/spi.c src/soc/cavium/cn81xx/spi.c src/soc/intel/baytrail/spi.c src/soc/intel/braswell/spi.c src/soc/intel/broadwell/spi.c src/soc/intel/fsp_baytrail/spi.c src/soc/qualcomm/qcs405/spi.c src/southbridge/amd/agesa/hudson/spi.c src/southbridge/amd/cimx/sb800/spi.c src/southbridge/amd/sb700/spi.c src/southbridge/intel/common/spi.c src/southbridge/intel/fsp_rangeley/spi.c
$ git grep -l spi_flash_vector_helper src/drivers/spi/spi_flash.c src/include/spi_flash.h src/soc/amd/stoneyridge/spi.c src/soc/intel/baytrail/spi.c src/soc/intel/braswell/spi.c src/soc/intel/broadwell/spi.c src/soc/intel/fsp_baytrail/spi.c src/soc/qualcomm/qcs405/spi.c src/southbridge/amd/agesa/hudson/spi.c src/southbridge/amd/cimx/sb800/spi.c src/southbridge/amd/sb700/spi.c src/southbridge/intel/common/spi.c src/southbridge/intel/fsp_rangeley/spi.c
I think having cn81xx just implement xfer would be the fix for its xfer_vector usage.
Added a tracking bug here: https://ticket.coreboot.org/issues/217
-
Aaron Durbin (Code Review)