Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )
Change subject: device/pci: Don't enabled unassigned resources ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@517 PS1, Line 517: resource->size = 0;
can we just call pci_store_bridge_resource() here and return?
Hmmm, yes, if we'd move the ASSIGNED check below all the other ones. In other words, the only thing we want to skip is the COMMAND bits, right?
https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@558 PS1, Line 558: resource->flags |= IORESOURCE_STORED;
Shouldn't we set this in the respective functions (pci_store_bridge_resource() and pci_store_resourc […]
Uff, no feelings here. IIRC, report_resource_stored() actually checks IORESOURCE_STORED by itself. So it expects to be always called, which made it weird enough to me to not change the flow ;)
https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@575 PS1, Line 575: for (res = dev->resource_list; res; res = res->next) : pci_set_resource(dev, res, &command_mask); : /* If there are unassigned resources, we might : have to disable others of the same type. */ : dev->command &= command_mask; :
We have to gather information from all resource first and then make the final decision
Bridge resources never result in clearing of command bits. Thus, if non-bridge resources are considered after bridge resources, you can rely on the state provided by non-bridge resource. Also, we don't have to set the command bit in the loop and instead we can use a flag in pci_dev_set_nonbridge_resources_by_type() to decide if command bit should be set.
Not sure where we are talking past each other. I'll give an example of what we might have to expect from a random PCI device:
{ { .base = 0, .size = 0x10000, .flags = IORESOURCE_MEM, .index = 0x10, }, { .base = 0xc32fc000, .size = 0x4000, .flags = IORESOURCE_ASSIGNED | IORESOURCE_MEM, .index = 0x14, }, }
This can happen. A PCI device can have multiple resources of the same type and if a later one is smaller, the allocator might have found space for it even if it failed for an earlier, bigger resource. But in the PCI case, we must either have all resources of a type assigned or not set the COMMAND bit.