Change in coreboot[master]: device: add commentary to dev_find_slot()

[Aaron Durbin (Code Review)]

Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35632 )

Change subject: device: add commentary to dev_find_slot()
......................................................................

device: add commentary to dev_find_slot()

dev_find_slot() can fail sometimes fail to return the
desired device object prior to full PCI enumeration. Comment
the declaration and implementation accordingly to help the
user understand the problem and avoid its usage.

Change-Id: I3fe1f24ff015d3e4f272323947f057e4c910186c
Signed-off-by: Aaron Durbin 
---
M src/device/device_const.c
M src/include/device/device.h
2 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35632/1

diff --git a/src/device/device_const.c b/src/device/device_const.c
index 5143563..27197f2 100644
--- a/src/device/device_const.c
+++ b/src/device/device_const.c
@@ -24,6 +24,12 @@
 /**
  * Given a PCI bus and a devfn number, find the device structure.
  *
+ * Note that this function can return the incorrect device prior
+ * to PCI enumeration because the secondary field of the bus object
+ * is 0. The failing scenario is determined by the order of the
+ * devices in all_devices singly-linked list as well as the time
+ * when this function is called (secondary reflecting topology).
+ *
  * @param bus The bus number.
  * @param devfn A device/function number.
  * @return Pointer to the device structure (if found), 0 otherwise.
diff --git a/src/include/device/device.h b/src/include/device/device.h
index b2221cc..f24e4b2 100644
--- a/src/include/device/device.h
+++ b/src/include/device/device.h
@@ -298,7 +298,14 @@
 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. */
+/* To be deprecated, avoid using.
+ *
+ * Note that this function can return the incorrect device prior
+ * to PCI enumeration because the secondary field of the bus object
+ * is 0. The failing scenario is determined by the order of the
+ * devices in all_devices singly-linked list as well as the time
+ * when this function is called (secondary reflecting topology).
+ */
 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);

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

Let me know if I missed something or you want me to tweak it.

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

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
Move up right after dev_find_slot() implementation?

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
> Move up right after dev_find_slot() implementation?

You want it placed closer to the deprecated function? Or should I ammend the comments and say please use pcidev_path_on_root[_debug]() functions for more correct usage?

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

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
> You want it placed closer to the deprecated function? Or should I ammend the comments and say please […]

Remaining dev_find_slot() calls (all in AMDFAM10) cannot be replaced with pcidev_path_on_root() due to non-zero bus argument. Perhaps we don't want to deprecate this one when dev_find_slot() is removed?

https://review.coreboot.org/c/coreboot/+/35632/1/src/include/device/device.h 
File src/include/device/device.h:

https://review.coreboot.org/c/coreboot/+/35632/1/src/include/device/device.h... 
PS1, Line 310: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func);
Maybe latter should move up if we intent to have it around after dev_find_slot() is gone?

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
> Remaining dev_find_slot() calls (all in AMDFAM10) cannot be replaced with pcidev_path_on_root() due  […]

Can't we provide a variant that supplies bus number? Go through dev_root->link_list to find those devices/buses?

Or am I misunderstanding the AMDFAM10 issue? If I am could you elaborate on it further for me, please?

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

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
> Can't we provide a variant that supplies bus number? Go through dev_root->link_list to find those devices/buses?

IMO we should not support API to search for a device on a specific bus number as we already have pcidev_path_behind(). Even that function is questionable, it can return devices behind bridges before said bridge has had secondary and subordinate bus numbers assigned. In other words, PCI configuration writes would not go through.
> Or am I misunderstanding the AMDFAM10 issue? If I am could you elaborate on it further for me, please?

There is something more, but nobody showing active interest to debug it or in general tidy up that AMDFAM10 codebase. Those dev_find_slot() calls all appear to be pre-devicetree PCI topology tracking.

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
> > Can't we provide a variant that supplies bus number? Go through dev_root->link_list to find those devices/buses?
>
> IMO we should not support API to search for a device on a specific bus number as we already have pcidev_path_behind(). Even that function is questionable, it can return devices behind bridges before said bridge has had secondary and subordinate bus numbers assigned. In other words, PCI configuration writes would not go through.

I was thinking of fixed buses such as nehalem/westmere server sockets where there were multiple static buses in addition to bus 0. In that situation it makes sense, but your point about pcidev_path_behind() would equally work well once we can grab the device easier with a symbol that makes sense.

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

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35632 )

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/35632/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35632/1//COMMIT_MSG@9 
PS1, Line 9: fail
One fail is enough.

https://review.coreboot.org/c/coreboot/+/35632/1//COMMIT_MSG@9 
PS1, Line 9: dev_find_slot() can fail sometimes fail to return the
           : desired device object prior to full PCI enumeration. Comment
           : the declaration and implementation accordingly to help the
           : user understand the problem and avoid its usage.
Allowed text width is 75 characters.

[Aaron Durbin (Code Review)]

Hello Kyösti Mälkki, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

device: add commentary to dev_find_slot()

dev_find_slot() can sometimes fail to return the desired device object
prior to full PCI enumeration. Comment the declaration and
implementation accordingly to help the user understand the problem and
avoid its usage.

Change-Id: I3fe1f24ff015d3e4f272323947f057e4c910186c
Signed-off-by: Aaron Durbin 
---
M src/device/device_const.c
M src/include/device/device.h
2 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/35632/2

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35632/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35632/1//COMMIT_MSG@9 
PS1, Line 9: fail
> One fail is enough.

Done

https://review.coreboot.org/c/coreboot/+/35632/1//COMMIT_MSG@9 
PS1, Line 9: dev_find_slot() can fail sometimes fail to return the
           : desired device object prior to full PCI enumeration. Comment
           : the declaration and implementation accordingly to help the
           : user understand the problem and avoid its usage.
> Allowed text width is 75 characters.

Done

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

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 2: Code-Review+2

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c 
File src/device/device_const.c:

https://review.coreboot.org/c/coreboot/+/35632/1/src/device/device_const.c@2... 
PS1, Line 236: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func)
> > > Can't we provide a variant that supplies bus number? Go through dev_root->link_list to find thos […]

Done

https://review.coreboot.org/c/coreboot/+/35632/1/src/include/device/device.h 
File src/include/device/device.h:

https://review.coreboot.org/c/coreboot/+/35632/1/src/include/device/device.h... 
PS1, Line 310: DEVTREE_CONST struct device *pcidev_path_on_root_debug(pci_devfn_t devfn, const char *func);
> Maybe latter should move up if we intent to have it around after dev_find_slot() is gone?

Done

[Aaron Durbin (Code Review)]

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

Change subject: device: add commentary to dev_find_slot()
......................................................................

Patch Set 2:

I didn't know one had to resolve all the comments now to submit.

[Aaron Durbin (Code Review)]

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35632 )

Change subject: device: add commentary to dev_find_slot()
......................................................................

device: add commentary to dev_find_slot()

dev_find_slot() can sometimes fail to return the desired device object
prior to full PCI enumeration. Comment the declaration and
implementation accordingly to help the user understand the problem and
avoid its usage.

Change-Id: I3fe1f24ff015d3e4f272323947f057e4c910186c
Signed-off-by: Aaron Durbin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35632
Reviewed-by: Kyösti Mälkki 
Tested-by: build bot (Jenkins) 
---
M src/device/device_const.c
M src/include/device/device.h
2 files changed, 14 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, approved

diff --git a/src/device/device_const.c b/src/device/device_const.c
index 5143563..27197f2 100644
--- a/src/device/device_const.c
+++ b/src/device/device_const.c
@@ -24,6 +24,12 @@
 /**
  * Given a PCI bus and a devfn number, find the device structure.
  *
+ * Note that this function can return the incorrect device prior
+ * to PCI enumeration because the secondary field of the bus object
+ * is 0. The failing scenario is determined by the order of the
+ * devices in all_devices singly-linked list as well as the time
+ * when this function is called (secondary reflecting topology).
+ *
  * @param bus The bus number.
  * @param devfn A device/function number.
  * @return Pointer to the device structure (if found), 0 otherwise.
diff --git a/src/include/device/device.h b/src/include/device/device.h
index b2221cc..f24e4b2 100644
--- a/src/include/device/device.h
+++ b/src/include/device/device.h
@@ -298,7 +298,14 @@
 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. */
+/* To be deprecated, avoid using.
+ *
+ * Note that this function can return the incorrect device prior
+ * to PCI enumeration because the secondary field of the bus object
+ * is 0. The failing scenario is determined by the order of the
+ * devices in all_devices singly-linked list as well as the time
+ * when this function is called (secondary reflecting topology).
+ */
 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);