Nico Huber has posted comments on this change. ( https://review.coreboot.org/25706 )
Change subject: linux_mtd: Initial import ......................................................................
Patch Set 4:
(17 comments)
Looks good over all. The only real issues that have to be fixed is a leaked `FILE *` and internal URLs, IIRC. Mostly everything else is in the "we can do better" category ;)
https://review.coreboot.org/#/c/25706/4/Makefile File Makefile:
https://review.coreboot.org/#/c/25706/4/Makefile@346 PS4, Line 346: # Android is handled internally as separate OS, but it supports CONFIG_LINUX_SPI and CONFIG_MSTARDDC_SPI sigh, outdated comment (I hate comments)
https://review.coreboot.org/#/c/25706/4/internal.c File internal.c:
https://review.coreboot.org/#/c/25706/4/internal.c@241 PS4, Line 241: #if CONFIG_LINUX_MTD == 1 A little nicer would be an inline stub in the header file in case of !CONFIG_LINUX_MTD. Then we wouldn't clutter the C file with #if.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c File linux_mtd.c:
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@59 PS4, Line 59: read Using low-level read() might be ok here together with sysfs. But I'm not inclined to check and would prefer stream functions (fread()) instead.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@94 PS4, Line 94: endptr != &buf[strlen(buf)] Nit, same as `*endptr`?
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@131 PS4, Line 131: __builtin_popcount How portable is this?
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@152 PS4, Line 152: msg_pspew("%s: device_name: "%s", is_writeable: %d, " This only runs once on init, right? msg_pdbg() would be more appropriate, IMHO.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@167 PS4, Line 167: flash->chip->block_erasers[0].eraseblocks[0].count = Nit, line break seems unnecessary.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@187 PS4, Line 187: See, for example, : * https://b/35573113 I doubt, my browser would find much there.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@193 PS4, Line 193: if (read(dev_fd, buf + i, step) != step) { Again, fread() is generally preferred. read() doesn't retry if it's interrupted and may return a short byte count even if more data is available.
As we loop here anyway, this could also be solved by not bailing out on a short byte count. More specifically
if ((step = read(...)) <= 0 && errno != EINTR) { ... return 1; }
IIRC (I usually just use fread() if available).
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@224 PS4, Line 224: See, for example, : * https://b/35573113 ...
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@231 PS4, Line 231: write Same as for read().
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@256 PS4, Line 256: use MEMGETREGIONCOUNT/MEMGETREGIONINFO ioctls */ Bail out like above?
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@294 PS4, Line 294: FILE *fp; Never closed? I would move it on top, initialize to `NULL` and below linux_mtd_setup_exit: `if (fp != NULL) fclose(fp);`.
Hmm, actually that might be overkill, we'd only need 2x fclose() after the fread().
Or just use read_sysfs_string()?
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@320 PS4, Line 320: return 1; `goto linux_mtd_setup_exit;`
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@326 PS4, Line 326: (const char *) Nit, why cast?
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@361 PS4, Line 361: (param == endptr) `*endptr` would also cover trailing data.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@364 PS4, Line 364: "device number\n", param); Nits, missing full-stop, output should be broken before 80 chars.
-
Nico Huber (Code Review)