Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 )
Change subject: ft2232_spi.c: Implement spi_send_multicommand() ......................................................................
Patch Set 29:
(8 comments)
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/932f17cb_41ad908b PS25, Line 189: ft2232_spi_send_command
Ack
Maybe we should do this rather early to have more test coverage.
https://review.coreboot.org/c/flashrom/+/40477/comment/762433bb_7066a27a PS25, Line 317: if (!cmds->readcnt && ((cmds + 1)->writecnt || (cmds + 1)->readcnt)){
Checking if next command fits is a very good idea! I did as you proposed. However I´m not sure if the '12' is too far from it´s origin if it´s in an extra function.
Well, you placed it really far up. I guess it could be directly above ft2232_spi_send_multicommand(), no?
More important: I think we need to reset 'i' after send_buf in case the next command does not fit into buffer?
Oh, right. I completely missed that (it is missing). It should be always reset after send_buf(), as that's where we consume all the gathered commands.
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/dae34668_0cdca5a1 PS29, Line 171: return 12 + cmd->writecnt + cmd->readcnt <= buffer_size; I just realized we could make the check more accurate, taking into account if we are going to write and read or just one of them. e.g.
const size_t cmd_len = 3; /* same length for any ft2232 command */ return /* commands for CS# assertion and de-assertion: */ cmd_len + cmd_len /* commands for either a write, a read or both: */ + (cmd->writecnt && cmd->readcnt ? cmd_len + cmd_len : cmd_len) /* payload: */ + cmd->writecnt + cmd->readcnt <= buffer_size;
https://review.coreboot.org/c/flashrom/+/40477/comment/62d2368e_37fe4a36 PS29, Line 301: (cmds->writecnt || cmds->readcnt) Nit, doesn't need parentheses anymore.
https://review.coreboot.org/c/flashrom/+/40477/comment/e6b3d865_2ccb6569 PS29, Line 306: /* Overflow check for buf : * 12 : up to 12 CMD-Bytes are added below : */ I think we can drop the comment here, the function name should be self-explanatory.
https://review.coreboot.org/c/flashrom/+/40477/comment/01d51678_856bbab6 PS29, Line 344: ((cmds + 1)->writecnt || (cmds + 1)->readcnt) && : ft2232_spi_command_fits((cmds + 1), FTDI_HW_BUFFER_SIZE - i)) { Please indent either with 4 spaces or two tabs (relative to the `if`). It shouldn't be indented like the if-body.
https://review.coreboot.org/c/flashrom/+/40477/comment/cb0de205_29120cf9 PS29, Line 352: msg_perr("send_buf failed: %i\n", ret); : break; Needs braces.
https://review.coreboot.org/c/flashrom/+/40477/comment/86bcfb36_d2a5401e PS29, Line 358: msg_perr("get_buf failed: %i\n", ret); : break; Same here.
-
Nico Huber (Code Review)