Attention is currently required from: Thomas Heijligen, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61275 )
Change subject: hwaccess_x86_io.c: refactor rget_io_perms() ......................................................................
Patch Set 3:
(14 comments)
Patchset:
PS3: Ooof, sorry lots of comments /o\
File hwaccess_x86_io.c:
https://review.coreboot.org/c/flashrom/+/61275/comment/561a1d91_d8c0a823 PS3, Line 22: therefor AFAIK, this term is used rather specific, in legal documents and such. Maybe just start the sentence with "For this purpose, ...".
https://review.coreboot.org/c/flashrom/+/61275/comment/1ab825ad_6bb3101b PS3, Line 23: those No platforms were named to refer to, AFAICT. So I would rather say "on the respective platform".
https://review.coreboot.org/c/flashrom/+/61275/comment/817a801c_8bba8169 PS3, Line 23: static int platform_get_io_perms(void) No need to mention the whole function signature (you too actually didn't on the next line), I would just say platform_get_io_perms() and platform_release_io_perms().
https://review.coreboot.org/c/flashrom/+/61275/comment/3b06eba1_38b9d9a0 PS3, Line 34: please separate multiple platforms with a comma
https://review.coreboot.org/c/flashrom/+/61275/comment/b78cd745_706f27ea PS3, Line 49: return sysi86(SI86V86, V86SC_IOPL, PS_IOPL); This needs <sys/sysi86.h> (our header file already includes it but we shouldn't rely on that).
https://review.coreboot.org/c/flashrom/+/61275/comment/ad20040c_08fca0b7 PS3, Line 65: return (io_fd = open("/dev/io", O_RDWR) >= 0 ? 0 : 1); Please do the assignment on a separate line :)
https://review.coreboot.org/c/flashrom/+/61275/comment/34745837_babe44d5 PS3, Line 65: 1 Other functions, e.g. iopl(), ioperm(), return -1 on error.
https://review.coreboot.org/c/flashrom/+/61275/comment/61bfa8c8_dbfe78cf PS3, Line 92: return iopl(3); <sys/io.h>
https://review.coreboot.org/c/flashrom/+/61275/comment/74923ab8_379b5776 PS3, Line 114: if (ret == 0) Please add braces on all paths if one path needs them. Or `return 0` on the positive path and drop the `else`. Would save indentation below.
https://review.coreboot.org/c/flashrom/+/61275/comment/7bfade17_41c04072 PS3, Line 115: register_shutdown(platform_release_io_perms, NULL); Could be a separate patch as this wasn't handled before:
``` if (register_shutdown(...)) { platform_release_io_perms(...); return 1; } return 0; ```
https://review.coreboot.org/c/flashrom/+/61275/comment/8c0ed24f_76b11e4d PS3, Line 115: NULL In theory, this could be a return parameter of platform_get_io_perms(). Then we wouldn't need the global `io_fd`, for instance.
https://review.coreboot.org/c/flashrom/+/61275/comment/eb05d468_fc081c4d PS3, Line 121: msg_perr("On NetBSD please reboot into single user mode or make sure\n" Can we make the output changes a separate patch?
https://review.coreboot.org/c/flashrom/+/61275/comment/05401917_0dec24f2 PS3, Line 124: return ret; Better use an explicit `return 0`/`return 1` as before. Currently this could also return a -1 from iopl()/ioperm().