Change in coreboot[master]: memranges: Change align attribute to be log2 of required alignment
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 12 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/39810/1
diff --git a/src/include/memrange.h b/src/include/memrange.h index cfd29e7..97669fb 100644 --- a/src/include/memrange.h +++ b/src/include/memrange.h @@ -15,6 +15,7 @@ #define MEMRANGE_H_
#include <device/resource.h> +#include <lib.h> #include <stdbool.h>
/* A memranges structure consists of a list of range_entry(s). The structure @@ -24,7 +25,7 @@ /* coreboot doesn't have a free() function. Therefore, keep a cache of * free'd entries. */ struct range_entry *free_list; - /* Alignment for base and end addresses of the range. (Must be power of 2). */ + /* Alignment(log 2) for base and end addresses of the range. */ size_t align; };
@@ -96,7 +97,7 @@
/* Initialize memranges structure providing an optional array of range_entry * to use as the free list. Additionally, it accepts an align parameter that - * determines the alignment of addresses. (Alignment must be a power of 2). */ + * represents the required alignment(log 2) of addresses. */ void memranges_init_empty_with_alignment(struct memranges *ranges, struct range_entry *free, size_t num_free, size_t align); @@ -104,7 +105,7 @@ /* Initialize and fill a memranges structure according to the * mask and match type for all memory resources. Tag each entry with the * specified type. Additionally, it accepts an align parameter that - * determines the alignment of addresses. (Alignment must be a power of 2). */ + * represents the required alignment(log 2) of addresses. */ void memranges_init_with_alignment(struct memranges *ranges, unsigned long mask, unsigned long match, unsigned long tag, size_t align); @@ -112,13 +113,13 @@ /* Initialize memranges structure providing an optional array of range_entry * to use as the free list. Addresses are default aligned to 4KiB. */ #define memranges_init_empty(__ranges, __free, __num_free) \ - memranges_init_empty_with_alignment(__ranges, __free, __num_free, 4 * KiB) + memranges_init_empty_with_alignment(__ranges, __free, __num_free, log2(4 * KiB))
/* Initialize and fill a memranges structure according to the * mask and match type for all memory resources. Tag each entry with the * specified type. Addresses are default aligned to 4KiB. */ #define memranges_init(__ranges, __mask, __match, __tag) \ - memranges_init_with_alignment(__ranges, __mask, __match, __tag, 4 * KiB) + memranges_init_with_alignment(__ranges, __mask, __match, __tag, log2(4 * KiB))
/* Clone a memrange. The new memrange has the same entries as the old one. */ void memranges_clone(struct memranges *newranges, struct memranges *oldranges); @@ -175,8 +176,7 @@ /* Steals memory from the available list in given ranges as per the constraints: * limit = Upper bound for the memory range to steal. * size = Requested size for the stolen memory. - * align = Alignment requirements for the starting address of the stolen memory. - * (Alignment must be a power of 2). + * align = Required alignment(log 2) for the starting address of the stolen memory. * tag = Use a range that matches the given tag. * * If the constraints can be satisfied, this function creates a hole in the memrange, diff --git a/src/lib/memrange.c b/src/lib/memrange.c index 5fb40df..f3cb907 100644 --- a/src/lib/memrange.c +++ b/src/lib/memrange.c @@ -230,12 +230,12 @@ if (size == 0) return;
- /* The addresses are aligned to 4096 bytes: the begin address is + /* The addresses are aligned to ranges->align bytes: the begin address is * aligned down while the end address is aligned up to be conservative * about the full range covered. */ - begin = ALIGN_DOWN(base, ranges->align); + begin = ALIGN_DOWN(base, 1 << ranges->align); end = begin + size + (base - begin); - end = ALIGN_UP(end, ranges->align) - 1; + end = ALIGN_UP(end, 1 << ranges->align) - 1; action(ranges, begin, end, tag); }
@@ -298,9 +298,6 @@ { size_t i;
- /* Alignment must be a power of 2. */ - assert(IS_POWER_OF_2(align)); - ranges->entries = NULL; ranges->free_list = NULL; ranges->align = align; @@ -403,18 +400,12 @@ if (size == 0) return NULL;
- if (!IS_POWER_OF_2(align)) - return NULL; - - if (!IS_ALIGNED(align, ranges->align)) - return NULL; - memranges_each_entry(r, ranges) {
if (r->tag != tag) continue;
- base = ALIGN_UP(r->begin, align); + base = ALIGN_UP(r->begin, 1 << align); end = base + size - 1;
if (end > r->end) @@ -438,7 +429,7 @@ if (r == NULL) return false;
- base = ALIGN_UP(r->begin, align); + base = ALIGN_UP(r->begin, 1 << align);
memranges_create_hole(ranges, base, size); *stolen_base = base;
Hello Patrick Georgi, Duncan Laurie, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39810
to look at the new patch set (#2).
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 12 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/39810/2
Furquan Shaikh has removed Name of user not set #1000432 from this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Removed reviewer null.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@29 PS2, Line 29: size_t align; Is size_t still justified here, struct resource uses unsigned char.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@29 PS2, Line 29: size_t align;
Is size_t still justified here, struct resource uses unsigned char.
Valid point. I think it makes sense to switch to unsigned char.
Hello build bot (Jenkins), Patrick Georgi, Duncan Laurie, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39810
to look at the new patch set (#3).
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 22 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/39810/3
Hello build bot (Jenkins), Patrick Georgi, Duncan Laurie, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39810
to look at the new patch set (#4).
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
This change also updates the type of align to be unsigned char.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 22 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/39810/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@116 PS2, Line 116: log2(4 * KiB) We could just use '12'. I'm not sure if the compiler is constant folding or this turns into a runtime check.
https://review.coreboot.org/c/coreboot/+/39810/2/src/lib/memrange.c File src/lib/memrange.c:
https://review.coreboot.org/c/coreboot/+/39810/2/src/lib/memrange.c@236 PS2, Line 236: 1 1ULL throughout as resource_t is a 64-bit value.
Hello build bot (Jenkins), Patrick Georgi, Duncan Laurie, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39810
to look at the new patch set (#5).
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
This change also updates the type of align to be unsigned char.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 21 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/39810/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@116 PS2, Line 116: log2(4 * KiB)
We could just use '12'. […]
True. I will update this.
https://review.coreboot.org/c/coreboot/+/39810/2/src/lib/memrange.c File src/lib/memrange.c:
https://review.coreboot.org/c/coreboot/+/39810/2/src/lib/memrange.c@236 PS2, Line 236: 1
1ULL throughout as resource_t is a 64-bit value.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39810/5/src/lib/memrange.c File src/lib/memrange.c:
https://review.coreboot.org/c/coreboot/+/39810/5/src/lib/memrange.c@236 PS5, Line 236: 1ULL << ranges->align Worth adding a helper that spits this out instead of open coding?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 5: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@116 PS2, Line 116: log2(4 * KiB)
True. I will update this.
Maybe leave some indication as to where this 12 comes from?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@116 PS2, Line 116: log2(4 * KiB)
Maybe leave some indication as to where this 12 comes from?
Isn't that the '4KiB' in the comment?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@116 PS2, Line 116: log2(4 * KiB)
Isn't that the '4KiB' in the comment?
It isn't immediately obvious, though. It's just a matter of stating that 4KiB == 2^12
Hello build bot (Jenkins), Patrick Georgi, Duncan Laurie, Angel Pons, Kyösti Mälkki, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39810
to look at the new patch set (#6).
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
This change also updates the type of align to be unsigned char.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 23 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/39810/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@29 PS2, Line 29: size_t align;
Valid point. I think it makes sense to switch to unsigned char.
Done
https://review.coreboot.org/c/coreboot/+/39810/2/src/include/memrange.h@116 PS2, Line 116: log2(4 * KiB)
It isn't immediately obvious, though. […]
Done
https://review.coreboot.org/c/coreboot/+/39810/5/src/lib/memrange.c File src/lib/memrange.c:
https://review.coreboot.org/c/coreboot/+/39810/5/src/lib/memrange.c@236 PS5, Line 236: 1ULL << ranges->align
Worth adding a helper that spits this out instead of open coding?
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39810 )
Change subject: memranges: Change align attribute to be log2 of required alignment ......................................................................
memranges: Change align attribute to be log2 of required alignment
This change updates the align attribute of memranges to be represented as log2 of the required alignment. This makes it consistent with how alignment is stored in struct resource as well.
Additionally, since memranges only allow power of 2 alignments, this change allows getting rid of checks at runtime and hence failure cases for non-power of 2 alignments.
This change also updates the type of align to be unsigned char.
BUG=b:149186922
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ie4d3868cdff55b2c7908b9b3ccd5f30a5288e62f Reviewed-on: https://review.coreboot.org/c/coreboot/+/39810 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/include/memrange.h M src/lib/memrange.c 2 files changed, 23 insertions(+), 33 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/include/memrange.h b/src/include/memrange.h index cfd29e7..dcab791 100644 --- a/src/include/memrange.h +++ b/src/include/memrange.h @@ -24,8 +24,8 @@ /* coreboot doesn't have a free() function. Therefore, keep a cache of * free'd entries. */ struct range_entry *free_list; - /* Alignment for base and end addresses of the range. (Must be power of 2). */ - size_t align; + /* Alignment(log 2) for base and end addresses of the range. */ + unsigned char align; };
/* Each region within a memranges structure is represented by a @@ -96,29 +96,29 @@
/* Initialize memranges structure providing an optional array of range_entry * to use as the free list. Additionally, it accepts an align parameter that - * determines the alignment of addresses. (Alignment must be a power of 2). */ + * represents the required alignment(log 2) of addresses. */ void memranges_init_empty_with_alignment(struct memranges *ranges, struct range_entry *free, - size_t num_free, size_t align); + size_t num_free, unsigned char align);
/* Initialize and fill a memranges structure according to the * mask and match type for all memory resources. Tag each entry with the * specified type. Additionally, it accepts an align parameter that - * determines the alignment of addresses. (Alignment must be a power of 2). */ + * represents the required alignment(log 2) of addresses. */ void memranges_init_with_alignment(struct memranges *ranges, unsigned long mask, unsigned long match, - unsigned long tag, size_t align); + unsigned long tag, unsigned char align);
/* Initialize memranges structure providing an optional array of range_entry - * to use as the free list. Addresses are default aligned to 4KiB. */ + * to use as the free list. Addresses are default aligned to 4KiB(2^12). */ #define memranges_init_empty(__ranges, __free, __num_free) \ - memranges_init_empty_with_alignment(__ranges, __free, __num_free, 4 * KiB) + memranges_init_empty_with_alignment(__ranges, __free, __num_free, 12);
/* Initialize and fill a memranges structure according to the * mask and match type for all memory resources. Tag each entry with the - * specified type. Addresses are default aligned to 4KiB. */ + * specified type. Addresses are default aligned to 4KiB(2^12). */ #define memranges_init(__ranges, __mask, __match, __tag) \ - memranges_init_with_alignment(__ranges, __mask, __match, __tag, 4 * KiB) + memranges_init_with_alignment(__ranges, __mask, __match, __tag, 12);
/* Clone a memrange. The new memrange has the same entries as the old one. */ void memranges_clone(struct memranges *newranges, struct memranges *oldranges); @@ -175,14 +175,13 @@ /* Steals memory from the available list in given ranges as per the constraints: * limit = Upper bound for the memory range to steal. * size = Requested size for the stolen memory. - * align = Alignment requirements for the starting address of the stolen memory. - * (Alignment must be a power of 2). + * align = Required alignment(log 2) for the starting address of the stolen memory. * tag = Use a range that matches the given tag. * * If the constraints can be satisfied, this function creates a hole in the memrange, * writes the base address of that hole to stolen_base and returns true. Otherwise it returns * false. */ -bool memranges_steal(struct memranges *ranges, resource_t limit, resource_t size, size_t align, - unsigned long tag, resource_t *stolen_base); +bool memranges_steal(struct memranges *ranges, resource_t limit, resource_t size, + unsigned char align, unsigned long tag, resource_t *stolen_base);
#endif /* MEMRANGE_H_ */ diff --git a/src/lib/memrange.c b/src/lib/memrange.c index 5fb40df..fd7a4d6 100644 --- a/src/lib/memrange.c +++ b/src/lib/memrange.c @@ -230,12 +230,12 @@ if (size == 0) return;
- /* The addresses are aligned to 4096 bytes: the begin address is + /* The addresses are aligned to (1ULL << ranges->align): the begin address is * aligned down while the end address is aligned up to be conservative * about the full range covered. */ - begin = ALIGN_DOWN(base, ranges->align); + begin = ALIGN_DOWN(base, POWER_OF_2(ranges->align)); end = begin + size + (base - begin); - end = ALIGN_UP(end, ranges->align) - 1; + end = ALIGN_UP(end, POWER_OF_2(ranges->align)) - 1; action(ranges, begin, end, tag); }
@@ -294,13 +294,10 @@
void memranges_init_empty_with_alignment(struct memranges *ranges, struct range_entry *to_free, - size_t num_free, size_t align) + size_t num_free, unsigned char align) { size_t i;
- /* Alignment must be a power of 2. */ - assert(IS_POWER_OF_2(align)); - ranges->entries = NULL; ranges->free_list = NULL; ranges->align = align; @@ -311,7 +308,7 @@
void memranges_init_with_alignment(struct memranges *ranges, unsigned long mask, unsigned long match, - unsigned long tag, size_t align) + unsigned long tag, unsigned char align) { memranges_init_empty_with_alignment(ranges, NULL, 0, align); memranges_add_resources(ranges, mask, match, tag); @@ -395,7 +392,7 @@ * required alignment, is big enough, does not exceed the limit and has a matching tag. */ static const struct range_entry *memranges_find_entry(struct memranges *ranges, resource_t limit, resource_t size, - size_t align, unsigned long tag) + unsigned char align, unsigned long tag) { const struct range_entry *r; resource_t base, end; @@ -403,18 +400,12 @@ if (size == 0) return NULL;
- if (!IS_POWER_OF_2(align)) - return NULL; - - if (!IS_ALIGNED(align, ranges->align)) - return NULL; - memranges_each_entry(r, ranges) {
if (r->tag != tag) continue;
- base = ALIGN_UP(r->begin, align); + base = ALIGN_UP(r->begin, POWER_OF_2(align)); end = base + size - 1;
if (end > r->end) @@ -429,8 +420,8 @@ return NULL; }
-bool memranges_steal(struct memranges *ranges, resource_t limit, resource_t size, size_t align, - unsigned long tag, resource_t *stolen_base) +bool memranges_steal(struct memranges *ranges, resource_t limit, resource_t size, + unsigned char align, unsigned long tag, resource_t *stolen_base) { resource_t base; const struct range_entry *r = memranges_find_entry(ranges, limit, size, align, tag); @@ -438,7 +429,7 @@ if (r == NULL) return false;
- base = ALIGN_UP(r->begin, align); + base = ALIGN_UP(r->begin, POWER_OF_2(align));
memranges_create_hole(ranges, base, size); *stolen_base = base;
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)