Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
drivers/pc80/rtc: Cache cmos_layout.bin region device
This avoids needing to search for the cmos_layout.bin cbfs file each time cmos_get or cmos_set is called.
Change-Id: Ic7679269f28d090e9a99625d68b37e0bf02b061a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/pc80/rtc/mc146818rtc.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/35827/1
diff --git a/src/drivers/pc80/rtc/mc146818rtc.c b/src/drivers/pc80/rtc/mc146818rtc.c index 9ea5414..ecb02e8 100644 --- a/src/drivers/pc80/rtc/mc146818rtc.c +++ b/src/drivers/pc80/rtc/mc146818rtc.c @@ -17,6 +17,7 @@
#include <arch/acpi.h> #include <arch/io.h> +#include <arch/early_variables.h> #include <bcd.h> #include <fallback.h> #include <version.h> @@ -242,10 +243,19 @@ return CB_SUCCESS; }
+static struct region_device layout_rdev CAR_GLOBAL; +static struct region_device *layout_rdev_p CAR_GLOBAL; + static enum cb_err locate_cmos_layout(struct region_device *rdev) { uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT; struct cbfsf fh; + struct region_device *cmos_layout_rdev = car_get_ptr(layout_rdev_p); + + if (cmos_layout_rdev) { + rdev = cmos_layout_rdev; + return CB_SUCCESS; + }
/* * In case VBOOT is enabled and this function is called from SMM, @@ -261,7 +271,10 @@ return CB_CMOS_LAYOUT_NOT_FOUND; }
- cbfs_file_data(rdev, &fh); + cbfs_file_data(cmos_layout_rdev, &fh); + + car_set_var(layout_rdev, *cmos_layout_rdev); + car_set_ptr(layout_rdev_p, car_get_ptr(&layout_rdev));
return CB_SUCCESS; }
Hello Kyösti Mälkki, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35827
to look at the new patch set (#2).
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
drivers/pc80/rtc: Cache cmos_layout.bin region device
This avoids needing to search for the cmos_layout.bin cbfs file each time cmos_get or cmos_set is called.
Change-Id: Ic7679269f28d090e9a99625d68b37e0bf02b061a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/pc80/rtc/mc146818rtc.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/35827/2
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35827
to look at the new patch set (#3).
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
drivers/pc80/rtc: Cache cmos_layout.bin region device
This avoids needing to search for the cmos_layout.bin cbfs file each time cmos_get or cmos_set is called.
Change-Id: Ic7679269f28d090e9a99625d68b37e0bf02b061a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/pc80/rtc/mc146818rtc.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/35827/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 278: rdev = car_get_ptr(layout_rdev_p); As Elyes pointed out, something is funky here. We're not setting the memory behind `rdev` anymore, but only the local pointer variable.
I suggest making the parameter a `struct region_device **const`.
(I guess I should start a discussion about declaring `const` what can be declared `const`. This wouldn't have happened if `rdev` were immu- table.)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 252: struct cbfsf fh; This might be all you need, if you just decide to skip optimisation for platforms with CAR migration:
MAYBE_STATIC struct cbfsf fh;
if (! region_device_sz(&(fh->data))) cbfs_locate_in_region();
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 274: cbfs_file_data(cmos_layout_rdev, &fh); You need this.
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 278: rdev = car_get_ptr(layout_rdev_p); You are confusing this with '*rdev = ' perhaps? This only assigns (local) pointer variable a new value.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 278: rdev = car_get_ptr(layout_rdev_p);
(I guess I should start a discussion about declaring `const` what can be declared `const`. This wouldn't have happened if `rdev` were immu- table.)
Nico, what did you mean by this specifically? I'm not following on the specifics.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 278: rdev = car_get_ptr(layout_rdev_p);
(I guess I should start a discussion about declaring `const` what can be declared `const`. This wouldn't have happened if `rdev` were immu- table.)
Nico, what did you mean by this specifically? I'm not following on the specifics.
We could generally make more use of the `const` keyword. Newer languages have actually inverted the concept, e.g. in Rust you have to explicitly declare a variable mutable if you don't want it to be `const`, and I concur. I feel like I've seen many bugs that could have been prevented if the original intention never to change a variable's (or even more often parameter's) value would have been enforced. It also makes code easier to read, IMHO, if one knows that they don't have to track the current state of a `variable`.
I haven't looked into it, maybe there is a compiler warning for that yet (e.g. "... could be declared const but isn't"). If not, I have no clue how we could enforce such a rule...
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 278: rdev = car_get_ptr(layout_rdev_p);
(I guess I should start a discussion about declaring `const` what can […]
Thanks for the response. I agree about the lack of const-ness in general, but I think in this particular case, rdev reqs, it's not directly applicable. In this particular patch it's definitely not the correct way to approach the problem.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/35827/3/src/drivers/pc80/rtc/mc1468... PS3, Line 278: rdev = car_get_ptr(layout_rdev_p); Not sure if we are still talking about the same problem :)
I suggest making the parameter a `struct region_device **const`.
This actually suggested two changes:
1. Use a pointer to a pointer. 2. Make the local variable `const`.
1. Would force an update to the caller and could solve the problem of the change, i.e. we want to return a pointer to a global struct instead of changing a given struct.
2. Just because I would have declared `rdev` `const` in the first place. If that would have been the case from the beginning, the compiler would already have rejected Arthur's patch.
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35827 )
Change subject: drivers/pc80/rtc: Cache cmos_layout.bin region device ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)