Nico Huber has posted comments on this change. ( https://review.coreboot.org/22344 )
Change subject: linux_spi: Dynamically detect max buffer size ......................................................................
Patch Set 5:
(5 comments)
I ended up re-writing a significant portion of this patch. Assuming the changes look good, what's the "right" thing to do to get things merged? Merge everything in this patch, or split my updates into a follow-up patch?
I think the changes should be done right away in this patch. In this case it wouldn't break anything in between two patches, but merging things that you know need an update feels generally wrong (bisect argument). The actual question we should ask is, who should do the changes? In this case, I would have asked Keno in advance if he wants to amend it (don't know if you did).
Back to the patch: You can ignore all the new comments and nits, looks quite good actually :) But... I really care about the change to the spi_read_chunked() call.
https://review.coreboot.org/#/c/22344/3/linux_spi.c File linux_spi.c:
https://review.coreboot.org/#/c/22344/3/linux_spi.c@199 PS3, Line 199: The implementation currently does not support requests that
Re "why": As to not rely on the driver to split large transactions? I'm not
No, I meant in the kernel. And the command is never written into the rx buffer anyway. Actually it seems that there is never a buffer that holds both tx and rx data throughout the whole stack starting with flashrom.
https://review.coreboot.org/#/c/22344/5/linux_spi.c File linux_spi.c:
https://review.coreboot.org/#/c/22344/5/linux_spi.c@136 PS5, Line 136: msg_pwarn("%s: Cannot open %s: %s.\n", __func__, BUF_SIZE_FROM_SYSFS, strerror(errno)); I was about to say, "function names in warnings?". But hey, we do it all over the place...
https://review.coreboot.org/#/c/22344/5/linux_spi.c@143 PS5, Line 143: msg_pwarn("%s: Cannot read %s: %s.\n", __func__, BUF_SIZE_FROM_SYSFS, strerror(errno)); Nit, in the unlikely event that we are presented an empty file, we'd print:
Cannot read /sys/module/spidev/parameters/bufsiz: Success.
i.e. EOF is not an error :-/
https://review.coreboot.org/#/c/22344/5/linux_spi.c@154 PS5, Line 154: max_kernel_buf_size = (size_t)tmp; Nit, possible overflow. Very theoretic, don't know any environment where sizeof(size_t) < sizeof(long).
https://review.coreboot.org/#/c/22344/5/linux_spi.c@163 PS5, Line 163: max_kernel_buf_size = (size_t)getpagesize(); getpagesize() is kind of deprecated, but we've used it before anyway.
-
Nico Huber (Code Review)