Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30879
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
sb/intel: Check for NULL-return of pcidev_on_root()
In these cases we have to expect a NULL pointer because the IGD device 0:2.0 may be disabled.
Change-Id: I1bab8fa3a82daca71d03453315cdd69d8951fc24 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801ix/lpc.c M src/southbridge/intel/i82801jx/lpc.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c 6 files changed, 27 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/30879/1
diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index e13c666..4de4f16 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -719,8 +719,10 @@ gnvs->mpen = 1; /* Enable Multi Processing */ gnvs->pcnt = dev_count_cpu();
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
#if IS_ENABLED(CONFIG_CHROMEOS) chromeos_init_chromeos_acpi(&(gnvs->chromeos)); diff --git a/src/southbridge/intel/i82801gx/lpc.c b/src/southbridge/intel/i82801gx/lpc.c index e8cfc74..b20b2aa 100644 --- a/src/southbridge/intel/i82801gx/lpc.c +++ b/src/southbridge/intel/i82801gx/lpc.c @@ -705,8 +705,10 @@
acpi_create_gnvs(gnvs);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/i82801ix/lpc.c b/src/southbridge/intel/i82801ix/lpc.c index dd37a0b..e18f616 100644 --- a/src/southbridge/intel/i82801ix/lpc.c +++ b/src/southbridge/intel/i82801ix/lpc.c @@ -545,8 +545,10 @@ memset(gnvs, 0, sizeof(*gnvs)); acpi_create_gnvs(gnvs);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/i82801jx/lpc.c b/src/southbridge/intel/i82801jx/lpc.c index 0f82f90..3f7095c 100644 --- a/src/southbridge/intel/i82801jx/lpc.c +++ b/src/southbridge/intel/i82801jx/lpc.c @@ -707,8 +707,10 @@ memset(gnvs, 0, sizeof(*gnvs)); acpi_create_gnvs(gnvs);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/ibexpeak/lpc.c b/src/southbridge/intel/ibexpeak/lpc.c index 3358633..a96b7ff 100644 --- a/src/southbridge/intel/ibexpeak/lpc.c +++ b/src/southbridge/intel/ibexpeak/lpc.c @@ -632,8 +632,11 @@ gnvs->apic = 1; gnvs->mpen = 1; /* Enable Multi Processing */ gnvs->pcnt = dev_count_cpu(); - gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index 474c7df..53e6da1 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -456,7 +456,8 @@ RCBA32_AND_OR(0x2614, 0x8bffffff, 0x0a206500);
/* Check for LPT-LP B2 stepping and 0:31.0@0xFA > 4 */ - if (pci_read_config8(pcidev_on_root(2, 0), 0x8) >= 0x0b) + const struct device *const gma = pcidev_on_root(2, 0); + if (gma && pci_read_config8(gma, 0x8) >= 0x0b) RCBA32_OR(0x2614, (1 << 26));
RCBA32_OR(0x900, 0x0000031f); @@ -760,8 +761,10 @@ /* Update the mem console pointer. */ gnvs->cbmc = (u32)cbmem_find(CBMEM_ID_CONSOLE);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL);
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30879 )
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30879/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30879/1//COMMIT_MSG@10 PS1, Line 10: 0:2.0 may be disabled. Maybe comment it removes some ACPI data when IGD is disabled? If it does so...
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30879
to look at the new patch set (#2).
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
sb/intel: Check for NULL-return of pcidev_on_root()
In these cases we have to expect a NULL pointer because the IGD device 0:2.0 may be disabled.
Change-Id: I1bab8fa3a82daca71d03453315cdd69d8951fc24 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801ix/lpc.c M src/southbridge/intel/i82801jx/lpc.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c 6 files changed, 27 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/30879/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30879 )
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
Patch Set 2: Code-Review+2
LGTM code-wise.
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30879
to look at the new patch set (#3).
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
sb/intel: Check for NULL-return of pcidev_on_root()
In these cases we have to expect a NULL pointer because the IGD device 0:2.0 may be disabled.
The behaviour still differs from using dev_find_slot(), which may return a disabled device. Though, if you'd try to read its config space you'd only read garbage (0xff) and in cases where we filled ACPI data with devicetree information, the information shouldn't be interpreted by the OS because of the disabled device.
Change-Id: I1bab8fa3a82daca71d03453315cdd69d8951fc24 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801ix/lpc.c M src/southbridge/intel/i82801jx/lpc.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c 6 files changed, 27 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/30879/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30879 )
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30879 )
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/30879/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30879/1//COMMIT_MSG@10 PS1, Line 10: 0:2.0 may be disabled.
Maybe comment it removes some ACPI data when IGD is disabled? If it does so...
Done
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30879 )
Change subject: sb/intel: Check for NULL-return of pcidev_on_root() ......................................................................
sb/intel: Check for NULL-return of pcidev_on_root()
In these cases we have to expect a NULL pointer because the IGD device 0:2.0 may be disabled.
The behaviour still differs from using dev_find_slot(), which may return a disabled device. Though, if you'd try to read its config space you'd only read garbage (0xff) and in cases where we filled ACPI data with devicetree information, the information shouldn't be interpreted by the OS because of the disabled device.
Change-Id: I1bab8fa3a82daca71d03453315cdd69d8951fc24 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/30879 Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801ix/lpc.c M src/southbridge/intel/i82801jx/lpc.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c 6 files changed, 27 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index e13c666..4de4f16 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -719,8 +719,10 @@ gnvs->mpen = 1; /* Enable Multi Processing */ gnvs->pcnt = dev_count_cpu();
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
#if IS_ENABLED(CONFIG_CHROMEOS) chromeos_init_chromeos_acpi(&(gnvs->chromeos)); diff --git a/src/southbridge/intel/i82801gx/lpc.c b/src/southbridge/intel/i82801gx/lpc.c index e8cfc74..b20b2aa 100644 --- a/src/southbridge/intel/i82801gx/lpc.c +++ b/src/southbridge/intel/i82801gx/lpc.c @@ -705,8 +705,10 @@
acpi_create_gnvs(gnvs);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/i82801ix/lpc.c b/src/southbridge/intel/i82801ix/lpc.c index dd37a0b..e18f616 100644 --- a/src/southbridge/intel/i82801ix/lpc.c +++ b/src/southbridge/intel/i82801ix/lpc.c @@ -545,8 +545,10 @@ memset(gnvs, 0, sizeof(*gnvs)); acpi_create_gnvs(gnvs);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/i82801jx/lpc.c b/src/southbridge/intel/i82801jx/lpc.c index 0f82f90..3f7095c 100644 --- a/src/southbridge/intel/i82801jx/lpc.c +++ b/src/southbridge/intel/i82801jx/lpc.c @@ -707,8 +707,10 @@ memset(gnvs, 0, sizeof(*gnvs)); acpi_create_gnvs(gnvs);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/ibexpeak/lpc.c b/src/southbridge/intel/ibexpeak/lpc.c index 3358633..a96b7ff 100644 --- a/src/southbridge/intel/ibexpeak/lpc.c +++ b/src/southbridge/intel/ibexpeak/lpc.c @@ -632,8 +632,11 @@ gnvs->apic = 1; gnvs->mpen = 1; /* Enable Multi Processing */ gnvs->pcnt = dev_count_cpu(); - gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL); diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index 474c7df..59074e0 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -456,7 +456,8 @@ RCBA32_AND_OR(0x2614, 0x8bffffff, 0x0a206500);
/* Check for LPT-LP B2 stepping and 0:31.0@0xFA > 4 */ - if (pci_read_config8(pcidev_on_root(2, 0), 0x8) >= 0x0b) + struct device *const gma = pcidev_on_root(2, 0); + if (gma && pci_read_config8(gma, 0x8) >= 0x0b) RCBA32_OR(0x2614, (1 << 26));
RCBA32_OR(0x900, 0x0000031f); @@ -760,8 +761,10 @@ /* Update the mem console pointer. */ gnvs->cbmc = (u32)cbmem_find(CBMEM_ID_CONSOLE);
- gnvs->ndid = gfx->ndid; - memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + if (gfx) { + gnvs->ndid = gfx->ndid; + memcpy(gnvs->did, gfx->did, sizeof(gnvs->did)); + }
/* And tell SMI about it */ smm_setup_structures(gnvs, NULL, NULL);
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)