Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Angel Pons, 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 35: Code-Review+1
(18 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/40477/comment/15c34477_066e1b05 PS8, Line 17: https://ibb.co/0c1J25d
I wouldn't provide them in a commit message
Done
Commit Message:
https://review.coreboot.org/c/flashrom/+/40477/comment/ba701a0c_52db1638 PS16, Line 25: in a similar way.
The infrastructure is already in place. You just have to implement […]
Done
Patchset:
PS32:
OK, I understand (I have/had got no idea of VM stuff). […]
Done
Patchset:
PS34:
Thanks for paying attention again. […]
I'll take over then. It's only a few nits left after all.
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/95c0abfd_b5e828e6 PS8, Line 542: /* Return to get second op (Program or Erase) without resetting buf nor i*/
s Line 473
Ack
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/5e48bb31_a0be59e3 PS16, Line 172: .multicommand = default_spi_send_multicommand,
I got it. […]
Done
https://review.coreboot.org/c/flashrom/+/40477/comment/cf1c0926_7b36aeaa PS16, Line 497: bufsize = max(writecnt + 19, 261 + 19);
What do you want to drop? Just the change from 260+9 to 261+19 or the whole max()-line? […]
Done
https://review.coreboot.org/c/flashrom/+/40477/comment/9c523e9b_0f36e9ad PS16, Line 562: if (writearr[0] == JEDEC_WREN) {
You are right, I was assuming JEDEC_WREN in any case. […]
Done
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/de4141b9_85c494c0 PS25, Line 189: ft2232_spi_send_command
Maybe we should do this rather early to have more test coverage.
Done in a subsequent commit.
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/296c1685_2e073c26 PS33, Line 290: + cmd->readcnt
One tiny mistake left. I also didn't realize this before: The buffer `buf` […]
Done
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/f506e043_5965a581 PS34, Line 82:
I guess this space is not intentional. In any case, it should align […]
Done
https://review.coreboot.org/c/flashrom/+/40477/comment/cfb9f5d4_bfaa60cd PS34, Line 283: const size_t cmd_len = 3; /* same length for any ft2232 command */
I don't think it's necessary, as the literal value is never used outside of this function. […]
I agree with Angel, it should be kept as close as possible to the code it's used in.
https://review.coreboot.org/c/flashrom/+/40477/comment/fd7114d0_21441666 PS34, Line 290: + cmd->writecnt + cmd->readcnt The buffer in question does not include the `readcnt` part.
https://review.coreboot.org/c/flashrom/+/40477/comment/c9932d66_132940b6 PS34, Line 300: int i = 0, ret = 0;
Shouldn't `i` be `unsigned int` or `size_t`? […]
Ack
It's used as index for a byte array and as parameter to ft2232_spi_command_fits(), so `size_t`.
https://review.coreboot.org/c/flashrom/+/40477/comment/2b8381fb_db0680a9 PS34, Line 307: 65536
hmmm, max_data_write is 256
That is right, and assuming an overall sound flashrom, we shouldn't hit this case. But it seems a good idea to check this "external" data locally again. Or do you mean we could make the check tighter? I guess we could do so. Probably in a follow up as this just mimicks the original code.
https://review.coreboot.org/c/flashrom/+/40477/comment/7934221c_abec5c51 PS34, Line 311: Out of memory!
`Out of memory!` is usually printed when a malloc() or similar function fails. […]
Ack
"Command does not fit" to align with other error messages that don't have a "FTDI" prefix.
https://review.coreboot.org/c/flashrom/+/40477/comment/2da3ab68_bd3f8ff8 PS34, Line 317:
nit: no space between unary operators: […]
Generally right, but it's mimicking the original code and that will be reverted later. I hope it's ok to leave it for the moment.
https://review.coreboot.org/c/flashrom/+/40477/comment/38a37f06_2b4bf34e PS34, Line 349: /* send_buf */
I'd say this comment is redundant, and would remove it
Ack