<p><a href="https://review.coreboot.org/24942">View Change</a></p><p>15 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h">File src/include/blob_provider.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@29">Patch Set #3, Line 29:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why can't we just use struct blob_locator? What do we gain from typedef'ing aside from not writing ' […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@48">Patch Set #3, Line 48:</a> <code style="font-family:monospace,monospace">     ID_DATA_NVRAM_VPD_RO_REGION = 17,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">put DATA_CODE_SPLIT in here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@234">Patch Set #3, Line 234:</a> <code style="font-family:monospace,monospace">*******</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What's the purpose of the fixed width return type? And what are the expected return value?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@238">Patch Set #3, Line 238:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same question as above</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/include/blob_provider.h@242">Patch Set #3, Line 242:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">cli means what?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c">File src/lib/blob_provider.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@27">Patch Set #3, Line 27:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why are we passing a full object instead of just a const pointer?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@30">Patch Set #3, Line 30:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this is a weird name -- all caps and also not just cbfs.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@45">Patch Set #3, Line 45:</a> <code style="font-family:monospace,monospace">                   0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why are we specifying policy on location based on id? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@80">Patch Set #3, Line 80:</a> <code style="font-family:monospace,monospace">     }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is there a reason to explode out all parameters? Why not jus tpass struct blob_locator?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@87">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What's this mean?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@87">Patch Set #3, Line 87:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this is true only for x86</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@88">Patch Set #3, Line 88:</a> <code style="font-family:monospace,monospace">          *size = region_device_sz(&data);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So we do mmap_full then munmap to make the hook read things?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@94">Patch Set #3, Line 94:</a> <code style="font-family:monospace,monospace">ev</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What does rd stand for?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/blob_provider.c@104">Patch Set #3, Line 104:</a> <code style="font-family:monospace,monospace">             printk(BIOS_ERR, "Can't use auto mmap function on code blob.");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24942/3/src/lib/coreboot_table.c">File src/lib/coreboot_table.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24942/3/src/lib/coreboot_table.c@511">Patch Set #3, Line 511:</a> <code style="font-family:monospace,monospace">                  blob_read_map(BLOB_DATA_NVRAM_CMOS_LAYOUT, NULL);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this should really be turned into a read.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/24942">change 24942</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/24942"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8f161f680cb36fa6481a8bce6cf233263a828bf7 </div>
<div style="display:none"> Gerrit-Change-Number: 24942 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 05 Mar 2018 00:58:49 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>