[PATCH v2 0/4] PCI: cpqphp: Fix and cleanups

[PATCH v2 0/4] PCI: cpqphp: Fix and cleanups

[Ilpo Järvinen]

Fix one PCIBIOS_* return value confusion in cpqphp and cleanup a few
other things.

v2:
- Add a warning print if the path missing recursion is executed.

Ilpo Järvinen (4):
  PCI: cpqphp: Fix PCIBIOS_* return value confusions
  PCI: cpqphp: Use pci_bus_read_dev_vendor_id() to detect presence
  PCI: cpqphp: Use define to read class/revision dword
  PCI: cpqphp: Simplify PCI_ScanBusForNonBridge()

 drivers/pci/hotplug/cpqphp_pci.c | 47 +++++++++++++-------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

-- 
2.39.5

[PATCH v2 1/4] PCI: cpqphp: Fix PCIBIOS_* return value confusions

[Ilpo Järvinen]

Code in and related to PCI_RefinedAccessConfig() has 3 types of return
type confusion:
 - PCI_RefinedAccessConfig() tests pci_bus_read_config_dword() return
   value against -1.
 - PCI_RefinedAccessConfig() returns both -1 and PCIBIOS_* return codes.
 - Callers of PCI_RefinedAccessConfig() only test for -1.

Make PCI_RefinedAccessConfig() return PCIBIOS_* codes consistently and
adapt callers accordingly.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/cpqphp_pci.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 718bc6cf12cb..974c7db3265b 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -135,11 +135,13 @@ int cpqhp_unconfigure_device(struct pci_func *func)
 static int PCI_RefinedAccessConfig(struct pci_bus *bus, unsigned int devfn, u8 offset, u32 *value)
 {
 	u32 vendID = 0;
+	int ret;
 
-	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &vendID) == -1)
-		return -1;
+	ret = pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &vendID);
+	if (ret != PCIBIOS_SUCCESSFUL)
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	if (PCI_POSSIBLE_ERROR(vendID))
-		return -1;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_read_config_dword(bus, devfn, offset, value);
 }
 
@@ -202,13 +204,15 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 {
 	u16 tdevice;
 	u32 work;
+	int ret;
 	u8 tbus;
 
 	ctrl->pci_bus->number = bus_num;
 
 	for (tdevice = 0; tdevice < 0xFF; tdevice++) {
 		/* Scan for access first */
-		if (PCI_RefinedAccessConfig(ctrl->pci_bus, tdevice, 0x08, &work) == -1)
+		ret = PCI_RefinedAccessConfig(ctrl->pci_bus, tdevice, 0x08, &work);
+		if (ret)
 			continue;
 		dbg("Looking for nonbridge bus_num %d dev_num %d\n", bus_num, tdevice);
 		/* Yep we got one. Not a bridge ? */
@@ -220,7 +224,8 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 	}
 	for (tdevice = 0; tdevice < 0xFF; tdevice++) {
 		/* Scan for access first */
-		if (PCI_RefinedAccessConfig(ctrl->pci_bus, tdevice, 0x08, &work) == -1)
+		ret = PCI_RefinedAccessConfig(ctrl->pci_bus, tdevice, 0x08, &work);
+		if (ret)
 			continue;
 		dbg("Looking for bridge bus_num %d dev_num %d\n", bus_num, tdevice);
 		/* Yep we got one. bridge ? */
-- 
2.39.5

[PATCH v2 2/4] PCI: cpqphp: Use pci_bus_read_dev_vendor_id() to detect presence

[Ilpo Järvinen]

The intent of the first part in PCI_RefinedAccessConfig() is to
read Vendor ID register and detect presense of the device that way.

Remove PCI_RefinedAccessConfig() (which was not named very helpfully to
begin with) and replace the call with pci_bus_read_dev_vendor_id() +
read config because it makes the logic more obvious at the caller side.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/cpqphp_pci.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 974c7db3265b..7844007dbc86 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -132,20 +132,6 @@ int cpqhp_unconfigure_device(struct pci_func *func)
 	return 0;
 }
 
-static int PCI_RefinedAccessConfig(struct pci_bus *bus, unsigned int devfn, u8 offset, u32 *value)
-{
-	u32 vendID = 0;
-	int ret;
-
-	ret = pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &vendID);
-	if (ret != PCIBIOS_SUCCESSFUL)
-		return PCIBIOS_DEVICE_NOT_FOUND;
-	if (PCI_POSSIBLE_ERROR(vendID))
-		return PCIBIOS_DEVICE_NOT_FOUND;
-	return pci_bus_read_config_dword(bus, devfn, offset, value);
-}
-
-
 /*
  * cpqhp_set_irq
  *
@@ -211,7 +197,9 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 
 	for (tdevice = 0; tdevice < 0xFF; tdevice++) {
 		/* Scan for access first */
-		ret = PCI_RefinedAccessConfig(ctrl->pci_bus, tdevice, 0x08, &work);
+		if (!pci_bus_read_dev_vendor_id(ctrl->pci_bus, tdevice, &work, 0))
+			continue;
+		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, 0x08, &work);
 		if (ret)
 			continue;
 		dbg("Looking for nonbridge bus_num %d dev_num %d\n", bus_num, tdevice);
@@ -224,7 +212,9 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 	}
 	for (tdevice = 0; tdevice < 0xFF; tdevice++) {
 		/* Scan for access first */
-		ret = PCI_RefinedAccessConfig(ctrl->pci_bus, tdevice, 0x08, &work);
+		if (!pci_bus_read_dev_vendor_id(ctrl->pci_bus, tdevice, &work, 0))
+			continue;
+		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, 0x08, &work);
 		if (ret)
 			continue;
 		dbg("Looking for bridge bus_num %d dev_num %d\n", bus_num, tdevice);
-- 
2.39.5

[PATCH v2 3/4] PCI: cpqphp: Use define to read class/revision dword

[Ilpo Järvinen]

Replace literal 0x08 with PCI_CLASS_REVISION.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/cpqphp_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 7844007dbc86..558866c15e03 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -199,7 +199,7 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 		/* Scan for access first */
 		if (!pci_bus_read_dev_vendor_id(ctrl->pci_bus, tdevice, &work, 0))
 			continue;
-		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, 0x08, &work);
+		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, PCI_CLASS_REVISION, &work);
 		if (ret)
 			continue;
 		dbg("Looking for nonbridge bus_num %d dev_num %d\n", bus_num, tdevice);
@@ -214,7 +214,7 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 		/* Scan for access first */
 		if (!pci_bus_read_dev_vendor_id(ctrl->pci_bus, tdevice, &work, 0))
 			continue;
-		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, 0x08, &work);
+		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, PCI_CLASS_REVISION, &work);
 		if (ret)
 			continue;
 		dbg("Looking for bridge bus_num %d dev_num %d\n", bus_num, tdevice);
-- 
2.39.5

[PATCH v2 4/4] PCI: cpqphp: Simplify PCI_ScanBusForNonBridge()

[Ilpo Järvinen]

PCI_ScanBusForNonBridge() has two loops, first searching for
non-bridges and second that looks for bridges. The second loop has
hints in a debug print it should do recursion for buses underneath the
bridge but no recursion is attempted.

Since the second loop is quite useless in its current form, just
eliminate it. This code hasn't been touched for very long time so
either it's unused or the missing parts are not important enough for
anyone to attempt to add them.

Leave only a warning print and comment about the missing recursion
for the unlikely case that somebody comes across the lack of
functionality. In any case, search whether an endpoint exists
downstream of a bridge sounds generic enough to belong to core so if
the functionality is to be extended it should probably be moved into
PCI core.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/cpqphp_pci.c | 34 ++++++++++++++------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 558866c15e03..ef7534a3ca40 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -12,8 +12,11 @@
  *
  */
 
+#define pr_fmt(fmt) "cpqphp: " fmt
+
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/printk.h>
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
@@ -190,8 +193,7 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 {
 	u16 tdevice;
 	u32 work;
-	int ret;
-	u8 tbus;
+	int ret = -1;
 
 	ctrl->pci_bus->number = bus_num;
 
@@ -208,26 +210,20 @@ static int PCI_ScanBusForNonBridge(struct controller *ctrl, u8 bus_num, u8 *dev_
 			*dev_num = tdevice;
 			dbg("found it !\n");
 			return 0;
-		}
-	}
-	for (tdevice = 0; tdevice < 0xFF; tdevice++) {
-		/* Scan for access first */
-		if (!pci_bus_read_dev_vendor_id(ctrl->pci_bus, tdevice, &work, 0))
-			continue;
-		ret = pci_bus_read_config_dword(ctrl->pci_bus, tdevice, PCI_CLASS_REVISION, &work);
-		if (ret)
-			continue;
-		dbg("Looking for bridge bus_num %d dev_num %d\n", bus_num, tdevice);
-		/* Yep we got one. bridge ? */
-		if ((work >> 8) == PCI_TO_PCI_BRIDGE_CLASS) {
-			pci_bus_read_config_byte(ctrl->pci_bus, PCI_DEVFN(tdevice, 0), PCI_SECONDARY_BUS, &tbus);
-			/* XXX: no recursion, wtf? */
-			dbg("Recurse on bus_num %d tdevice %d\n", tbus, tdevice);
-			return 0;
+		} else {
+			/*
+			 * XXX: Code whose debug printout indicated
+			 * recursion to buses underneath bridges might be
+			 * necessary was removed because it never did
+			 * any recursion.
+			 */
+			ret = 0;
+			pr_warn("missing feature: bridge scan recursion not implemented\n");
 		}
 	}
 
-	return -1;
+
+	return ret;
 }
 
 
-- 
2.39.5

Re: [PATCH v2 0/4] PCI: cpqphp: Fix and cleanups

[Bjorn Helgaas]

On Tue, Oct 22, 2024 at 12:11:36PM +0300, Ilpo Järvinen wrote:
> Fix one PCIBIOS_* return value confusion in cpqphp and cleanup a few
> other things.
> 
> v2:
> - Add a warning print if the path missing recursion is executed.
> 
> Ilpo Järvinen (4):
>   PCI: cpqphp: Fix PCIBIOS_* return value confusions
>   PCI: cpqphp: Use pci_bus_read_dev_vendor_id() to detect presence
>   PCI: cpqphp: Use define to read class/revision dword
>   PCI: cpqphp: Simplify PCI_ScanBusForNonBridge()
> 
>  drivers/pci/hotplug/cpqphp_pci.c | 47 +++++++++++++-------------------
>  1 file changed, 19 insertions(+), 28 deletions(-)

Applied to pci/hotplug for v6.13, thanks!