Change in coreboot[master]: soc/intel: Fix regression on pcidev_

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression on pcidev_
......................................................................

soc/intel: Fix regression on pcidev_

Fix regression with commit
  903b40a soc/intel: Replace uses of dev_find_slot()

Platforms where FSP hides PCI devices before enumeration
may halt with error message 'PCI: dev is NULL!'.

The workaround here is to print an error message revealing
the faulty source code function and revert to old behaviour
of dev_find_slot().

Change-Id: I5eab3e7f1993b686103eaa257aacda379dc259fa
Signed-off-by: Kyösti Mälkki 
---
M src/device/device_const.c
M src/include/device/device.h
M src/soc/intel/apollolake/include/soc/pci_devs.h
M src/soc/intel/broadwell/include/soc/pci_devs.h
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/denverton_ns/include/soc/pci_devs.h
M src/soc/intel/icelake/include/soc/pci_devs.h
M src/soc/intel/skylake/include/soc/pci_devs.h
8 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34285/1

diff --git a/src/device/device_const.c b/src/device/device_const.c
index c1b0b06..05dfb30 100644
--- a/src/device/device_const.c
+++ b/src/device/device_const.c
@@ -234,6 +234,16 @@
    return pcidev_path_on_root(PCI_DEVFN(dev, fn));
 }

+DEVTREE_CONST struct device *pcidev_debug_path_on_root(pci_devfn_t devfn, const char *func)
+{
+	DEVTREE_CONST struct device *dev = pcidev_path_on_root(devfn);
+	if (dev)
+		return dev;
+
+	printk(BIOS_ERR, "BUG: %s requests hidden 00:%02x.%d\n", func, devfn >> 3, devfn & 7);
+	return dev_find_slot(0, devfn);
+}
+
 /**
  * Given an SMBus bus and a device number, find the device structure.
  *
diff --git a/src/include/device/device.h b/src/include/device/device.h
index 007c51f..12c258e 100644
--- a/src/include/device/device.h
+++ b/src/include/device/device.h
@@ -275,8 +275,6 @@
 void tolm_test(void *gp, struct device *dev, struct resource *new);
 u32 find_pci_tolm(struct bus *bus);

-DEVTREE_CONST struct device *dev_find_slot(unsigned int bus,
-						unsigned int devfn);
 DEVTREE_CONST struct device *dev_find_next_pci_device(
    			DEVTREE_CONST struct device *previous_dev);
 DEVTREE_CONST struct device *dev_find_slot_on_smbus(unsigned int bus,
@@ -292,6 +290,10 @@
 DEVTREE_CONST struct device *pcidev_on_root(uint8_t dev, uint8_t fn);
 DEVTREE_CONST struct bus *pci_root_bus(void);

+/* To be deprecated, avoid using. */
+DEVTREE_CONST struct device *dev_find_slot(unsigned int bus, unsigned int devfn);
+DEVTREE_CONST struct device *pcidev_debug_path_on_root(pci_devfn_t devfn, const char *func);
+
 void scan_smbus(struct device *bus);
 void scan_generic_bus(struct device *bus);
 void scan_static_bus(struct device *bus);
diff --git a/src/soc/intel/apollolake/include/soc/pci_devs.h b/src/soc/intel/apollolake/include/soc/pci_devs.h
index a334f63..c5eaf4c 100644
--- a/src/soc/intel/apollolake/include/soc/pci_devs.h
+++ b/src/soc/intel/apollolake/include/soc/pci_devs.h
@@ -22,8 +22,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
diff --git a/src/soc/intel/broadwell/include/soc/pci_devs.h b/src/soc/intel/broadwell/include/soc/pci_devs.h
index 522e3eb..ae3e08f 100644
--- a/src/soc/intel/broadwell/include/soc/pci_devs.h
+++ b/src/soc/intel/broadwell/include/soc/pci_devs.h
@@ -25,8 +25,8 @@
 #else
 #include <device/device.h>
 #include <device/pci_def.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #endif

/* System Agent Devices */
diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h
index 1bc028f..33814b0 100644
--- a/src/soc/intel/cannonlake/include/soc/pci_devs.h
+++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h
@@ -24,8 +24,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
diff --git a/src/soc/intel/denverton_ns/include/soc/pci_devs.h b/src/soc/intel/denverton_ns/include/soc/pci_devs.h
index 303ba67..27f9e35 100644
--- a/src/soc/intel/denverton_ns/include/soc/pci_devs.h
+++ b/src/soc/intel/denverton_ns/include/soc/pci_devs.h
@@ -27,8 +27,8 @@
 #if ENV_RAMSTAGE
 #include <device/device.h>
 #include <device/pci_def.h>
-#define _SA_DEV(slot) pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func) pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot) pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func) pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot) PCI_DEV(0, SA_DEV_SLOT_##slot, 0)
 #define _PCH_DEV(slot, func) PCI_DEV(0, PCH_DEV_SLOT_##slot, func)
diff --git a/src/soc/intel/icelake/include/soc/pci_devs.h b/src/soc/intel/icelake/include/soc/pci_devs.h
index 1348f23b..0eddee2 100644
--- a/src/soc/intel/icelake/include/soc/pci_devs.h
+++ b/src/soc/intel/icelake/include/soc/pci_devs.h
@@ -23,8 +23,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
diff --git a/src/soc/intel/skylake/include/soc/pci_devs.h b/src/soc/intel/skylake/include/soc/pci_devs.h
index 5acaaeb..d25942f 100644
--- a/src/soc/intel/skylake/include/soc/pci_devs.h
+++ b/src/soc/intel/skylake/include/soc/pci_devs.h
@@ -24,8 +24,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_debug_path_on_root(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_debug_path_on_root(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
@@ -39,7 +39,7 @@

#define SA_DEV_SLOT_PEG		0x01
 #define  SA_DEVFN_PEG(func)	PCI_DEVFN(SA_DEV_SLOT_PEG, func)
-#define  SA_DEV_PEG(func)	pcidev_path_on_root(SA_DEVFN_PEG(func))
+#define  SA_DEV_PEG(func)	pcidev_debug_path_on_root(SA_DEVFN_PEG(func), __func__)
 #define  SA_DEV_PEG0		SA_DEV_PEG(0)
 #define  SA_DEV_PEG1		SA_DEV_PEG(1)
 #define  SA_DEV_PEG2		SA_DEV_PEG(2)

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Vanny E, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34285

to look at the new patch set (#2).

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

soc/intel: Fix regression with hidden PCI devices

Fix regression with commit
  903b40a soc/intel: Replace uses of dev_find_slot()

Platforms where FSP hides PCI devices before enumeration
may halt with error message 'PCI: dev is NULL!'.

The workaround here is to print an error message revealing
the faulty source code function and revert to old behaviour
of dev_find_slot().

Change-Id: I5eab3e7f1993b686103eaa257aacda379dc259fa
Signed-off-by: Kyösti Mälkki 
---
M src/device/device_const.c
M src/include/device/device.h
M src/soc/intel/apollolake/include/soc/pci_devs.h
M src/soc/intel/broadwell/include/soc/pci_devs.h
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/denverton_ns/include/soc/pci_devs.h
M src/soc/intel/icelake/include/soc/pci_devs.h
M src/soc/intel/skylake/include/soc/pci_devs.h
8 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34285/2

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 2:

Not tested, Maxim ?

Hopefully noisy enough to annoy people enough to add some proper NULL checks.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Vanny E, Maxim Polyakov, build bot (Jenkins), Nico Huber, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34285

to look at the new patch set (#3).

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

soc/intel: Fix regression with hidden PCI devices

Fix regression with commit
  903b40a soc/intel: Replace uses of dev_find_slot()

Platforms where FSP hides PCI devices before enumeration
may halt with error message 'PCI: dev is NULL!'.

The workaround here is to print an error message revealing
the faulty source code function and revert to old behaviour
of dev_find_slot().

Change-Id: I5eab3e7f1993b686103eaa257aacda379dc259fa
Signed-off-by: Kyösti Mälkki 
---
M src/device/device_const.c
M src/include/device/device.h
M src/soc/intel/apollolake/include/soc/pci_devs.h
M src/soc/intel/broadwell/include/soc/pci_devs.h
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/denverton_ns/include/soc/pci_devs.h
M src/soc/intel/icelake/include/soc/pci_devs.h
M src/soc/intel/skylake/include/soc/pci_devs.h
8 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34285/3

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3:
> Patch Set 2:
>
> Not tested, Maxim ?

Thanks for this patch. Now it works well
I can see such devices in the console:

$ grep -rn "BUG:" 34285-console.log

34:BUG: soc_peg_init_params requests hidden 00:01.1
35:BUG: soc_peg_init_params requests hidden 00:01.2
1132:BUG: me_read_config32 requests hidden 00:16.0
1370:BUG: pch_log_rp_wake_source requests hidden 00:1c.1
1371:BUG: pch_log_rp_wake_source requests hidden 00:1c.2
1372:BUG: pch_log_rp_wake_source requests hidden 00:1c.3
1373:BUG: pch_log_rp_wake_source requests hidden 00:1c.4
1374:BUG: pch_log_rp_wake_source requests hidden 00:1c.7
1375:BUG: pch_log_rp_wake_source requests hidden 00:1d.0
1376:BUG: pch_log_rp_wake_source requests hidden 00:1d.1
1377:BUG: pch_log_rp_wake_source requests hidden 00:1d.2
1378:BUG: pch_log_rp_wake_source requests hidden 00:1d.3
1638:BUG: me_read_config32 requests hidden 00:16.0
1639:BUG: me_read_config32 requests hidden 00:16.0
1640:BUG: me_read_config32 requests hidden 00:16.0
1641:BUG: me_read_config32 requests hidden 00:16.0
1645:BUG: me_read_config32 requests hidden 00:16.0
1647:BUG: me_read_config32 requests hidden 00:16.0
1675:BUG: p2sb_get_device requests hidden 00:1f.1
1676:BUG: p2sb_get_device requests hidden 00:1f.1
1677:BUG: p2sb_get_device requests hidden 00:1f.1
1678:BUG: p2sb_get_device requests hidden 00:1f.1
1679:BUG: p2sb_get_device requests hidden 00:1f.1
1680:BUG: p2sb_get_device requests hidden 00:1f.1
1681:BUG: p2sb_get_device requests hidden 00:1f.1
1682:BUG: p2sb_get_device requests hidden 00:1f.1
1683:BUG: p2sb_get_device requests hidden 00:1f.1
1684:BUG: p2sb_get_device requests hidden 00:1f.1
1685:BUG: p2sb_get_device requests hidden 00:1f.1
1686:BUG: p2sb_get_device requests hidden 00:1f.1
1687:BUG: p2sb_get_device requests hidden 00:1f.1
1688:BUG: p2sb_get_device requests hidden 00:1f.1
1689:BUG: p2sb_get_device requests hidden 00:1f.1
1690:BUG: p2sb_get_device requests hidden 00:1f.1
1691:BUG: p2sb_get_device requests hidden 00:1f.1
1692:BUG: p2sb_get_device requests hidden 00:1f.1
1693:BUG: p2sb_get_device requests hidden 00:1f.1
1694:BUG: p2sb_get_device requests hidden 00:1f.1
1695:BUG: p2sb_get_device requests hidden 00:1f.1
1696:BUG: p2sb_get_device requests hidden 00:1f.1
1697:BUG: p2sb_get_device requests hidden 00:1f.1
1698:BUG: p2sb_get_device requests hidden 00:1f.1

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3:
> Patch Set 3:
> > Patch Set 2:
> >
> > Not tested, Maxim ?
>
> Thanks for this patch. Now it works well
> I can see such devices in the console:
>
> $ grep -rn "BUG:" 34285-console.log
>
> 34:BUG: soc_peg_init_params requests hidden 00:01.1
> 35:BUG: soc_peg_init_params requests hidden 00:01.2

OK. While annoying, I think this is better than reverting the offending commit?

With the output here I think it is easy to track down the cases where we would try to access PCI devices that would not respond to configuration cycles.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3: Code-Review+1

I really like this idea. Can't test it, though.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

Just one tiny nit.

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c@2... 
PS3, Line 243: d
%u?

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 3:
> > Patch Set 2:
> >
> > Not tested, Maxim ?
>
> Thanks for this patch. Now it works well
> I can see such devices in the console:
>
> $ grep -rn "BUG:" 34285-console.log
>
> 34:BUG: soc_peg_init_params requests hidden 00:01.1
> 35:BUG: soc_peg_init_params requests hidden 00:01.2
> 1132:BUG: me_read_config32 requests hidden 00:16.0
> 1370:BUG: pch_log_rp_wake_source requests hidden 00:1c.1
> 1371:BUG: pch_log_rp_wake_source requests hidden 00:1c.2
> 1372:BUG: pch_log_rp_wake_source requests hidden 00:1c.3
> 1373:BUG: pch_log_rp_wake_source requests hidden 00:1c.4
> 1374:BUG: pch_log_rp_wake_source requests hidden 00:1c.7
> 1375:BUG: pch_log_rp_wake_source requests hidden 00:1d.0
> 1376:BUG: pch_log_rp_wake_source requests hidden 00:1d.1
> 1377:BUG: pch_log_rp_wake_source requests hidden 00:1d.2
> 1378:BUG: pch_log_rp_wake_source requests hidden 00:1d.3
> 1638:BUG: me_read_config32 requests hidden 00:16.0
> 1639:BUG: me_read_config32 requests hidden 00:16.0
> 1640:BUG: me_read_config32 requests hidden 00:16.0
> 1641:BUG: me_read_config32 requests hidden 00:16.0
> 1645:BUG: me_read_config32 requests hidden 00:16.0
> 1647:BUG: me_read_config32 requests hidden 00:16.0
> 1675:BUG: p2sb_get_device requests hidden 00:1f.1
> 1676:BUG: p2sb_get_device requests hidden 00:1f.1
> 1677:BUG: p2sb_get_device requests hidden 00:1f.1
> 1678:BUG: p2sb_get_device requests hidden 00:1f.1
> 1679:BUG: p2sb_get_device requests hidden 00:1f.1
> 1680:BUG: p2sb_get_device requests hidden 00:1f.1
> 1681:BUG: p2sb_get_device requests hidden 00:1f.1
> 1682:BUG: p2sb_get_device requests hidden 00:1f.1
> 1683:BUG: p2sb_get_device requests hidden 00:1f.1
> 1684:BUG: p2sb_get_device requests hidden 00:1f.1
> 1685:BUG: p2sb_get_device requests hidden 00:1f.1
> 1686:BUG: p2sb_get_device requests hidden 00:1f.1
> 1687:BUG: p2sb_get_device requests hidden 00:1f.1
> 1688:BUG: p2sb_get_device requests hidden 00:1f.1
> 1689:BUG: p2sb_get_device requests hidden 00:1f.1
> 1690:BUG: p2sb_get_device requests hidden 00:1f.1
> 1691:BUG: p2sb_get_device requests hidden 00:1f.1
> 1692:BUG: p2sb_get_device requests hidden 00:1f.1
> 1693:BUG: p2sb_get_device requests hidden 00:1f.1
> 1694:BUG: p2sb_get_device requests hidden 00:1f.1
> 1695:BUG: p2sb_get_device requests hidden 00:1f.1
> 1696:BUG: p2sb_get_device requests hidden 00:1f.1
> 1697:BUG: p2sb_get_device requests hidden 00:1f.1
> 1698:BUG: p2sb_get_device requests hidden 00:1f.1

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c@2... 
PS3, Line 243: BUG: %s requests hidden 00:%02x.%d\
Not everything that is reported will truly be a bug. In the output pasted by Maxim, following calls are fine since they check against !dev:

soc_peg_init_params
pch_log_rp_wake_source

On the other hand this is a true bug: me_read_config32. It does not check for NULL. But I think that would be a common problem. I see a lot of uses of PCH_DEV_* macro being passed as an argument to some function without any check.

On the other hand, this is another problem: p2sb_get_device. Here the device seems to be hidden and the functions seem to be un-hiding it. If you plan to get rid of dev_find_slot() completely, what is your plan on supporting such devices?

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c@2... 
PS3, Line 243: BUG: %s requests hidden 00:%02x.%d\
> Not everything that is reported will truly be a bug. In the output pasted by Maxim, following calls are fine since they check against !dev:
>
> soc_peg_init_params
> pch_log_rp_wake_source

I'll make them explicitly call pcidev_path_on_root() instead. Maybe I mentioned earlier that I find soc/pci_devs.h most disgusting use of preprocessor. In one context macro expands to constant value, in other it's a function returning pointer...
> On the other hand this is a true bug: me_read_config32. It does not check for NULL. But I think that would be a common problem. I see a lot of uses of PCH_DEV_* macro being passed as an argument to some function without any check.

The log should annoy one enough to add the NULL checks where necessary.
> On the other hand, this is another problem: p2sb_get_device. Here the device seems to be hidden and the functions seem to be un-hiding it. If you plan to get rid of dev_find_slot() completely, what is your plan on supporting such devices?

Awww.. yuck. So "hiding" makes the device not respond to PCI ID register reads but *some* configuration registers can be accessed and written. Ever heard of PCI Specifications, Intel?

Well I'll probably have to rename dev_find_slot() then and leave it available.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3:

Also we have pci_s_read_config() that takes BDF as argument, no need for device node pointer. I can use that on code like p2sb_hide() and p2sb_unhide():

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 3:
>
> Also we have pci_s_read_config() that takes BDF as argument, no need for device node pointer. I can use that on code like p2sb_hide() and p2sb_unhide():

Why not do it for all devices that are currently accessed using PCH_DEV_* i.e. just use _s_ variant to pass in PCH_DEVFN_* and get rid of all PCH_DEV_* completely.

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/3/src/device/device_const.c@2... 
PS3, Line 243: BUG: %s requests hidden 00:%02x.%d\
> Maybe I mentioned earlier that I find soc/pci_devs.h most disgusting use of preprocessor.

Me too!

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Vanny E, Angel Pons, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber, Furquan Shaikh, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34285

to look at the new patch set (#4).

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

soc/intel: Fix regression with hidden PCI devices

Fix regression with commit
  903b40a soc/intel: Replace uses of dev_find_slot()

Platforms where FSP hides PCI devices before enumeration
may halt with error message 'PCI: dev is NULL!'.

The workaround here is to print an error message revealing
the faulty source code function and revert to old behaviour
of dev_find_slot().

Change-Id: I5eab3e7f1993b686103eaa257aacda379dc259fa
Signed-off-by: Kyösti Mälkki 
---
M src/device/device_const.c
M src/include/device/device.h
M src/soc/intel/apollolake/include/soc/pci_devs.h
M src/soc/intel/broadwell/include/soc/pci_devs.h
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/denverton_ns/include/soc/pci_devs.h
M src/soc/intel/icelake/include/soc/pci_devs.h
M src/soc/intel/skylake/include/soc/pci_devs.h
8 files changed, 27 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34285/4

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 4:
> Patch Set 3:
>
> (1 comment)
> > Patch Set 3:
> >
> > Also we have pci_s_read_config() that takes BDF as argument, no need for device node pointer. I can use that on code like p2sb_hide() and p2sb_unhide():
>
> Why not do it for all devices that are currently accessed using PCH_DEV_* i.e. just use _s_ variant to pass in PCH_DEVFN_* and get rid of all PCH_DEV_* completely.

IMO what p2sb_hide/unhide does is a specs violation, sending configuration cycles to a PCI device that does not respond to the PCI ID register query. Currently I am assuming the register with hide/unide bit is the only one hardware always decodes.

I would rather make that one an explicit quirk, complain in the logs (maybe BIOS_DEBUG, not BIOS_ERR) of having to do so, and maybe even make it require a "select PCI_QUIRK_SPECS_VIOLATION" in the platform configs. I did not check PCI space of that device, I bet there would be bunch of interesting bits for firmware, maybe even OS. Like SERR/PERR perhaps.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 4:

Adding some Intel folks to this.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 3:
> >
> > (1 comment)
> > > Patch Set 3:
> > >
> > > Also we have pci_s_read_config() that takes BDF as argument, no need for device node pointer. I can use that on code like p2sb_hide() and p2sb_unhide():
> >
> > Why not do it for all devices that are currently accessed using PCH_DEV_* i.e. just use _s_ variant to pass in PCH_DEVFN_* and get rid of all PCH_DEV_* completely.
>
> IMO what p2sb_hide/unhide does is a specs violation, sending configuration cycles to a PCI device that does not respond to the PCI ID register query. Currently I am assuming the register with hide/unide bit is the only one hardware always decodes.

The device responds back with all 1s. Basically, the EDS says that when the hide bit gets set, this device will read 1s for any PCI config read. All other transactions are unaffected. I am not sure if it is a violation of spec to return back all 1s. That is a special value which basically indicates no device. But, I am not a PCI spec expert.

IIRC, Linux kernel follows a similar way of doing this i.e. accessing P2SB device.
> I would rather make that one an explicit quirk, complain in the logs (maybe BIOS_DEBUG, not BIOS_ERR) of having to do so, and maybe even make it require a "select PCI_QUIRK_SPECS_VIOLATION" in the platform configs. I did not check PCI space of that device, I bet there would be bunch of interesting bits for firmware, maybe even OS. Like SERR/PERR perhaps.

What purpose would this "PCI_QUIRK_SPECS_VIOLATION" be used for?

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 4:
> The device responds back with all 1s. Basically, the EDS says that when the hide bit gets set, this device will read 1s for any PCI config read. All other transactions are unaffected. I am not sure if it is a violation of spec to return back all 1s. That is a special value which basically indicates no device. But, I am not a PCI spec expert.

Huh.. so  you can write but cannot read? Or did you mean "unaffected -> not decoded" ?

Old PCI 2.3 had this phrase: 6.1. Configuration Space Organization
"A device's Configuration Space must be accessible at all times, not just during system boot."

Could not find exact same in PCIe 3.0. So let's call it borderline compliant.
> IIRC, Linux kernel follows a similar way of doing this i.e. accessing P2SB device.

You mean kernel also does this unhide/hide thingy? Would that not imply that hiding devices in the first place was a quirk for firmware that was not able to allocate fixed addresses for critical BARs like ACPI / PM ?
> > I would rather make that one an explicit quirk, complain in the logs (maybe BIOS_DEBUG, not BIOS_ERR) of having to do so, and maybe even make it require a "select PCI_QUIRK_SPECS_VIOLATION" in the platform configs. I did not check PCI space of that device, I bet there would be bunch of interesting bits for firmware, maybe even OS. Like SERR/PERR perhaps.
>
> What purpose would this "PCI_QUIRK_SPECS_VIOLATION" be used for?

I would guard pcidev_path_on_root_debug() and/or dev_find_slot() behind this. So seeing that in the platform Kconfig would serve as a reminder that some ugly hack is in place and needs attention. We can call it DEBUG_PCI_TREE but I would like to select it for affected soc/intel.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Angel Pons, Aamir Bohra, Matt DeVillier, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh, Vanny E, Maxim Polyakov, Nico Huber, David Guckian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34285

to look at the new patch set (#5).

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

soc/intel: Fix regression with hidden PCI devices

Fix regression with commit
  903b40a soc/intel: Replace uses of dev_find_slot()

Platforms where FSP hides PCI devices before enumeration
may halt with error message 'PCI: dev is NULL!'.

The workaround here is to print an error message revealing
the faulty source code function and revert to old behaviour
of dev_find_slot().

Change-Id: I5eab3e7f1993b686103eaa257aacda379dc259fa
Signed-off-by: Kyösti Mälkki 
---
M src/device/device_const.c
M src/include/device/device.h
M src/soc/intel/apollolake/include/soc/pci_devs.h
M src/soc/intel/broadwell/include/soc/pci_devs.h
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/denverton_ns/include/soc/pci_devs.h
M src/soc/intel/icelake/include/soc/pci_devs.h
M src/soc/intel/skylake/include/soc/pci_devs.h
8 files changed, 29 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34285/5

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 5:
> > The device responds back with all 1s. Basically, the EDS says that when the hide bit gets set, this device will read 1s for any PCI config read. All other transactions are unaffected. I am not sure if it is a violation of spec to return back all 1s. That is a special value which basically indicates no device. But, I am not a PCI spec expert.
>
> Huh.. so  you can write but cannot read? Or did you mean "unaffected -> not decoded" ?

What I know so far about the P2SB hiding:

* The device is still active when hidden, obviously.
* All config read cycles are ignored when hidden.
* On SKL, KBL: Config write requests work on only one register, 32 bits, to unhide it.
* You are supposed to write only the second byte in the register, because you can't maintain the bits around otherwise.
* Unhiding only works as long as the P2SB uses BOOT_SAI. On later platforms, it's switched to POSTBOOT_SAI by FSP looooooooooooong before the system is booted.
* All public references assume that it's hidden because of broken ASL code that uses constants for (parts of?) the P2SB BAR (instead of easier, native ACPI means).
* I personally assume, most SMM code doesn't do better.
* It's the only way to discover the P2SB BAR, which is required for a lot of things. So everybody, including kernel developers, have to risk havoc by unhiding it. I've seen kernel code that doesn't lock properly, so another thread could detect the BAR, and would have to disable it (best case) because it conflicts with ACPI resources.

To sum it up: My impression is that Intel desperately tries to cover
up for broken software. And even use hardware means (SAI switch) to
do so (otherwise unhiding would allow to hack broken SMM code?).
Nobody can handle the added complexity, it seems. Best case scenario,
IMHO: coreboot gets control of all hiding/locking/SAI switching in
the future. Otherwise, I fear, we'll never see reliable firmware on
Intel systems again.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 5:
> On SKL, KBL: Config write requests work on only one register, 32 bits, to unhide it.

Sorry, I remembered this wrong. All config writes should work.
> Unhiding only works as long as the P2SB uses BOOT_SAI.

I'm not sure about this any more, either. But at least there
is something related that makes it impossible to unhide.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 5:
> Patch Set 4: 
> Huh.. so  you can write but cannot read? Or did you mean "unaffected -> not decoded" ?

That is my understanding i.e. you can write but cannot read.
> > What purpose would this "PCI_QUIRK_SPECS_VIOLATION" be used for?
>
> I would guard pcidev_path_on_root_debug() and/or dev_find_slot() behind this. So seeing that in the platform Kconfig would serve as a reminder that some ugly hack is in place and needs attention. We can call it DEBUG_PCI_TREE but I would like to select it for affected soc/intel.

Humm okay.. I think most recent Intel platforms behave this way i.e. P2SB gets hidden.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 5:
> Patch Set 5:
> > On SKL, KBL: Config write requests work on only one register, 32 bits, to unhide it.
>
> Sorry, I remembered this wrong. All config writes should work.

That is right. Writes should work.
> > Unhiding only works as long as the P2SB uses BOOT_SAI.
>
> I'm not sure about this any more, either. But at least there
> is something related that makes it impossible to unhide.

I think you are referring to endpoint mask locking: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > On SKL, KBL: Config write requests work on only one register, 32 bits, to unhide it.
> >
> > Sorry, I remembered this wrong. All config writes should work.
>
> That is right. Writes should work.
> > > Unhiding only works as long as the P2SB uses BOOT_SAI.
> >
> > I'm not sure about this any more, either. But at least there
> > is something related that makes it impossible to unhide.
>
> I think you are referring to endpoint mask locking: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...

No, but often confused. With the endpoint masks you can further restrict
what silicon blocks you can target via the P2SB, AIUI. I was referring
to something new regarding the hiding, I'm not supposed to talk about in
detail, so: see Cannonlake PCH BIOS Spec.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 5:

At this moment, I don't have better to offer. Submit and/or rebase as you find necessary, I may not do much coreboot reviews until 18th.

We need to modify the PCI enumeration step such that we flag hidden missing devices instead of dropping them outside the topology link structures. Only once we have that, can we avoid calling dev_find_slot().

This commit is also dependency to CB:34298 and CB:34299, which seem to receive some positive feedback. It's somewhat orthogonal work, but target is same; to clean up the device node references.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 6: Code-Review+1

IMHO looks good, but can't test.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 6: Code-Review+1

fixes boot on purism/librem_skl w/FSP 2.0

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 6: Code-Review+1
> Patch Set 3:
> > Patch Set 2:
> >
> > Not tested, Maxim ?
>
> Thanks for this patch. Now it works well

Tested on Asrock-h110m-dvs

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 6: Code-Review+1

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

soc/intel: Fix regression with hidden PCI devices

Fix regression with commit
  903b40a soc/intel: Replace uses of dev_find_slot()

Platforms where FSP hides PCI devices before enumeration
may halt with error message 'PCI: dev is NULL!'.

The workaround here is to print an error message revealing
the faulty source code function and revert to old behaviour
of dev_find_slot().

Change-Id: I5eab3e7f1993b686103eaa257aacda379dc259fa
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34285
Reviewed-by: Matt DeVillier 
Reviewed-by: Maxim Polyakov 
Reviewed-by: Angel Pons 
Reviewed-by: Christian Walter 
Tested-by: build bot (Jenkins) 
---
M src/device/device_const.c
M src/include/device/device.h
M src/soc/intel/apollolake/include/soc/pci_devs.h
M src/soc/intel/broadwell/include/soc/pci_devs.h
M src/soc/intel/cannonlake/include/soc/pci_devs.h
M src/soc/intel/denverton_ns/include/soc/pci_devs.h
M src/soc/intel/icelake/include/soc/pci_devs.h
M src/soc/intel/skylake/include/soc/pci_devs.h
8 files changed, 29 insertions(+), 15 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Matt DeVillier: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved
  Maxim Polyakov: Looks good to me, but someone else must approve
  Christian Walter: Looks good to me, but someone else must approve

diff --git a/src/device/device_const.c b/src/device/device_const.c
index c1b0b06..f2f0177 100644
--- a/src/device/device_const.c
+++ b/src/device/device_const.c
@@ -234,6 +234,18 @@
    return pcidev_path_on_root(PCI_DEVFN(dev, fn));
 }

+DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
+{
+	DEVTREE_CONST struct device *dev = pcidev_path_on_root(devfn);
+	if (dev)
+		return dev;
+
+	printk(BIOS_ERR, "BUG: %s requests hidden 00:%02x.%u\n", func, devfn >> 3, devfn & 7);
+
+	/* FIXME: This can return wrong device. */
+	return dev_find_slot(0, devfn);
+}
+
 /**
  * Given an SMBus bus and a device number, find the device structure.
  *
diff --git a/src/include/device/device.h b/src/include/device/device.h
index 676da65..d6f80ce 100644
--- a/src/include/device/device.h
+++ b/src/include/device/device.h
@@ -275,8 +275,6 @@
 void tolm_test(void *gp, struct device *dev, struct resource *new);
 u32 find_pci_tolm(struct bus *bus);

-DEVTREE_CONST struct device *dev_find_slot(unsigned int bus,
-						unsigned int devfn);
 DEVTREE_CONST struct device *dev_find_next_pci_device(
    			DEVTREE_CONST struct device *previous_dev);
 DEVTREE_CONST struct device *dev_find_slot_on_smbus(unsigned int bus,
@@ -292,6 +290,10 @@
 DEVTREE_CONST struct device *pcidev_on_root(uint8_t dev, uint8_t fn);
 DEVTREE_CONST struct bus *pci_root_bus(void);

+/* To be deprecated, avoid using. */
+DEVTREE_CONST struct device *dev_find_slot(unsigned int bus, unsigned int devfn);
+DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func);
+
 void scan_smbus(struct device *bus);
 void scan_generic_bus(struct device *bus);
 void scan_static_bus(struct device *bus);
diff --git a/src/soc/intel/apollolake/include/soc/pci_devs.h b/src/soc/intel/apollolake/include/soc/pci_devs.h
index a334f63..c5eaf4c 100644
--- a/src/soc/intel/apollolake/include/soc/pci_devs.h
+++ b/src/soc/intel/apollolake/include/soc/pci_devs.h
@@ -22,8 +22,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
diff --git a/src/soc/intel/broadwell/include/soc/pci_devs.h b/src/soc/intel/broadwell/include/soc/pci_devs.h
index 522e3eb..ae3e08f 100644
--- a/src/soc/intel/broadwell/include/soc/pci_devs.h
+++ b/src/soc/intel/broadwell/include/soc/pci_devs.h
@@ -25,8 +25,8 @@
 #else
 #include <device/device.h>
 #include <device/pci_def.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #endif

/* System Agent Devices */
diff --git a/src/soc/intel/cannonlake/include/soc/pci_devs.h b/src/soc/intel/cannonlake/include/soc/pci_devs.h
index 1bc028f..33814b0 100644
--- a/src/soc/intel/cannonlake/include/soc/pci_devs.h
+++ b/src/soc/intel/cannonlake/include/soc/pci_devs.h
@@ -24,8 +24,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
diff --git a/src/soc/intel/denverton_ns/include/soc/pci_devs.h b/src/soc/intel/denverton_ns/include/soc/pci_devs.h
index 303ba67..27f9e35 100644
--- a/src/soc/intel/denverton_ns/include/soc/pci_devs.h
+++ b/src/soc/intel/denverton_ns/include/soc/pci_devs.h
@@ -27,8 +27,8 @@
 #if ENV_RAMSTAGE
 #include <device/device.h>
 #include <device/pci_def.h>
-#define _SA_DEV(slot) pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func) pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot) pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func) pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot) PCI_DEV(0, SA_DEV_SLOT_##slot, 0)
 #define _PCH_DEV(slot, func) PCI_DEV(0, PCH_DEV_SLOT_##slot, func)
diff --git a/src/soc/intel/icelake/include/soc/pci_devs.h b/src/soc/intel/icelake/include/soc/pci_devs.h
index 1348f23b..0eddee2 100644
--- a/src/soc/intel/icelake/include/soc/pci_devs.h
+++ b/src/soc/intel/icelake/include/soc/pci_devs.h
@@ -23,8 +23,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
diff --git a/src/soc/intel/skylake/include/soc/pci_devs.h b/src/soc/intel/skylake/include/soc/pci_devs.h
index 5acaaeb..0669ced 100644
--- a/src/soc/intel/skylake/include/soc/pci_devs.h
+++ b/src/soc/intel/skylake/include/soc/pci_devs.h
@@ -24,8 +24,8 @@

#if !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>
-#define _SA_DEV(slot)		pcidev_path_on_root(_SA_DEVFN(slot))
-#define _PCH_DEV(slot, func)	pcidev_path_on_root(_PCH_DEVFN(slot, func))
+#define _SA_DEV(slot)		pcidev_path_on_root_debug(_SA_DEVFN(slot), __func__)
+#define _PCH_DEV(slot, func)	pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
 #else
 #define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
 #define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
@@ -39,7 +39,7 @@

#define SA_DEV_SLOT_PEG		0x01
 #define  SA_DEVFN_PEG(func)	PCI_DEVFN(SA_DEV_SLOT_PEG, func)
-#define  SA_DEV_PEG(func)	pcidev_path_on_root(SA_DEVFN_PEG(func))
+#define  SA_DEV_PEG(func)	pcidev_path_on_root_debug(SA_DEVFN_PEG(func), __func__)
 #define  SA_DEV_PEG0		SA_DEV_PEG(0)
 #define  SA_DEV_PEG1		SA_DEV_PEG(1)
 #define  SA_DEV_PEG2		SA_DEV_PEG(2)

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34285/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34285/7//COMMIT_MSG@13 
PS7, Line 13: may halt with error message 'PCI: dev is NULL!'.
At what stage in the boot sequence was this hit? And from what code?

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c@2... 
PS7, Line 243: 	printk(BIOS_ERR, "BUG: %s requests hidden 00:%02x.%u\n", func, devfn >> 3, devfn & 7);
Why is the dev getting returned NULL? It was in the static device tree right? why wouldn't the linkage of the objects still be there? pcidev_path_behind() is just doing a a find_dev_path() which walks all the children and compares struct device_paths.

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c@2... 
PS7, Line 243: 	printk(BIOS_ERR, "BUG: %s requests hidden 00:%02x.%u\n", func, devfn >> 3, devfn & 7);
> Why is the dev getting returned NULL? It was in the static device tree right? why wouldn't the linka […]

I get it now. It's coming back to be w.r.t. the p2sb. However, that device is unhidden after fsp-s. So what code was causing a look up failure? A different device?

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c@2... 
PS7, Line 243: 	printk(BIOS_ERR, "BUG: %s requests hidden 00:%02x.%u\n", func, devfn >> 3, devfn & 7);
> I get it now. It's coming back to be w.r.t. the p2sb. However, that device is unhidden after fsp-s. […]

It was in the static devicetree, but failing ID query it was removed from the topology links pcidev_path_behind() traverses. The workaround below traverses linked list of all devices, and hidden PCI devices are still present there.

Somewhere I read these hidden PCI devices will respond to all PCI config reads with 0xff's while accepting all PCI config writes. Might have been Nico or Furquan who found some details about this.

Conclusion: PCI config reads are not allowed, but PCI config writes need to go through??

Change in coreboot[master]: soc/intel: Fix regression with hidden PCI devices

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34285 )

Change subject: soc/intel: Fix regression with hidden PCI devices
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/34285/7/src/device/device_const.c@2... 
PS7, Line 243: 	printk(BIOS_ERR, "BUG: %s requests hidden 00:%02x.%u\n", func, devfn >> 3, devfn & 7);
> It was in the static devicetree, but failing ID query it was removed from the topology links pcidev_ […]

Thanks for the description. I pieced it together when digging earlier as well as your clarifications on the mailing list.

They put in a decode filter for certain transaction types and addresses when the p2sb device is hidden. Moreover, there is a fabric id policy check as well, I think, but that comes into play when shedding privileges during boot.