Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 )
Change subject: drivers/smmstore: Fix some issues ......................................................................
Patch Set 17:
(18 comments)
https://review.coreboot.org/c/coreboot/+/30432/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/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.
Ack
https://review.coreboot.org/c/coreboot/+/30432/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30432/16//COMMIT_MSG@15 PS16, Line 15: UNTESTED.
Should be tested.
Done
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... 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.
Ack
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... 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 […]
Done
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 201: 1
you open coded 1 here, but didn't below w/ nul. I think we should be consistent.
Done
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 230: return 0;
We're just always successful it seems?
Done
https://review.coreboot.org/c/coreboot/+/30432/8/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/8/src/drivers/smmstore/store.... PS8, Line 94: -1
The interface is defined above and the function calling it also uses plain numbers.
Ack
https://review.coreboot.org/c/coreboot/+/30432/8/src/drivers/smmstore/store.... PS8, Line 107: 0
CB_SUCCESS (other instances as well?)
Ack
https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... PS9, Line 227: if (rdev_writeat(&store, &nul, end, sizeof(nul)) != sizeof(nul)) {
Would it be better to have the equivalent of FILE such that the offset is tracked and bumped when re […]
And an equivalent of fseek etc? That's getting complex fast and I'd prefer to see more places where it might be useful before we go there. But definitely something to keep in mind if this pattern proliferates.
https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... PS12, Line 58: struct region_device *rdev;
This is just a pointer that is uninitialized and isn't pointing to a valid object.
Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 60: rdev.region
*region_device_region(&rdev);
Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 87: store_rdev
This change seems like quite the short cut. store_rdev only is used once below. […]
Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 211: data_sz
I don't understand why one is passing in data_sz scan_end() can obtain it directly since 'store' is […]
Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 219: &store) < 0) {
You already have a writable rdev by way of using incoherent_rdev in lookup_store().
Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 231: end
This should be 0. The rdev_chain() tracks the window of the region it is operating on. […]
Done
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c:
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 136: ssize_t *end
There's no need for this.
Done
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 185:
you can do this here: […]
Done
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 217: end
And this would be 0 once scan_end() has positioned the region device correctly. […]
Done