Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
When you say "other parts interpret" can you be a bit more concrete. […]
I'm describing a bug in flashrom, the one that this block comment talks about:
/* * Unfortunately there doesn't seem to be a way to specify the maximum number * of bytes that your SPI device can read/write, these values are the maximum * data chunk size that flashrom will package up with an additional five bytes * of command for the flash device, resulting in a 62 byte packet, that we then * add two bytes to in either direction, making our way up to the 64 byte * maximum USB packet size for the device. * * The largest command that flashrom generates is the byte program command, so * we use that command header maximum size here. */
Some of the flashrom drivers interpret the fields 'max_data_read' and 'max_data_write' as the maximum number of bytes we can fit in a payload, not the maximum size of the packet meaning it will generate packets too large.
To avoid this, we are currently storing 2 different constants, one which is keeping track of actual maximum packet size, the other is telling flashrom a false value that is 5 bytes smaller. As this value is now a variable, the behavior had to be duplicated.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
I am not saying to not communicate with the device to interrogate what it supports however the resul […]
Are you suggesting copying the data from 'spi_master_raiden_debug' into a malloc'd buffer. That pointer then gets passed to the configure_protocol() function and the shutdown will be responsible for freeing both buffers. This would prevent us from ever changing spi_master_raiden_debug and allow us to keep it const like it was before the data field was added to it.