Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 )
Change subject: drivers/smmstore: Fix some issues ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/#/c/30432/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30432/6//COMMIT_MSG@11 PS6, Line 11: no dependency on size_t I don't understand the point of this line item. API can freely use size_t as it shouldn't matter.
https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c:
https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@90 PS6, Line 90: uint32_t I don't understand the point of this change. There's not reason one can't use ssize_t or size_t.
https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@105 PS6, Line 105: bufsize I don't like the change of the type, but this is a nonsense check now that bufsize is pointing to an unsigned integer.
https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@187 PS6, Line 187: if (scan_end(&end) == CB_ERR) We're doing lookup_store() inside of scan_end(). Why are we duplicating those paths? Can we combine these paths so we're not duplicating work?
Similarly, I would expect lookup_store() to return a writable region_device. The fact that it differs based on IS_ENABLED(CONFIG_SMMSTORE_IN_CBFS) is odd.
https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@201 PS6, Line 201: 1 you open coded 1 here, but didn't below w/ nul. I think we should be consistent.
https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@230 PS6, Line 230: return 0; We're just always successful it seems?