Change in coreboot[master]: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree
Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree
With commit 8950cfb66f8f1fd4b047fbef2347134be0aeacec (soc/intel: Use config_of()) a new function was introduced to get the pointer to the devicetree-config for a given device. This function has now discovered a bug in the IOU PCI-driver of FSP based Broadwell-DE implementation. This implementation assigned the value to the config_t pointer in the way 'const config_t *config = dev->chip_info;' without checking that the dev->chip_info value is not NULL. On mainboards touched in this patch the static devicetree does not have entries for the PCI devices handled by this IOU driver. This leads to a NULL-pointer for dev->chip_info and hence the driver dereferences this NULL-pointer.
With the patch mentioned above the config_of() function now performs the NULL-pointer check and enters a devtree_die(); call which leads to a non booting mainboard.
I have checked on mc_bdx1 mainboard which value was used for config->pcie_compltoval if the NULL-pointer is derefferenced and got the following result:
IOU: config=00000000, config->pcie_compltoval=0
So it will be the same if we add this PCI devices to devicetree and do not provide a register entry for pcie_compltoval (defaults to 0 then).
This patch adds now the missing PCI devices to the mainboards in question which prevents this boards now from calling devtree_die().
Change-Id: I00a2e7ac14cf006718de55aa7a843442892d6280 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/mainboard/facebook/watson/devicetree.cb M src/mainboard/intel/camelbackmountain_fsp/devicetree.cb M src/mainboard/ocp/monolake/devicetree.cb M src/mainboard/siemens/mc_bdx1/devicetree.cb 4 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/34540/1
diff --git a/src/mainboard/facebook/watson/devicetree.cb b/src/mainboard/facebook/watson/devicetree.cb index 92fe9c3..1cff425 100644 --- a/src/mainboard/facebook/watson/devicetree.cb +++ b/src/mainboard/facebook/watson/devicetree.cb @@ -4,6 +4,8 @@ end device domain 0 on device pci 00.0 on end # SoC router + device pci 01.0 on end # IOU 2 + device pci 03.0 on end # IOU 1 device pci 14.0 on end # xHCI Controller device pci 19.0 on end # Gigabit LAN Controller device pci 1d.0 on end # EHCI Controller diff --git a/src/mainboard/intel/camelbackmountain_fsp/devicetree.cb b/src/mainboard/intel/camelbackmountain_fsp/devicetree.cb index 30d99c2..7d2c3dd 100644 --- a/src/mainboard/intel/camelbackmountain_fsp/devicetree.cb +++ b/src/mainboard/intel/camelbackmountain_fsp/devicetree.cb @@ -4,6 +4,8 @@ end device domain 0 on device pci 00.0 on end # SoC router + device pci 01.0 on end # IOU 2 + device pci 03.0 on end # IOU 1 device pci 14.0 on end # xHCI Controller device pci 19.0 on end # Gigabit LAN Controller device pci 1d.0 on end # EHCI Controller diff --git a/src/mainboard/ocp/monolake/devicetree.cb b/src/mainboard/ocp/monolake/devicetree.cb index 2d91b73..6fdae0c 100644 --- a/src/mainboard/ocp/monolake/devicetree.cb +++ b/src/mainboard/ocp/monolake/devicetree.cb @@ -4,6 +4,8 @@ end device domain 0 on device pci 00.0 on end # SoC router + device pci 01.0 on end # IOU 2 + device pci 03.0 on end # IOU 1 device pci 14.0 on end # xHCI Controller device pci 19.0 on end # Gigabit LAN Controller device pci 1d.0 on end # EHCI Controller diff --git a/src/mainboard/siemens/mc_bdx1/devicetree.cb b/src/mainboard/siemens/mc_bdx1/devicetree.cb index fd7d83f..6b4d192 100644 --- a/src/mainboard/siemens/mc_bdx1/devicetree.cb +++ b/src/mainboard/siemens/mc_bdx1/devicetree.cb @@ -4,6 +4,8 @@ end device domain 0 on device pci 00.0 on end # SoC router + device pci 01.0 on end # IOU 2 + device pci 03.0 on end # IOU 1 device pci 14.0 on end # xHCI Controller device pci 19.0 on end # Gigabit LAN Controller device pci 1d.0 on end # EHCI Controller
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Patch Set 1: Code-Review+2
Ok.. I'll see if we can improve config_of() somehow.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
Ok.. I'll see if we can improve config_of() somehow.
I find your patch usefull as it has discovered this bug. Dereferencing a NULL-pointer is always a bad Idea and we were not aware of that. So your patch was and still is wellcome. The only thing we need to think about if the die-call is the best way to handle this error.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG@9 PS1, Line 9: With commit 8950cfb66f8f1fd4b047fbef2347134be0aeacec : (soc/intel: Use config_of()) a new function was introduced to get the : pointer to the devicetree-config for a given device. Maybe:
Commit 8950cfb6 (soc/intel: Use config_of()) adds a new function to get the pointer to the devicetree config for a given device.
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG@17 PS1, Line 17: NULL-pointer I’d spell it without -.
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG@25 PS1, Line 25: derefferenced dereferenced
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Patch Set 1: -Code-Review
Would CB:34547 work for you?
I think it's a bad approach to require updates on other static devicetree files when you add code using config_of(). It appears for soc/ we could inherit .chip_info from 0:0.0 to auto-discoverable PCI devices that were not present in static tree. For split directory cpu/nb/sb platforms it would be a bit more tricky, but possible.
Hello Kyösti Mälkki, Patrick Rudolph, Paul Menzel, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34540
to look at the new patch set (#2).
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree
Commit 8950cfb66f8f1fd4b047fbef2347134be0aeacec (soc/intel: Use config_of()) adds a new function to get the pointer to the devicetree config for a given device. This function has now discovered a bug in the IOU PCI-driver of FSP based Broadwell-DE implementation. This implementation assigned the value to the config_t pointer in the way 'const config_t *config = dev->chip_info;' without checking that the dev->chip_info value is not NULL. On mainboards touched in this patch the static devicetree does not have entries for the PCI devices handled by this IOU driver. This leads to a NULL-pointer for dev->chip_info and hence the driver dereferences this NULL pointer.
With the patch mentioned above the config_of() function now performs the NULL-pointer check and enters a devtree_die(); call which leads to a non booting mainboard.
I have checked on mc_bdx1 mainboard which value was used for config->pcie_compltoval if the NULL-pointer is dereferenced and got the following result:
IOU: config=00000000, config->pcie_compltoval=0
So it will be the same if we add this PCI devices to devicetree and do not provide a register entry for pcie_compltoval (defaults to 0 then).
This patch adds now the missing PCI devices to the mainboards in question which prevents this boards now from calling devtree_die().
Change-Id: I00a2e7ac14cf006718de55aa7a843442892d6280 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/mainboard/facebook/watson/devicetree.cb M src/mainboard/intel/camelbackmountain_fsp/devicetree.cb M src/mainboard/ocp/monolake/devicetree.cb M src/mainboard/siemens/mc_bdx1/devicetree.cb 4 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/34540/2
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG@9 PS1, Line 9: With commit 8950cfb66f8f1fd4b047fbef2347134be0aeacec : (soc/intel: Use config_of()) a new function was introduced to get the : pointer to the devicetree-config for a given device.
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG@17 PS1, Line 17: NULL-pointer
I’d spell it without -.
Done
https://review.coreboot.org/c/coreboot/+/34540/1//COMMIT_MSG@25 PS1, Line 25: derefferenced
dereferenced
Done
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Patch Set 2:
Patch Set 1: -Code-Review
Would CB:34547 work for you?
I think it's a bad approach to require updates on other static devicetree files when you add code using config_of(). It appears for soc/ we could inherit .chip_info from 0:0.0 to auto-discoverable PCI devices that were not present in static tree. For split directory cpu/nb/sb platforms it would be a bit more tricky, but possible.
Will try and report. Thanks Kyösti.
Werner Zeh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34540 )
Change subject: src/mainboard: Add PCI device for IUO1 and IOU2 to devicetree ......................................................................
Abandoned
This issue was fixed differently in https://review.coreboot.org/c/coreboot/+/34547
-
Kyösti Mälkki (Code Review) -
Paul Menzel (Code Review)
-
Werner Zeh (Code Review)