[coreboot-gerrit] Change in coreboot[master]: WIP / lib: Add blob_provider interface

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/24942 )

Change subject: WIP / lib: Add blob_provider interface
......................................................................


Patch Set 9:

(15 comments)

https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h
File src/include/blob_provider.h:

https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@29
PS3, Line 29: 
> why can't we just use struct blob_locator? What do we gain from typedef'ing aside from not writing ' […]
Done


https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@48
PS3, Line 48: 	ID_DATA_NVRAM_VPD_RO_REGION = 17,
> put DATA_CODE_SPLIT in here?
Done


https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@234
PS3, Line 234: *******
> What's the purpose of the fixed width return type? And what are the expected return value?
Done


https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@238
PS3, Line 238: 
> same question as above
Done


https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@242
PS3, Line 242: 
> cli means what?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c
File src/lib/blob_provider.c:

https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@27
PS3, Line 27: 
> Why are we passing a full object instead of just a const pointer?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@30
PS3, Line 30: 
> this is a weird name -- all caps and also not just cbfs.
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@45
PS3, Line 45: 		    0) {
> Why are we specifying policy on location based on id? […]
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@80
PS3, Line 80: 	}
> Is there a reason to explode out all parameters? Why not jus tpass struct blob_locator?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@87
PS3, Line 87: 
> What's this mean?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@87
PS3, Line 87: 
> this is true only for x86
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@88
PS3, Line 88: 		*size = region_device_sz(&data);
> So we do mmap_full then munmap to make the hook read things?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@94
PS3, Line 94: ev
> What does rd stand for?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@104
PS3, Line 104: 		printk(BIOS_ERR, "Can't use auto mmap function on code blob.");
> ?
Done


https://review.coreboot.org/#/c/24942/3/src/lib/coreboot_table.c
File src/lib/coreboot_table.c:

https://review.coreboot.org/#/c/24942/3/src/lib/coreboot_table.c@511
PS3, Line 511: 		    blob_read_map(BLOB_DATA_NVRAM_CMOS_LAYOUT, NULL);
> this should really be turned into a read.
Done



-- 
To view, visit https://review.coreboot.org/24942
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f161f680cb36fa6481a8bce6cf233263a828bf7
Gerrit-Change-Number: 24942
Gerrit-PatchSet: 9
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Mon, 05 Mar 2018 00:58:49 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180305/6b457408/attachment.html>