David Hendricks 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)
Yeah, I was on the fence whether to ask Keno to make the changes. Since my updates to the original patch are straight-forward and don't change the intended behavior I figured it would save everyone time to hijack it rather than go back-and-forth.
I suppose that's another one of the finer points of community engagement we should think about and document as a guideline on the wiki.
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
No, I meant in the kernel. And the command is never written into
Ah, I understand what you are saying now. The only time we can hit this limit is for writes. So the old code is correct and we should not subtract the maximum command size (5) from the read buffer length.
I reverted the change to the read function and updated the comment to clarify the assumptions made here.
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
You're right - Function names in warnings are kind of ugly and probably not needed. I like to use them in debug prints, but they are excessive for a relatively benign and easy to trace warning such as this.
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,
Good point, however I think either if the kernel provides the file (e.g. the above fopen() works) then it will have an integer to read from.
In any case, I went ahead and added an feof() to check for this condition. I was thinking of using stat(), but that would add a few more lines and seemed redundant with fopen() error handling in this case. LMK if you have a preference.
BTW - It may be worth moving all this logic to a file handling library some day, especially as we improve support for Linux interfaces that involve searching thru sysfs and proc and potentially opening/closing a lot of files.
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
Yeah, that's certainly an assumption that I suppose language lawyers may take issue with. But for this use case it seems like a reasonably safe conversion since we're expecting the kernel to give us a small integer.
If we add a generic file library then we definitely should add extra paranoid checking. For example we might check that the length of the string read from the file can't form a number that exceeds the size of the integer type.
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.
Should I follow-up with a patch to use sysconf(_SC_PAGESIZE) instead, as suggested by the man page for getpagesize()?
-
David Hendricks (Code Review)