Change in coreboot[master]: device/resource_allocator_v4: Improve the logging in resource allocator
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
device/resource_allocator_v4: Improve the logging in resource allocator
This change makes the following improvements to debug logging in resource allocator: 1. Print depth is added to functions in pass 1 to better represent how the resource requirements of child devices impact the resource windows for parent bridge. 2. Device path is added to resource ranges to make it easier to understand what device the resouce ranges are associated with. 3. Prints in pass 2 (update constraints, resource ranges, resource assignment) are shifted left by 1 to make it easier to visualize resource allocation for each bridge including domain.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3356a7278060e281d1a57d253537b097472827a1 --- M src/device/resource_allocator_v4.c 1 file changed, 29 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/41478/1
diff --git a/src/device/resource_allocator_v4.c b/src/device/resource_allocator_v4.c index 916f3b8..adccbe5 100644 --- a/src/device/resource_allocator_v4.c +++ b/src/device/resource_allocator_v4.c @@ -34,6 +34,8 @@ return bus && bus->children; }
+#define res_printk(depth, str, ...) printk(BIOS_DEBUG, "%*c"str, depth, ' ', __VA_ARGS__) + /* * During pass 1, once all the requirements for downstream devices of a bridge are gathered, * this function calculates the overall resource requirement for the bridge. It starts by @@ -46,7 +48,7 @@ * on the tightest constraints downstream. */ static void update_bridge_resource(const struct device *bridge, struct resource *bridge_res, - unsigned long type_match) + unsigned long type_match, int print_depth) { const struct device *child; struct resource *child_res; @@ -67,7 +69,7 @@ */ base = 0;
- printk(BIOS_DEBUG, "%s %s: size: %llx align: %d gran: %d limit: %llx\n", + res_printk(print_depth, "%s %s: size: %llx align: %d gran: %d limit: %llx\n", dev_path(bridge), resource2str(bridge_res), bridge_res->size, bridge_res->align, bridge_res->gran, bridge_res->limit);
@@ -122,7 +124,7 @@ */ base = round(base, child_res->align);
- printk(BIOS_DEBUG, "%s %02lx * [0x%llx - 0x%llx] %s\n", + res_printk(print_depth + 1, "%s %02lx * [0x%llx - 0x%llx] %s\n", dev_path(child), child_res->index, base, base + child_res->size - 1, resource2str(child_res));
@@ -137,7 +139,7 @@ */ bridge_res->size = round(base, bridge_res->gran);
- printk(BIOS_DEBUG, "%s %s: size: %llx align: %d gran: %d limit: %llx done\n", + res_printk(print_depth, "%s %s: size: %llx align: %d gran: %d limit: %llx done\n", dev_path(bridge), resource2str(bridge_res), bridge_res->size, bridge_res->align, bridge_res->gran, bridge_res->limit); } @@ -146,7 +148,8 @@ * During pass 1, resource allocator at bridge level gathers requirements from downstream * devices and updates its own resource windows for the provided resource type. */ -static void compute_bridge_resources(const struct device *bridge, unsigned long type_match) +static void compute_bridge_resources(const struct device *bridge, unsigned long type_match, + int print_depth) { const struct device *child; struct resource *res; @@ -167,14 +170,14 @@ for (child = bus->children; child; child = child->sibling) { if (!dev_has_children(child)) continue; - compute_bridge_resources(child, type_match); + compute_bridge_resources(child, type_match, print_depth + 1); }
/* * Update the window for current bridge resource now that all downstream * requirements are gathered. */ - update_bridge_resource(bridge, res, type_match); + update_bridge_resource(bridge, res, type_match, print_depth); } }
@@ -194,6 +197,7 @@ static void compute_domain_resources(const struct device *domain) { const struct device *child; + const int print_depth = 1;
if (domain->link_list == NULL) return; @@ -204,9 +208,10 @@ if (!dev_has_children(child)) continue;
- compute_bridge_resources(child, IORESOURCE_IO); - compute_bridge_resources(child, IORESOURCE_MEM); - compute_bridge_resources(child, IORESOURCE_MEM | IORESOURCE_PREFETCH); + compute_bridge_resources(child, IORESOURCE_IO, print_depth); + compute_bridge_resources(child, IORESOURCE_MEM, print_depth); + compute_bridge_resources(child, IORESOURCE_MEM | IORESOURCE_PREFETCH, + print_depth); } }
@@ -315,17 +320,17 @@ memranges_insert(ranges, res->base, res->limit - res->base + 1, memrange_type); }
-static void print_resource_ranges(const struct memranges *ranges) +static void print_resource_ranges(const struct device *dev, const struct memranges *ranges) { const struct range_entry *r;
- printk(BIOS_INFO, "Resource ranges:\n"); + printk(BIOS_INFO, " %s: Resource ranges:\n", dev_path(dev));
if (memranges_is_empty(ranges)) - printk(BIOS_INFO, "EMPTY!!\n"); + printk(BIOS_INFO, " * EMPTY!!\n");
memranges_each_entry(r, ranges) { - printk(BIOS_INFO, "Base: %llx, Size: %llx, Tag: %lx\n", + printk(BIOS_INFO, " * Base: %llx, Size: %llx, Tag: %lx\n", range_entry_base(r), range_entry_size(r), range_entry_tag(r)); } } @@ -362,8 +367,8 @@
if (memranges_steal(ranges, resource->limit, resource->size, resource->align, type_match, &resource->base) == false) { - printk(BIOS_ERR, "ERROR: Resource didn't fit!!! "); - printk(BIOS_DEBUG, "%s %02lx * size: 0x%llx limit: %llx %s\n", + printk(BIOS_ERR, " ERROR: Resource didn't fit!!! "); + printk(BIOS_DEBUG, " %s %02lx * size: 0x%llx limit: %llx %s\n", dev_path(dev), resource->index, resource->size, resource->limit, resource2str(resource)); mark_resource_invalid(resource); @@ -373,7 +378,7 @@ resource->limit = resource->base + resource->size - 1; resource->flags |= IORESOURCE_ASSIGNED;
- printk(BIOS_DEBUG, "%s %02lx * [0x%llx - 0x%llx] limit: %llx %s\n", + printk(BIOS_DEBUG, " %s %02lx * [0x%llx - 0x%llx] limit: %llx %s\n", dev_path(dev), resource->index, resource->base, resource->size ? resource->base + resource->size - 1 : resource->base, resource->limit, resource2str(resource)); @@ -386,7 +391,7 @@ if (!res->size) return;
- printk(BIOS_DEBUG, "%s: %s %02lx base %08llx limit %08llx %s (fixed)\n", + printk(BIOS_DEBUG, " %s: %s %02lx base %08llx limit %08llx %s (fixed)\n", __func__, dev_path(dev), res->index, res->base, res->base + res->size - 1, resource2str(res));
@@ -470,7 +475,7 @@ initialize_bridge_memranges(ranges, res, type); }
- print_resource_ranges(ranges); + print_resource_ranges(dev, ranges); }
static void cleanup_resource_ranges(const struct device *dev, struct memranges *ranges, @@ -658,13 +663,16 @@ post_log_path(child);
/* Pass 1 - Gather requirements. */ - printk(BIOS_INFO, "Resource allocator: %s - Pass 1 (gathering requirements)\n", + printk(BIOS_INFO, "==== Resource allocator: %s - Pass 1 (gathering requirements) ===\n", dev_path(child)); compute_domain_resources(child);
/* Pass 2 - Allocate resources as per gathered requirements. */ - printk(BIOS_INFO, "Resource allocator: %s - Pass 2 (allocating resources)\n", + printk(BIOS_INFO, "=== Resource allocator: %s - Pass 2 (allocating resources) ===\n", dev_path(child)); allocate_domain_resources(child); + + printk(BIOS_INFO, "=== Resource allocator: %s - resource allocation complete ===\n", + dev_path(child)); } }
Furquan Shaikh has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
device/resource_allocator_v4: Improve the logging in resource allocator
This change makes the following improvements to debug logging in resource allocator: 1. Print depth is added to functions in pass 1 to better represent how the resource requirements of child devices impact the resource windows for parent bridge. 2. Device path is added to resource ranges to make it easier to understand what device the resouce ranges are associated with. 3. Prints in pass 2 (update constraints, resource ranges, resource assignment) are shifted left by 1 to make it easier to visualize resource allocation for each bridge including domain.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3356a7278060e281d1a57d253537b097472827a1 --- M src/device/resource_allocator_v4.c 1 file changed, 29 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/41478/2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
Patch Set 3:
I've used current changes rebased on CB:41566 and rebased on master Got only one boot and some pix :) 1st pix: https://www.dropbox.com/s/4gvb8p88urleg0k/error.jpeg?dl=0 here it start to boot 2nd pix: got the time to store a dmesg , then the system start to display errors on the ata0 https://www.dropbox.com/s/y8sz6ep36mrvkuw/disk-error.jpg?dl=0
here is the dmesg: https://www.dropbox.com/s/x074pe7pviukt68/dmesg-41478.txt?dl=0 and the log: https://www.dropbox.com/s/3aqwn0qm6nuli7e/41478.log?dl=0
after the 1st boot, it didn't find the disk.
I'm sorry, you lost time with this issue, which is not linked to your patch. Thank you so much for your time and your help.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
Patch Set 3:
Patch Set 3:
I've used current changes rebased on CB:41566 and rebased on master Got only one boot and some pix :) 1st pix: https://www.dropbox.com/s/4gvb8p88urleg0k/error.jpeg?dl=0 here it start to boot 2nd pix: got the time to store a dmesg , then the system start to display errors on the ata0 https://www.dropbox.com/s/y8sz6ep36mrvkuw/disk-error.jpg?dl=0
here is the dmesg: https://www.dropbox.com/s/x074pe7pviukt68/dmesg-41478.txt?dl=0 and the log: https://www.dropbox.com/s/3aqwn0qm6nuli7e/41478.log?dl=0
after the 1st boot, it didn't find the disk.
I'm sorry, you lost time with this issue, which is not linked to your patch. Thank you so much for your time and your help.
I saw "PGTABLE" in dmesg. this is odd, isn't it?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
I've used current changes rebased on CB:41566 and rebased on master Got only one boot and some pix :) 1st pix: https://www.dropbox.com/s/4gvb8p88urleg0k/error.jpeg?dl=0 here it start to boot 2nd pix: got the time to store a dmesg , then the system start to display errors on the ata0 https://www.dropbox.com/s/y8sz6ep36mrvkuw/disk-error.jpg?dl=0
here is the dmesg: https://www.dropbox.com/s/x074pe7pviukt68/dmesg-41478.txt?dl=0 and the log: https://www.dropbox.com/s/3aqwn0qm6nuli7e/41478.log?dl=0
after the 1st boot, it didn't find the disk.
I'm sorry, you lost time with this issue, which is not linked to your patch. Thank you so much for your time and your help.
I saw "PGTABLE" in dmesg. this is odd, isn't it?
sorry for the noise :)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41478 )
Change subject: device/resource_allocator_v4: Improve the logging in resource allocator ......................................................................
device/resource_allocator_v4: Improve the logging in resource allocator
This change makes the following improvements to debug logging in resource allocator: 1. Print depth is added to functions in pass 1 to better represent how the resource requirements of child devices impact the resource windows for parent bridge. 2. Device path is added to resource ranges to make it easier to understand what device the resouce ranges are associated with. 3. Prints in pass 2 (update constraints, resource ranges, resource assignment) are shifted left by 1 to make it easier to visualize resource allocation for each bridge including domain.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I3356a7278060e281d1a57d253537b097472827a1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41478 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/resource_allocator_v4.c 1 file changed, 29 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/device/resource_allocator_v4.c b/src/device/resource_allocator_v4.c index 75e8392..3a11032 100644 --- a/src/device/resource_allocator_v4.c +++ b/src/device/resource_allocator_v4.c @@ -34,6 +34,8 @@ return bus && bus->children; }
+#define res_printk(depth, str, ...) printk(BIOS_DEBUG, "%*c"str, depth, ' ', __VA_ARGS__) + /* * During pass 1, once all the requirements for downstream devices of a bridge are gathered, * this function calculates the overall resource requirement for the bridge. It starts by @@ -46,7 +48,7 @@ * on the tightest constraints downstream. */ static void update_bridge_resource(const struct device *bridge, struct resource *bridge_res, - unsigned long type_match) + unsigned long type_match, int print_depth) { const struct device *child; struct resource *child_res; @@ -67,7 +69,7 @@ */ base = 0;
- printk(BIOS_DEBUG, "%s %s: size: %llx align: %d gran: %d limit: %llx\n", + res_printk(print_depth, "%s %s: size: %llx align: %d gran: %d limit: %llx\n", dev_path(bridge), resource2str(bridge_res), bridge_res->size, bridge_res->align, bridge_res->gran, bridge_res->limit);
@@ -122,7 +124,7 @@ */ base = round(base, child_res->align);
- printk(BIOS_DEBUG, "%s %02lx * [0x%llx - 0x%llx] %s\n", + res_printk(print_depth + 1, "%s %02lx * [0x%llx - 0x%llx] %s\n", dev_path(child), child_res->index, base, base + child_res->size - 1, resource2str(child_res));
@@ -137,7 +139,7 @@ */ bridge_res->size = round(base, bridge_res->gran);
- printk(BIOS_DEBUG, "%s %s: size: %llx align: %d gran: %d limit: %llx done\n", + res_printk(print_depth, "%s %s: size: %llx align: %d gran: %d limit: %llx done\n", dev_path(bridge), resource2str(bridge_res), bridge_res->size, bridge_res->align, bridge_res->gran, bridge_res->limit); } @@ -146,7 +148,8 @@ * During pass 1, resource allocator at bridge level gathers requirements from downstream * devices and updates its own resource windows for the provided resource type. */ -static void compute_bridge_resources(const struct device *bridge, unsigned long type_match) +static void compute_bridge_resources(const struct device *bridge, unsigned long type_match, + int print_depth) { const struct device *child; struct resource *res; @@ -167,14 +170,14 @@ for (child = bus->children; child; child = child->sibling) { if (!dev_has_children(child)) continue; - compute_bridge_resources(child, type_match); + compute_bridge_resources(child, type_match, print_depth + 1); }
/* * Update the window for current bridge resource now that all downstream * requirements are gathered. */ - update_bridge_resource(bridge, res, type_match); + update_bridge_resource(bridge, res, type_match, print_depth); } }
@@ -194,6 +197,7 @@ static void compute_domain_resources(const struct device *domain) { const struct device *child; + const int print_depth = 1;
if (domain->link_list == NULL) return; @@ -204,9 +208,10 @@ if (!dev_has_children(child)) continue;
- compute_bridge_resources(child, IORESOURCE_IO); - compute_bridge_resources(child, IORESOURCE_MEM); - compute_bridge_resources(child, IORESOURCE_MEM | IORESOURCE_PREFETCH); + compute_bridge_resources(child, IORESOURCE_IO, print_depth); + compute_bridge_resources(child, IORESOURCE_MEM, print_depth); + compute_bridge_resources(child, IORESOURCE_MEM | IORESOURCE_PREFETCH, + print_depth); } }
@@ -335,17 +340,17 @@ memranges_insert(ranges, res->base, res->limit - res->base + 1, memrange_type); }
-static void print_resource_ranges(const struct memranges *ranges) +static void print_resource_ranges(const struct device *dev, const struct memranges *ranges) { const struct range_entry *r;
- printk(BIOS_INFO, "Resource ranges:\n"); + printk(BIOS_INFO, " %s: Resource ranges:\n", dev_path(dev));
if (memranges_is_empty(ranges)) - printk(BIOS_INFO, "EMPTY!!\n"); + printk(BIOS_INFO, " * EMPTY!!\n");
memranges_each_entry(r, ranges) { - printk(BIOS_INFO, "Base: %llx, Size: %llx, Tag: %lx\n", + printk(BIOS_INFO, " * Base: %llx, Size: %llx, Tag: %lx\n", range_entry_base(r), range_entry_size(r), range_entry_tag(r)); } } @@ -370,8 +375,8 @@
if (memranges_steal(ranges, resource->limit, resource->size, resource->align, type_match, &resource->base) == false) { - printk(BIOS_ERR, "ERROR: Resource didn't fit!!! "); - printk(BIOS_DEBUG, "%s %02lx * size: 0x%llx limit: %llx %s\n", + printk(BIOS_ERR, " ERROR: Resource didn't fit!!! "); + printk(BIOS_DEBUG, " %s %02lx * size: 0x%llx limit: %llx %s\n", dev_path(dev), resource->index, resource->size, resource->limit, resource2str(resource)); continue; @@ -380,7 +385,7 @@ resource->limit = resource->base + resource->size - 1; resource->flags |= IORESOURCE_ASSIGNED;
- printk(BIOS_DEBUG, "%s %02lx * [0x%llx - 0x%llx] limit: %llx %s\n", + printk(BIOS_DEBUG, " %s %02lx * [0x%llx - 0x%llx] limit: %llx %s\n", dev_path(dev), resource->index, resource->base, resource->size ? resource->base + resource->size - 1 : resource->base, resource->limit, resource2str(resource)); @@ -393,7 +398,7 @@ if (!res->size) return;
- printk(BIOS_DEBUG, "%s: %s %02lx base %08llx limit %08llx %s (fixed)\n", + printk(BIOS_DEBUG, " %s: %s %02lx base %08llx limit %08llx %s (fixed)\n", __func__, dev_path(dev), res->index, res->base, res->base + res->size - 1, resource2str(res));
@@ -477,7 +482,7 @@ initialize_bridge_memranges(ranges, res, type); }
- print_resource_ranges(ranges); + print_resource_ranges(dev, ranges); }
static void cleanup_resource_ranges(const struct device *dev, struct memranges *ranges, @@ -665,13 +670,16 @@ post_log_path(child);
/* Pass 1 - Gather requirements. */ - printk(BIOS_INFO, "Resource allocator: %s - Pass 1 (gathering requirements)\n", + printk(BIOS_INFO, "==== Resource allocator: %s - Pass 1 (gathering requirements) ===\n", dev_path(child)); compute_domain_resources(child);
/* Pass 2 - Allocate resources as per gathered requirements. */ - printk(BIOS_INFO, "Resource allocator: %s - Pass 2 (allocating resources)\n", + printk(BIOS_INFO, "=== Resource allocator: %s - Pass 2 (allocating resources) ===\n", dev_path(child)); allocate_domain_resources(child); + + printk(BIOS_INFO, "=== Resource allocator: %s - resource allocation complete ===\n", + dev_path(child)); } }
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
HAOUAS Elyes (Code Review)
-
Patrick Georgi (Code Review)