Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Krishna P Bhat D.
9 comments:
Patchset:
Do to urgency of this feature, we would like to update this in a subsequent patch, as it would requi […]
Well, what would you prefer: that you make this change now and test it on your hardware, or that I make this change in your code later blindly because I have no hardware to test it on? Besides, "we need this urgently" should never be a valid reason to reject review feedback on coreboot ToT. If individual companies have schedules to keep, they're free to build their temporary code in local branches while the upstream discussion is still ongoing. There should never be a need to rush unfinished code into the master branch.
Honestly, looking closer at what you're doing here, cbfs_locate_file_in_region() is absolutely not the right approach for that to begin with anyway. You're not trying to access a custom CBFS (like the other case that's looking into the actual CSE FMAP section), you're just reading the default boot CBFS here. You're always supposed to use the main cbfs_load()/cbfs_map() APIs for that, not manually dig through vboot internals and hardcode the matching FMAP sections yourself. Here, I'll point out in other comments how this code should be changed...
File src/soc/intel/common/block/cse/cse_lite.c:
Patch Set #13, Line 406: __weak bool is_cse_sub_part_update_req(void)
This doesn't seem like a good use case for weak functions (see CB:58934 for some ongoing discussion about where weak functions are appropriate and where they aren't). You already have a Kconfig for this feature, so it doesn't make sense to default to not using it on boards that have that Kconfig enabled unless they also implement some extra function. I would either flip this to default to doing the update (i.e. name it `bool skip_cse_sub_part_update()` and the update always happens on platforms with this Kconfig unless they explicitly implement this function and return true from it), or make the function non-weak so that every platform with the Kconfig is forced to implement it.
Patch Set #13, Line 755: static ssize_t rdev_readat_offset(const struct region_device *rd, void *b, size_t offset,
Why are you redefining a well-known existing function with a different name, anyway? All this achieves is confuse people who are used to the existing name. Just use rdev_readat() directly.
Patch Set #13, Line 833: static const char *get_source_rdev_fmap_name(void)
Get rid of this function. Don't reimplement what vboot and the CBFS core are already doing for you.
Patch Set #13, Line 854: static bool cse_sub_part_get_source_rdev(struct region_device *rdev, const char *name)
Get rid of this function. The new CBFS API intentionally abstracts away from raw rdevs. Just map the whole file and use a pointer to that.
Patch Set #13, Line 932: if (!cse_sub_part_get_source_rdev(&source_rdev, name))
If you want to get the `name` CBFS file from the currently active boot CBFS, just use
size_t size;
void *subpart_cbfs_rw = cbfs_map(name, &size);
if (!subpart_cbfs_rw)
return ...;
Patch Set #13, Line 936: if (!cse_get_sub_part_fw_version(type, &source_rdev, &source_fw_ver))
Just pass the `subpart_cbfs_rw` pointer returned from the above in here, and add the appropriate offsets onto it when you dereference.
Patch Set #13, Line 983: region_device_sz(&source_rdev)
Here you would just use the `size` variable that got filled out above by cbfs_map().
Patch Set #13, Line 989: rdev_munmap(&source_rdev, subpart_cbfs_rw);
...and here you would use
cbfs_unmap(subpart_cbfs_rw);
instead.
To view, visit change 59685. To unsubscribe, or for help writing mail filters, visit settings.