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

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

[Ilpo Järvinen]

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

As for the last patch, I'm not entire sure if it's a step forward or
backward. I guess alternatively it would be possible to add the missing
recursion too if that's preferred but I cannot test the code (and it's
somewhat unclear what that code even attempts to do when considering
all possible topologies).

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 | 43 +++++++++++---------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

-- 
2.39.5

[PATCH 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 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 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 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 simple 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 | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 558866c15e03..b2efc4a90864 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -190,8 +190,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 +207,19 @@ 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;
 		}
 	}
 
-	return -1;
+
+	return ret;
 }
 
 
-- 
2.39.5

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

[Bjorn Helgaas]

On Thu, Oct 17, 2024 at 05:31:31PM +0300, Ilpo Järvinen wrote:
> 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 simple 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 | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
> index 558866c15e03..b2efc4a90864 100644
> --- a/drivers/pci/hotplug/cpqphp_pci.c
> +++ b/drivers/pci/hotplug/cpqphp_pci.c
> @@ -190,8 +190,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 +207,19 @@ 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;

I'm OK with this except that I wonder if we should leave an actual
info or even warning level printk here as a more visible debugging
hint if somebody hits this.  I'm not sure that simply returning 0
would be enough of a hint about why devices below the bridge weren't
found.

>  		}
>  	}
>  
> -	return -1;
> +
> +	return ret;
>  }
>  
>  
> -- 
> 2.39.5
>