Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
sb/intel: Fix bad uses of `find_resource`
The returned value of `find_resource` can never be null, as the function calls `probe_resource` and dies if it returned null. So, if southbridge code checks if the returned value of `find_resource` is null, replace it with `probe_resource` instead.
Change-Id: I22e2e2ce046cc7ac6f2f2a814d694ff50a8dc89b Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/bd82x6x/azalia.c M src/southbridge/intel/bd82x6x/me.c M src/southbridge/intel/bd82x6x/me_8.x.c M src/southbridge/intel/bd82x6x/sata.c M src/southbridge/intel/bd82x6x/smbus.c M src/southbridge/intel/bd82x6x/usb_ehci.c M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/sata.c M src/southbridge/intel/i82801ix/hdaudio.c M src/southbridge/intel/i82801ix/sata.c M src/southbridge/intel/i82801jx/hdaudio.c M src/southbridge/intel/ibexpeak/azalia.c M src/southbridge/intel/ibexpeak/me.c M src/southbridge/intel/ibexpeak/smbus.c M src/southbridge/intel/ibexpeak/thermal.c M src/southbridge/intel/ibexpeak/usb_ehci.c M src/southbridge/intel/lynxpoint/azalia.c M src/southbridge/intel/lynxpoint/me_9.x.c M src/southbridge/intel/lynxpoint/serialio.c M src/southbridge/intel/lynxpoint/smbus.c 20 files changed, 25 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/42802/1
diff --git a/src/southbridge/intel/bd82x6x/azalia.c b/src/southbridge/intel/bd82x6x/azalia.c index 4f5d8ca..4bb45e5 100644 --- a/src/southbridge/intel/bd82x6x/azalia.c +++ b/src/southbridge/intel/bd82x6x/azalia.c @@ -218,7 +218,7 @@ u32 reg32;
/* Find base address */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res) return;
diff --git a/src/southbridge/intel/bd82x6x/me.c b/src/southbridge/intel/bd82x6x/me.c index 03b954f..4d62b80 100644 --- a/src/southbridge/intel/bd82x6x/me.c +++ b/src/southbridge/intel/bd82x6x/me.c @@ -556,7 +556,7 @@ struct mei_csr host;
/* Find the MMIO base for the ME interface */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res || res->base == 0 || res->size == 0) { printk(BIOS_DEBUG, "ME: MEI resource not present!\n"); return -1; diff --git a/src/southbridge/intel/bd82x6x/me_8.x.c b/src/southbridge/intel/bd82x6x/me_8.x.c index ff94a88..da2ef6a 100644 --- a/src/southbridge/intel/bd82x6x/me_8.x.c +++ b/src/southbridge/intel/bd82x6x/me_8.x.c @@ -541,7 +541,7 @@ struct mei_csr host;
/* Find the MMIO base for the ME interface */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res || res->base == 0 || res->size == 0) { printk(BIOS_DEBUG, "ME: MEI resource not present!\n"); return -1; diff --git a/src/southbridge/intel/bd82x6x/sata.c b/src/southbridge/intel/bd82x6x/sata.c index 63801a2..2b03140 100644 --- a/src/southbridge/intel/bd82x6x/sata.c +++ b/src/southbridge/intel/bd82x6x/sata.c @@ -40,28 +40,28 @@ if (sata_mode != 2) return;
- res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (res) { res->base = 0x1f0; res->size = 8; res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; }
- res = find_resource(dev, PCI_BASE_ADDRESS_1); + res = probe_resource(dev, PCI_BASE_ADDRESS_1); if (res) { res->base = 0x3f4; res->size = 4; res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; }
- res = find_resource(dev, PCI_BASE_ADDRESS_2); + res = probe_resource(dev, PCI_BASE_ADDRESS_2); if (res) { res->base = 0x170; res->size = 8; res->flags = IORESOURCE_IO | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; }
- res = find_resource(dev, PCI_BASE_ADDRESS_3); + res = probe_resource(dev, PCI_BASE_ADDRESS_3); if (res) { res->base = 0x374; res->size = 4; @@ -77,7 +77,7 @@ if (sata_mode == 2) { unsigned int i; for (i = PCI_BASE_ADDRESS_0; i <= PCI_BASE_ADDRESS_3; i += 4) { - struct resource *const res = find_resource(dev, i); + struct resource *const res = probe_resource(dev, i); if (res) res->flags &= ~IORESOURCE_FIXED; } diff --git a/src/southbridge/intel/bd82x6x/smbus.c b/src/southbridge/intel/bd82x6x/smbus.c index dcd2724..e5255d3 100644 --- a/src/southbridge/intel/bd82x6x/smbus.c +++ b/src/southbridge/intel/bd82x6x/smbus.c @@ -21,7 +21,7 @@ pci_write_config32(dev, 0x80, reg16);
/* Set Receive Slave Address */ - res = find_resource(dev, PCI_BASE_ADDRESS_4); + res = probe_resource(dev, PCI_BASE_ADDRESS_4); if (res) smbus_set_slave_addr(res->base, SMBUS_SLAVE_ADDR); } diff --git a/src/southbridge/intel/bd82x6x/usb_ehci.c b/src/southbridge/intel/bd82x6x/usb_ehci.c index 8bc45f6..2764861 100644 --- a/src/southbridge/intel/bd82x6x/usb_ehci.c +++ b/src/southbridge/intel/bd82x6x/usb_ehci.c @@ -43,7 +43,7 @@ /* Enable writes to protected registers. */ pci_write_config8(dev, 0x80, access_cntl | 1);
- res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (res) { /* Number of ports and companion controllers. */ reg32 = read32((void *)(uintptr_t)(res->base + 4)); diff --git a/src/southbridge/intel/i82801gx/azalia.c b/src/southbridge/intel/i82801gx/azalia.c index 91f5857..5f8e89c 100644 --- a/src/southbridge/intel/i82801gx/azalia.c +++ b/src/southbridge/intel/i82801gx/azalia.c @@ -264,7 +264,7 @@ // pm_iowrite(0x59, 0xB); #endif
- res = find_resource(dev, 0x10); + res = probe_resource(dev, 0x10); if (!res) return;
diff --git a/src/southbridge/intel/i82801gx/sata.c b/src/southbridge/intel/i82801gx/sata.c index 6efdef7..7fb1433 100644 --- a/src/southbridge/intel/i82801gx/sata.c +++ b/src/southbridge/intel/i82801gx/sata.c @@ -132,7 +132,7 @@ /* Interrupt Pin is set by D31IP.PIP */ pci_write_config8(dev, INTR_LN, 0x0a);
- struct resource *ahci_res = find_resource(dev, PCI_BASE_ADDRESS_5); + struct resource *ahci_res = probe_resource(dev, PCI_BASE_ADDRESS_5); if (ahci_res != NULL) /* write AHCI GHC_PI register */ write32(res2mmio(ahci_res, 0xc, 0), config->sata_ports_implemented); diff --git a/src/southbridge/intel/i82801ix/hdaudio.c b/src/southbridge/intel/i82801ix/hdaudio.c index c954f9c..3158000 100644 --- a/src/southbridge/intel/i82801ix/hdaudio.c +++ b/src/southbridge/intel/i82801ix/hdaudio.c @@ -228,7 +228,7 @@ /* Lock some R/WO bits by writing their current value. */ pci_update_config32(dev, 0x74, ~0, 0);
- res = find_resource(dev, 0x10); + res = probe_resource(dev, 0x10); if (!res) return;
diff --git a/src/southbridge/intel/i82801ix/sata.c b/src/southbridge/intel/i82801ix/sata.c index cac0375..8ac9bee 100644 --- a/src/southbridge/intel/i82801ix/sata.c +++ b/src/southbridge/intel/i82801ix/sata.c @@ -23,7 +23,7 @@ struct resource *res;
/* Initialize AHCI memory-mapped space */ - res = find_resource(dev, PCI_BASE_ADDRESS_5); + res = probe_resource(dev, PCI_BASE_ADDRESS_5); if (!res) return;
diff --git a/src/southbridge/intel/i82801jx/hdaudio.c b/src/southbridge/intel/i82801jx/hdaudio.c index ba20a6a..cb54243 100644 --- a/src/southbridge/intel/i82801jx/hdaudio.c +++ b/src/southbridge/intel/i82801jx/hdaudio.c @@ -228,7 +228,7 @@ /* Lock some R/WO bits by writing their current value. */ pci_update_config32(dev, 0x74, ~0, 0);
- res = find_resource(dev, 0x10); + res = probe_resource(dev, 0x10); if (!res) return;
diff --git a/src/southbridge/intel/ibexpeak/azalia.c b/src/southbridge/intel/ibexpeak/azalia.c index 59a384a..6bc9a4f 100644 --- a/src/southbridge/intel/ibexpeak/azalia.c +++ b/src/southbridge/intel/ibexpeak/azalia.c @@ -216,7 +216,7 @@ u32 reg32;
/* Find base address */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res) return;
diff --git a/src/southbridge/intel/ibexpeak/me.c b/src/southbridge/intel/ibexpeak/me.c index 2c9c87c..7e35d40 100644 --- a/src/southbridge/intel/ibexpeak/me.c +++ b/src/southbridge/intel/ibexpeak/me.c @@ -478,7 +478,7 @@ u16 reg16;
/* Find the MMIO base for the ME interface */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res || res->base == 0 || res->size == 0) { printk(BIOS_DEBUG, "ME: MEI resource not present!\n"); return -1; diff --git a/src/southbridge/intel/ibexpeak/smbus.c b/src/southbridge/intel/ibexpeak/smbus.c index 7e9aa57..e5c43e5 100644 --- a/src/southbridge/intel/ibexpeak/smbus.c +++ b/src/southbridge/intel/ibexpeak/smbus.c @@ -20,7 +20,7 @@ pci_write_config32(dev, 0x80, reg16);
/* Set Receive Slave Address */ - res = find_resource(dev, PCI_BASE_ADDRESS_4); + res = probe_resource(dev, PCI_BASE_ADDRESS_4); if (res) smbus_set_slave_addr(res->base, SMBUS_SLAVE_ADDR); } diff --git a/src/southbridge/intel/ibexpeak/thermal.c b/src/southbridge/intel/ibexpeak/thermal.c index 0b496da..3ff31e1 100644 --- a/src/southbridge/intel/ibexpeak/thermal.c +++ b/src/southbridge/intel/ibexpeak/thermal.c @@ -13,7 +13,7 @@ u8 *base; printk(BIOS_DEBUG, "Thermal init start.\n");
- res = find_resource(dev, 0x10); + res = probe_resource(dev, 0x10); if (!res) return;
diff --git a/src/southbridge/intel/ibexpeak/usb_ehci.c b/src/southbridge/intel/ibexpeak/usb_ehci.c index 7ad9929..a597a56 100644 --- a/src/southbridge/intel/ibexpeak/usb_ehci.c +++ b/src/southbridge/intel/ibexpeak/usb_ehci.c @@ -36,7 +36,7 @@ /* Enable writes to protected registers. */ pci_write_config8(dev, 0x80, access_cntl | 1);
- res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (res) { /* Number of ports and companion controllers. */ reg32 = read32((u32 *)(uintptr_t)(res->base + 4)); diff --git a/src/southbridge/intel/lynxpoint/azalia.c b/src/southbridge/intel/lynxpoint/azalia.c index f415170..cc29efa 100644 --- a/src/southbridge/intel/lynxpoint/azalia.c +++ b/src/southbridge/intel/lynxpoint/azalia.c @@ -117,7 +117,7 @@ u32 codec_mask;
/* Find base address */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res) return;
diff --git a/src/southbridge/intel/lynxpoint/me_9.x.c b/src/southbridge/intel/lynxpoint/me_9.x.c index cdae271..658a4f4 100644 --- a/src/southbridge/intel/lynxpoint/me_9.x.c +++ b/src/southbridge/intel/lynxpoint/me_9.x.c @@ -727,7 +727,7 @@ struct mei_csr host;
/* Find the MMIO base for the ME interface */ - res = find_resource(dev, PCI_BASE_ADDRESS_0); + res = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!res || res->base == 0 || res->size == 0) { printk(BIOS_DEBUG, "ME: MEI resource not present!\n"); return -1; diff --git a/src/southbridge/intel/lynxpoint/serialio.c b/src/southbridge/intel/lynxpoint/serialio.c index 3973a8d..00bc691 100644 --- a/src/southbridge/intel/lynxpoint/serialio.c +++ b/src/southbridge/intel/lynxpoint/serialio.c @@ -140,10 +140,10 @@ pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
/* Find BAR0 and BAR1 */ - bar0 = find_resource(dev, PCI_BASE_ADDRESS_0); + bar0 = probe_resource(dev, PCI_BASE_ADDRESS_0); if (!bar0) return; - bar1 = find_resource(dev, PCI_BASE_ADDRESS_1); + bar1 = probe_resource(dev, PCI_BASE_ADDRESS_1); if (!bar1) return;
diff --git a/src/southbridge/intel/lynxpoint/smbus.c b/src/southbridge/intel/lynxpoint/smbus.c index 39003f6..f88bd2b 100644 --- a/src/southbridge/intel/lynxpoint/smbus.c +++ b/src/southbridge/intel/lynxpoint/smbus.c @@ -20,7 +20,7 @@ pci_write_config32(dev, 0x80, reg16);
/* Set Receive Slave Address */ - res = find_resource(dev, PCI_BASE_ADDRESS_4); + res = probe_resource(dev, PCI_BASE_ADDRESS_4); if (res) smbus_set_slave_addr(res->base, SMBUS_SLAVE_ADDR); }
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Patch Set 1: Code-Review+1
Nice find. Did some tool find it or your review?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG@12 PS3, Line 12: with `probe_resource` instead. Why not just remove the NULL checking and let things die? Many of those cases seem like the resource they are looking for should absolutely be there.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG@12 PS3, Line 12: with `probe_resource` instead.
Why not just remove the NULL checking and let things die? Many of those cases seem like the resource […]
I just replaced all uses blindly. I didn't stop to check if they were absolutely necessary or not
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG@12 PS3, Line 12: with `probe_resource` instead.
I just replaced all uses blindly. […]
OK. I suspect people will be copying things blindly, but I guess we can always clean them up later.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42802 )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42802/3//COMMIT_MSG@12 PS3, Line 12: with `probe_resource` instead.
Why not just remove the NULL checking and let things die? Many of those cases seem like the resource […]
It's unlikely to ever run into the case where no resource is assigned, so I don't have a strong opinion about that.
If you want an always booting platform, you'd rather not use find_resource. On a QA system you likely want to die as boot failures are easy to spot.
Maybe we can keep using probe_resource() and NULL checks for general code usage, but add a Kconfig to die if a resource isn't found.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42802?usp=email )
Change subject: sb/intel: Fix bad uses of `find_resource` ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Martin L Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)